diff mbox

[projects] wpa_actiond: Wait for three "failed" PONGs before disconnecting

Message ID 20170821093420.26303-1-emil.l.velikov@gmail.com
State Accepted
Headers show

Commit Message

Emil Velikov via arch-projects Aug. 21, 2017, 9:34 a.m. UTC 
When the system is very low on resources, select() may return 0 even
when wpa_supplicant is alive and kicking.

Don't disconnect as soon at that occurs - wait three times and clearly
log it.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
Hi all,

Using [projects] since [wpa_actiond] gets kicked by the email filter.
Can anyone update the message to include a list of supported tags, or 
link to an article/wiki with information?

Back on topic:

This is a small patch I had for 2 months and I thought about getting
upstreamed.

In particular - on certain workloads my system can get really low on
memory (and available fds).

With the patch in place, wpa_actiond no longer kills my connection
although in practise I never seen two consecutive "wpa_supplicant failed
to reply (PONG)" messages.

My patch could be completely bonkers, so any suggestions how to address
this correctly will be appreciated.
---
 wpa_actiond.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Emil Velikov via arch-projects Sept. 29, 2017, 1:03 p.m. UTC | #1 
On 21 August 2017 at 10:34, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> When the system is very low on resources, select() may return 0 even
> when wpa_supplicant is alive and kicking.
>
> Don't disconnect as soon at that occurs - wait three times and clearly
> log it.
>
> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
> ---
> Hi all,
>
> Using [projects] since [wpa_actiond] gets kicked by the email filter.
> Can anyone update the message to include a list of supported tags, or
> link to an article/wiki with information?
>
> Back on topic:
>
> This is a small patch I had for 2 months and I thought about getting
> upstreamed.
>
> In particular - on certain workloads my system can get really low on
> memory (and available fds).
>
> With the patch in place, wpa_actiond no longer kills my connection
> although in practise I never seen two consecutive "wpa_supplicant failed
> to reply (PONG)" messages.
>
> My patch could be completely bonkers, so any suggestions how to address
> this correctly will be appreciated.
> ---
>  wpa_actiond.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/wpa_actiond.c b/wpa_actiond.c
> index d60d885..03a9d7f 100644
> --- a/wpa_actiond.c
> +++ b/wpa_actiond.c
> @@ -292,6 +292,7 @@ static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
>    /* path to control socket */
>    char *ctrlsock = NULL;
>    int ctrlsocklen;
> +  int pong_failures = 0;
>
>    ssid[0] = '\0';
>    old_ssid[0] = '\0';
> @@ -359,6 +360,7 @@ static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
>          }
>          logevent(WPA_ACTIOND_LOG_TERMINATE, iface, "");
>          state = WPA_ACTIOND_STATE_TERMINATED;
> +        pong_failures = 0;
>          break;
>        case 0:
>          if (state == WPA_ACTIOND_STATE_CONNECTION_LOST) {
> @@ -372,8 +374,15 @@ static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
>              reply_len = 0;
>            reply[reply_len] = '\0';
>            if(!str_match(reply, "PONG")) {
> +            if (pong_failures <= 3) {
> +              logevent(WPA_ACTIOND_LOG_CUSTOM_ERROR, iface, "wpa_supplicant failed to reply (PONG)");
> +              pong_failures++;
> +              break;
> +            }
> +
>              /* supplicant has been terminated */
>              if(state == WPA_ACTIOND_STATE_CONNECTED || state == WPA_ACTIOND_STATE_CONNECTION_LOST) {
> +              logevent(WPA_ACTIOND_LOG_CUSTOM_ERROR, iface, "wpa_supplicant failed to reply three times in a row - disconnecting");
>                logevent(WPA_ACTIOND_LOG_DISCONNECTED, iface, ssid);
>                action(WPA_ACTIOND_ACTION_DISCONNECT, iface, ssid, idstr, wpa_ctrl_get_fd(ctrl), script);
>              }
> @@ -381,6 +390,7 @@ static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
>              state = WPA_ACTIOND_STATE_TERMINATED;
>            }
>          }
> +        pong_failures = 0;
>          break;
>        default:
>          /* event received */
> @@ -446,6 +456,7 @@ static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
>              /* we are not interested in this event */
>              break;
>          }
> +        pong_failures = 0;
>      }
>    }
>
> --
Humble ping anyone?

Thanks
Emil
Bartłomiej Piotrowski Oct. 9, 2017, 8:22 a.m. UTC | #2 
On 2017-09-29 15:03, Emil Velikov via arch-projects wrote:
> On 21 August 2017 at 10:34, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> When the system is very low on resources, select() may return 0 even
>> when wpa_supplicant is alive and kicking.
>>
>> Don't disconnect as soon at that occurs - wait three times and clearly
>> log it.
>>
>> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
>> ---
>> Hi all,
>>
>> Using [projects] since [wpa_actiond] gets kicked by the email filter.
>> Can anyone update the message to include a list of supported tags, or
>> link to an article/wiki with information?
>>
>> Back on topic:
>>
>> This is a small patch I had for 2 months and I thought about getting
>> upstreamed.
>>
>> In particular - on certain workloads my system can get really low on
>> memory (and available fds).
>>
>> With the patch in place, wpa_actiond no longer kills my connection
>> although in practise I never seen two consecutive "wpa_supplicant failed
>> to reply (PONG)" messages.
>>
>> My patch could be completely bonkers, so any suggestions how to address
>> this correctly will be appreciated.
>> ---
>>  wpa_actiond.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/wpa_actiond.c b/wpa_actiond.c
>> index d60d885..03a9d7f 100644
>> --- a/wpa_actiond.c
>> +++ b/wpa_actiond.c
>> @@ -292,6 +292,7 @@ static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
>>    /* path to control socket */
>>    char *ctrlsock = NULL;
>>    int ctrlsocklen;
>> +  int pong_failures = 0;
>>
>>    ssid[0] = '\0';
>>    old_ssid[0] = '\0';
>> @@ -359,6 +360,7 @@ static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
>>          }
>>          logevent(WPA_ACTIOND_LOG_TERMINATE, iface, "");
>>          state = WPA_ACTIOND_STATE_TERMINATED;
>> +        pong_failures = 0;
>>          break;
>>        case 0:
>>          if (state == WPA_ACTIOND_STATE_CONNECTION_LOST) {
>> @@ -372,8 +374,15 @@ static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
>>              reply_len = 0;
>>            reply[reply_len] = '\0';
>>            if(!str_match(reply, "PONG")) {
>> +            if (pong_failures <= 3) {
>> +              logevent(WPA_ACTIOND_LOG_CUSTOM_ERROR, iface, "wpa_supplicant failed to reply (PONG)");
>> +              pong_failures++;
>> +              break;
>> +            }
>> +
>>              /* supplicant has been terminated */
>>              if(state == WPA_ACTIOND_STATE_CONNECTED || state == WPA_ACTIOND_STATE_CONNECTION_LOST) {
>> +              logevent(WPA_ACTIOND_LOG_CUSTOM_ERROR, iface, "wpa_supplicant failed to reply three times in a row - disconnecting");
>>                logevent(WPA_ACTIOND_LOG_DISCONNECTED, iface, ssid);
>>                action(WPA_ACTIOND_ACTION_DISCONNECT, iface, ssid, idstr, wpa_ctrl_get_fd(ctrl), script);
>>              }
>> @@ -381,6 +390,7 @@ static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
>>              state = WPA_ACTIOND_STATE_TERMINATED;
>>            }
>>          }
>> +        pong_failures = 0;
>>          break;
>>        default:
>>          /* event received */
>> @@ -446,6 +456,7 @@ static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
>>              /* we are not interested in this event */
>>              break;
>>          }
>> +        pong_failures = 0;
>>      }
>>    }
>>
>> --
> Humble ping anyone?
> 
> Thanks
> Emil
> 

I don't think anyone maintains wpa_actiond these days. I will look at
your change this week, but forking it completely wouldn't be insane.

Bartłomiej
Emil Velikov via arch-projects Oct. 23, 2017, 12:56 p.m. UTC | #3 
On 9 October 2017 at 09:22, Bartłomiej Piotrowski
<bpiotrowski@archlinux.org> wrote:
> On 2017-09-29 15:03, Emil Velikov via arch-projects wrote:
>> On 21 August 2017 at 10:34, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>> When the system is very low on resources, select() may return 0 even
>>> when wpa_supplicant is alive and kicking.
>>>
>>> Don't disconnect as soon at that occurs - wait three times and clearly
>>> log it.
>>>
>>> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
>>> ---
>>> Hi all,
>>>
>>> Using [projects] since [wpa_actiond] gets kicked by the email filter.
>>> Can anyone update the message to include a list of supported tags, or
>>> link to an article/wiki with information?
>>>
>>> Back on topic:
>>>
>>> This is a small patch I had for 2 months and I thought about getting
>>> upstreamed.
>>>
>>> In particular - on certain workloads my system can get really low on
>>> memory (and available fds).
>>>
>>> With the patch in place, wpa_actiond no longer kills my connection
>>> although in practise I never seen two consecutive "wpa_supplicant failed
>>> to reply (PONG)" messages.
>>>
>>> My patch could be completely bonkers, so any suggestions how to address
>>> this correctly will be appreciated.
>>> ---
>>>  wpa_actiond.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/wpa_actiond.c b/wpa_actiond.c
>>> index d60d885..03a9d7f 100644
>>> --- a/wpa_actiond.c
>>> +++ b/wpa_actiond.c
>>> @@ -292,6 +292,7 @@ static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
>>>    /* path to control socket */
>>>    char *ctrlsock = NULL;
>>>    int ctrlsocklen;
>>> +  int pong_failures = 0;
>>>
>>>    ssid[0] = '\0';
>>>    old_ssid[0] = '\0';
>>> @@ -359,6 +360,7 @@ static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
>>>          }
>>>          logevent(WPA_ACTIOND_LOG_TERMINATE, iface, "");
>>>          state = WPA_ACTIOND_STATE_TERMINATED;
>>> +        pong_failures = 0;
>>>          break;
>>>        case 0:
>>>          if (state == WPA_ACTIOND_STATE_CONNECTION_LOST) {
>>> @@ -372,8 +374,15 @@ static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
>>>              reply_len = 0;
>>>            reply[reply_len] = '\0';
>>>            if(!str_match(reply, "PONG")) {
>>> +            if (pong_failures <= 3) {
>>> +              logevent(WPA_ACTIOND_LOG_CUSTOM_ERROR, iface, "wpa_supplicant failed to reply (PONG)");
>>> +              pong_failures++;
>>> +              break;
>>> +            }
>>> +
>>>              /* supplicant has been terminated */
>>>              if(state == WPA_ACTIOND_STATE_CONNECTED || state == WPA_ACTIOND_STATE_CONNECTION_LOST) {
>>> +              logevent(WPA_ACTIOND_LOG_CUSTOM_ERROR, iface, "wpa_supplicant failed to reply three times in a row - disconnecting");
>>>                logevent(WPA_ACTIOND_LOG_DISCONNECTED, iface, ssid);
>>>                action(WPA_ACTIOND_ACTION_DISCONNECT, iface, ssid, idstr, wpa_ctrl_get_fd(ctrl), script);
>>>              }
>>> @@ -381,6 +390,7 @@ static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
>>>              state = WPA_ACTIOND_STATE_TERMINATED;
>>>            }
>>>          }
>>> +        pong_failures = 0;
>>>          break;
>>>        default:
>>>          /* event received */
>>> @@ -446,6 +456,7 @@ static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
>>>              /* we are not interested in this event */
>>>              break;
>>>          }
>>> +        pong_failures = 0;
>>>      }
>>>    }
>>>
>>> --
>> Humble ping anyone?
>>
>> Thanks
>> Emil
>>
>
> I don't think anyone maintains wpa_actiond these days. I will look at
> your change this week, but forking it completely wouldn't be insane.
>
Thanks Bartłomiej.

Forking the project for such trivial corner case seems like an overkill.

To the best of your knowledge, is Arch moving away from this project/package?
Or simply wpa_actiond has reached its peak and development (hence
maintainer) is MIA?

-Emil
Bartłomiej Piotrowski Nov. 14, 2017, 11:47 a.m. UTC | #4 
On 2017-10-23 14:56, Emil Velikov via arch-projects wrote:
> Thanks Bartłomiej.
> 
> Forking the project for such trivial corner case seems like an overkill.
> 
> To the best of your knowledge, is Arch moving away from this project/package?
> Or simply wpa_actiond has reached its peak and development (hence
> maintainer) is MIA?
> 
> -Emil
> 

Long overdue, but pushed your change to master, thanks. I will backport
it to the package later.

It never was a core project, more a Thomas' side project. I have never
used it, but looking at low commit number, it probably just serves its
purpose alright.

Bartłomiej
diff mbox

Patch

diff --git a/wpa_actiond.c b/wpa_actiond.c
index d60d885..03a9d7f 100644
--- a/wpa_actiond.c
+++ b/wpa_actiond.c
@@ -292,6 +292,7 @@  static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
   /* path to control socket */
   char *ctrlsock = NULL;
   int ctrlsocklen;
+  int pong_failures = 0;
 
   ssid[0] = '\0';
   old_ssid[0] = '\0';
@@ -359,6 +360,7 @@  static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
         }
         logevent(WPA_ACTIOND_LOG_TERMINATE, iface, "");
         state = WPA_ACTIOND_STATE_TERMINATED;
+        pong_failures = 0;
         break;
       case 0:
         if (state == WPA_ACTIOND_STATE_CONNECTION_LOST) {
@@ -372,8 +374,15 @@  static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
             reply_len = 0;
           reply[reply_len] = '\0';
           if(!str_match(reply, "PONG")) {
+            if (pong_failures <= 3) {
+              logevent(WPA_ACTIOND_LOG_CUSTOM_ERROR, iface, "wpa_supplicant failed to reply (PONG)");
+              pong_failures++;
+              break;
+            }
+
             /* supplicant has been terminated */
             if(state == WPA_ACTIOND_STATE_CONNECTED || state == WPA_ACTIOND_STATE_CONNECTION_LOST) {
+              logevent(WPA_ACTIOND_LOG_CUSTOM_ERROR, iface, "wpa_supplicant failed to reply three times in a row - disconnecting");
               logevent(WPA_ACTIOND_LOG_DISCONNECTED, iface, ssid);
               action(WPA_ACTIOND_ACTION_DISCONNECT, iface, ssid, idstr, wpa_ctrl_get_fd(ctrl), script);
             }
@@ -381,6 +390,7 @@  static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
             state = WPA_ACTIOND_STATE_TERMINATED;
           }
         }
+        pong_failures = 0;
         break;
       default:
         /* event received */
@@ -446,6 +456,7 @@  static void loop(const char *iface, const char *ctrlpath, const int disconnect_t
             /* we are not interested in this event */
             break;
         }
+        pong_failures = 0;
     }
   }