Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.01.org/mailman/listinfo/connman
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. Re: Disabling the network on Disconnect Notification (Adam Moore)
2. Re: [PATCH v3] gsupplicant: Mem leak in wpa_s because
"RemoveNetwork" not called (Naveen Singh)
3. Re: [PATCH v3] gsupplicant: Mem leak in wpa_s because
"RemoveNetwork" not called (Patrik Flykt)
4. RE: nftables (Zheng, Wu)
----------------------------------------------------------------------
Message: 1
Date: Tue, 23 Feb 2016 03:15:19 +0000
From: Adam Moore <[email protected]>
To: Patrik Flykt <[email protected]>, Naveen Singh
<[email protected]>
Cc: "[email protected]" <[email protected]>
Subject: Re: Disabling the network on Disconnect Notification
Message-ID: <d2f10d13.1ca4d%[email protected]>
Content-Type: text/plain; charset="iso-8859-1"
Hi,
On 2/22/16, 12:18 AM, "connman on behalf of Patrik Flykt"
<[email protected] on behalf of [email protected]>
wrote:
>
> Hi,
>
>On Fri, 2016-02-19 at 22:50 -0800, Naveen Singh wrote:
>> On Thu, Feb 18, 2016 at 11:20 PM, Patrik Flykt
>> <[email protected]> wrote:
>> >
>> > Hi,
>> >
>> > On Thu, 2016-02-18 at 22:52 -0800, Naveen Singh wrote:
>> >> Hi All
>> >> On the disconnect notification from wpa_supplicant (when the
>>interface
>> >> state becomes G_SUPPLICANT_STATE_DISCONNECTED), connman disables the
>> >> network (SSID). This is done from interface_state function in
>> >> plugins/wifi.c:
>> >>
>> >> if
>>(g_supplicant_interface_enable_selected_network(interface,
>> >> FALSE) != 0)
>> >> DBG("Could not disable selected network");
>> >>
>> >> This severely impacts wpa_supplicant's attempt of getting device
>> >> connected back to WiFi. Lot of enterprise AP now have band steering
>> >> and/or load balancing enabled as a result of which they keep sending
>> >> deauth frames to client till it picks the correct BSSID for
>> >> connection. Now if wpa_supplicant is left alone, it would properly
>> >> converge to the right BSSID where the association would stick (by
>> >> blacklisting a BSSID if it does not allow or deny the connection and
>> >> trying the next one)
>> >>
>> >> But now with the very first disconnect notification, connman disables
>> >> the whole network which ends up disabling the whole SSID and
>> >> wpa_supplicant does not continue with its connection attempts.
>> >> Next time when this SSID gets enabled from connman (as a part of next
>> >> connect), the same story repeats. End result device sometimes never
>> >> get connected. The above piece of code is impacting L2 level roaming
>> >> decisions in connman. And I do not think connman has any intention or
>> >> desire to interfere in any BSSID level roaming decision of
>> >> wpa_supplicant.
>> >>
>> >> Even for the AP which sends deauth with reason code 6/7/4,
>> >> wpa_supplicant has internal logic to get device connected fast. This
>> >> also gets impacted quite a bit because of disable logic.
>> >>
>> >> Even logs look very confusing at the time of disconnection. You see a
>> >> successful association (because of wpa_s connection attempt) followed
>> >> by an internally generated disconnect.
>> >>
>> >> I did a test by removing this code and following are the events that
>> >> happened on receiving deauth:
>> >>
>> >> 1) Client receives deauth frame (reason code 15)
>> >> 2) wpa_supplicant blacklists the current AP and schedule a scan.
>> >> 3) It generates disconnect notification to connman
>> >
>> > At this point a properly designed API would not tell that the network
>> > disconnected, as it obviously is looking for another AP to connect to.
>> > wpa_supplicant does not have the worlds greatest API design in place
>>as
>> > we've seen with other patches in the recent past...
>> >
>> I had a discussion with Jouni on this and he agreed in principle that
>> we need two API. Once when the initial connection is lost and second
>> when wpa_supplicant has tried connecting with all the known BSSID. But
>> the problem is that wpa_supplicant will never stop try connecting it.
>> If all the known BSSID are blacklisted and still device cannot get
>> connected, wpa_s will clear the blacklist and try again. So there is
>> no way we can say when to generate the second event.
>
>
>This is fixed by having wpa_supplicant getting its act together and
>informing any client of a disconnected network once the list of known
>BSSIDs has been tried once. No earlier and no later. The client, in this
>case ConnMan, is not supposed to know more of the network details than
>wpa_supplicant.
A couple of concerns:
1) When trying to reconnect to a troubled ESS containing many BSS - won?t
we be held back for a variable amount of time without being notified of a
loss of connectivity?
2) It feels like a step in the wrong direction to suppress already
existing signaling information from supplicant. More information about
what is happening gives a connection/network manager more flexibility in
deciding how to react. (though maybe this signaling needs to become even
more verbose by adding reason codes, etc)
supplicant may know more about the details of the current ESS, but Connman
has potentially more valuable information: additional configured Wi-Fi
networks, technologies, sessions, user preferences, and is ideally suited
to make service transition decisions. There may be cases where it is
better to fall back to WWAN or an alternate WLAN network at the first sign
of trouble or cases where it is better to give a service a recovery grace
period. (I?m reminded of QCOM's Connectivity Engine/CnE work from few
years back as an example how complex the decision to transition between
WLAN and WWAN has become on some smartphones.)
>
>> One of the proposal that came out from that discussion was to not
>> immediately handle the disconnect notification from wpa_s. Delay it by
>> say 20 sec, and if the connected event happens before the timer expiry
>> cancel the timer and do nothing but if nothing happens for 20 sec,
>> make the state disconnected. But in my view it is not correct to do
>> first because for that 20 sec data path is still ON (as application is
>> not notified) even if there is no L2 level connection. Also I am not
>> sure what should be the proper value for timeout.
>
>This is a solution that won't work. It leaves ConnMan without WiFi
>connectivity for 20s after going out of range or loosing the connection
>otherwise. So one has to wait 20s for a reconnection when going out of
>range from one's home WiFi? Why was this even proposed in the first
>place...
My interpretation of the suggestion is that it gives supplicant a chance
to recover more quickly on its own given that it only knows of one
configured network at a time - the suggestion makes more sense to me if
you assume there are no other technologies to fall back on. I agree 20
seconds isn?t ideal for a mobile device or maybe any device with a WWAN
capability, but it might be fine for an in-home/stationary device with a
few configured Wi-Fi services. A device with a single configured Wi-Fi
service might be best served by doing nothing and letting supplicant try
to connect to its Connman provided network forever.
The best answer may lie somewhere between unconditionally disabling the
service (as done today), and giving supplicant a long grace period to
recover, and may also need to factor in the device?s capabilities and
current configuration.
-Adam
>
>> >> 4) Connman released its current IP.
>> >> 5) wpa_supplicant gets the scan result and finds all the AP
>> >> advertising the same SSID.
>> >> 6) It proceeds with the connection with next BSSID.
>> >> 7) It connects back with this BSSID
>> >> 8) Connman gets device the IP address
>> >>
>> >> If deauth is received with reason code 6 or 7 or 4 then there is even
>> >> no scan and device gets connected real fast.
>> >>
>> >> Let me know your thoughts on this. I will send my patch soon
>> >
>> > ConnMan can implement the workaround not to drop the connection when
>>it
>> > is obvious that wpa_supplicant will be looking for another AP to
>>connect
>> > to. Then again we don't want ConnMan to be stuck with connecting
>> > forever, either.
>>
>> I think the only change my patch would do is to not disable the
>> network. Everything still would be handled the same way. As far as
>> connman is concerned the state is disconnected and it has even
>> released the IP address.
>
>In addition the service is disconnected.
>
>> On the next connection connman immediately
>> proceeds with DHCP REQ (asking for same IP). And I think
>> conservatively speaking it is probably a good thing to get a new IP
>> address than sitting on the old IP address (Some user may have two AP
>> in their house broadcasting same SSID but both of them are acting as a
>> DHCP server).
>
>At this point ConnMan has already selected another service, if
>available, so there is no guarantees that the same WiFi will be
>reconnected.
>
>The only thing that might work here is that one or more of the reason
>codes 6, 7 or 4 will not be sent in any other circumstances than doing
>this unnecessary disconnect notification from wpa_supplicant and is an
>indication that the notification can be ignored. Probably it does not
>work out that nicely.
>
>Cheers,
>
> Patrik
>
>
>_______________________________________________
>connman mailing list
>[email protected]
>https://lists.01.org/mailman/listinfo/connman
Statement of Confidentiality
The contents of this e-mail message and any attachments are confidential and
are intended solely for the addressee. The information may also be legally
privileged. This transmission is sent in trust, and the sole purpose of
delivery to the intended recipient. If you have received this transmission in
error, any use, reproduction or dissemination of this transmission is strictly
prohibited. If you are not the intended recipient, please immediately notify
the sender by reply e-mail or at 508.683.2500 and delete this message and its
attachments, if any.
------------------------------
Message: 2
Date: Mon, 22 Feb 2016 19:44:04 -0800
From: Naveen Singh <[email protected]>
To: Patrik Flykt <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH v3] gsupplicant: Mem leak in wpa_s because
"RemoveNetwork" not called
Message-ID:
<CAGTDzK=tp0lotqppxxmq0zjo6r2ght18gzkx7haxrqdrcpj...@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Hi
On Mon, Feb 22, 2016 at 12:58 AM, Patrik Flykt
<[email protected]> wrote:
>
> Hi,
>
> On Thu, 2016-02-18 at 22:11 -0800, Naveen Singh wrote:
>> From: nasingh <[email protected]>
>>
>> Connman did not call netwok_remove in case AP deauthenticated client causing
>> wpa_s to re-allocate the ssid pointer even if the next connection attempt
>> is for the same SSID. This change ensures that at the time of connection
>> (DBUS Method call AddNetwork) if the network is found not removed, it calls
>> the dbus API to remove the network and once network is removed, proceed with
>> the connection.
>>
>> Tested by running a deauth loop script at the AP end and ensure that wpa_s
>> does
>> not allocate memory for struct wpa_ssid for all the subsequent
>> connection attempts. During the test memory usage of wpa_s is monitored.
>> ---
>> gsupplicant/supplicant.c | 156
>> ++++++++++++++++++++++++++++++++---------------
>> 1 file changed, 108 insertions(+), 48 deletions(-)
>>
>> diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
>> index 342cb01..f2065b2 100644
>> --- a/gsupplicant/supplicant.c
>> +++ b/gsupplicant/supplicant.c
>> @@ -249,6 +249,50 @@ struct _GSupplicantGroup {
>> GSList *members;
>> };
>>
>> +struct interface_data {
>> + GSupplicantInterface *interface;
>> + char *path; /* Interface path cannot be taken from interface (above) as
>> + * it might have been freed already.
>> + */
>>+
>> + GSupplicantInterfaceCallback callback;
>> + void *user_data;
>> + bool network_remove_in_progress;
>> + GSupplicantSSID *ssid;
>> +};
>> +
>> +struct interface_create_data {
>> + char *ifname;
>> + char *driver;
>> + char *bridge;
>> + GSupplicantInterface *interface;
>> + GSupplicantInterfaceCallback callback;
>> + void *user_data;
>> +};
>> +
>> +struct interface_connect_data {
>> + GSupplicantInterface *interface;
>> + char *path;
>> + GSupplicantInterfaceCallback callback;
>> + void *user_data;
>> + union {
>> + GSupplicantSSID *ssid;
>> + GSupplicantPeerParams *peer;
>> + };
>> +};
>> +
>> +struct interface_scan_data {
>> + GSupplicantInterface *interface;
>> + char *path;
>> + GSupplicantInterfaceCallback callback;
>> + GSupplicantScanParams *scan_params;
>> + void *user_data;
>> +};
>> +
>> +static int network_remove(struct interface_data *data);
>> +static void network_remove_params(DBusMessageIter *iter, void *user_data);
>> +static void network_remove_result(const char *error,
>> + DBusMessageIter *iter, void *user_data);
>
> The two latter function declarations are not needed.
>
>> static inline void debug(const char *format, ...)
>> {
>> char str[256];
>> @@ -3476,43 +3520,6 @@ GSupplicantPeer
>> *g_supplicant_interface_peer_lookup(GSupplicantInterface *interf
>> return peer;
>> }
>>
>> -struct interface_data {
>> - GSupplicantInterface *interface;
>> - char *path; /* Interface path cannot be taken from interface (above) as
>> - * it might have been freed already.
>> - */
>> - GSupplicantInterfaceCallback callback;
>> - void *user_data;
>> -};
>> -
>> -struct interface_create_data {
>> - char *ifname;
>> - char *driver;
>> - char *bridge;
>> - GSupplicantInterface *interface;
>> - GSupplicantInterfaceCallback callback;
>> - void *user_data;
>> -};
>> -
>> -struct interface_connect_data {
>> - GSupplicantInterface *interface;
>> - char *path;
>> - GSupplicantInterfaceCallback callback;
>> - union {
>> - GSupplicantSSID *ssid;
>> - GSupplicantPeerParams *peer;
>> - };
>> - void *user_data;
>> -};
>> -
>> -struct interface_scan_data {
>> - GSupplicantInterface *interface;
>> - char *path;
>> - GSupplicantInterfaceCallback callback;
>> - GSupplicantScanParams *scan_params;
>> - void *user_data;
>> -};
>> -
>> static void interface_create_data_free(struct interface_create_data *data)
>> {
>> g_free(data->ifname);
>> @@ -4105,7 +4112,6 @@ static void interface_add_network_result(const char
>> *error,
>>
>> SUPPLICANT_DBG("PATH: %s", path);
>>
>> - g_free(interface->network_path);
>> interface->network_path = g_strdup(path);
>>
>> supplicant_dbus_method_call(data->interface->path,
>> @@ -4656,7 +4662,8 @@ int
>> g_supplicant_interface_connect(GSupplicantInterface *interface,
>> void *user_data)
>> {
>> struct interface_connect_data *data;
>> - int ret;
>> + struct interface_data *intf_data;
>> + int ret = 0;
>>
>> if (!interface)
>> return -EINVAL;
>> @@ -4685,12 +4692,44 @@ int
>> g_supplicant_interface_connect(GSupplicantInterface *interface,
>> SUPPLICANT_INTERFACE ".Interface.WPS",
>> "ProcessCredentials", DBUS_TYPE_BOOLEAN_AS_STRING,
>> wps_process_credentials, wps_start, data, interface);
>> - } else
>> - ret = supplicant_dbus_method_call(interface->path,
>> - SUPPLICANT_INTERFACE ".Interface", "AddNetwork",
>> - interface_add_network_params,
>> - interface_add_network_result, data,
>> - interface);
>> + } else {
>> + /* By the time there is a request for connect and the network
>> + * path is not NULL it means that connman has not removed the
>> + * previous network pointer. This can happen in the case AP
>> + * deauthenticated client and connman does not remove the
>> + * previously connected network pointer. This causes supplicant
>> + * to reallocate the memory for struct wpa_ssid again even if
>> it
>> + * is the same SSID. This causes memory usage of wpa_supplicnat
>> + * to go high. The idea here is that if the previously
>> connected
>> + * network is not removed at the time of next connection
>> attempt
>> + * check if the network path is not NULL. In case it is
>> non-NULL
>> + * first remove the network and then once removal is
>> successful, add
>> + * the network.
>> + */
>
> Add this comment to the commit message as it explains more in depth what
> is going on.
>
>> + if (interface->network_path != NULL) {
>> + g_free(data->path);
>> + dbus_free(data);
>> +
>> + intf_data = dbus_malloc0(sizeof(*intf_data));
>> + if (!intf_data)
>> + return -ENOMEM;
>> +
>> + intf_data->interface = interface;
>> + intf_data->path = g_strdup(interface->path);
>> + intf_data->callback = callback;
>> + intf_data->ssid = ssid;
>> + intf_data->user_data = user_data;
>> + intf_data->network_remove_in_progress = TRUE;
>> + network_remove(intf_data);
>> + } else {
>> + ret = supplicant_dbus_method_call(interface->path,
>> + SUPPLICANT_INTERFACE ".Interface",
>> "AddNetwork",
>> + interface_add_network_params,
>> + interface_add_network_result, data,
>> + interface);
>> + }
>> + }
>>
>> if (ret < 0) {
>> g_free(data->path);
>> @@ -4705,6 +4744,7 @@ static void network_remove_result(const char *error,
>> DBusMessageIter *iter, void *user_data)
>> {
>> struct interface_data *data = user_data;
>> + struct interface_connect_data * connect_data;
>> int result = 0;
>>
>> SUPPLICANT_DBG("");
>> @@ -4716,11 +4756,31 @@ static void network_remove_result(const char *error,
>> result = -ECONNABORTED;
>> }
>>
>> - g_free(data->path);
>> + g_free(data->interface->network_path);
>> + data->interface->network_path = NULL;
>>
>> - if (data->callback)
>> - data->callback(result, data->interface, data->user_data);
>> + if (data->network_remove_in_progress == TRUE) {
>> + data->network_remove_in_progress = FALSE;
>> + connect_data = dbus_malloc0(sizeof(*connect_data));
>> + if (!connect_data)
>> + return;
>> +
>> + connect_data->interface = data->interface;
>> + connect_data->path = data->path;
>
> As connect_data "steals" the value of data->path here, it's a good idea
> to explicitly set data->path to NULL right after to indicate intention.
> Then...
>
Probably doing this is the right thing:
connect_data->path = g_strdup(data->path);
and the move g_free(data->path) outside of else as it was in the early code.
connect_data->path will then eventually be freed in
interface_select_network_result.
>> + connect_data->callback = data->callback;
>> + connect_data->ssid = data->ssid;
>> + connect_data->user_data = data->user_data;
>>
>> + supplicant_dbus_method_call(data->interface->path,
>> + SUPPLICANT_INTERFACE ".Interface", "AddNetwork",
>> + interface_add_network_params,
>> + interface_add_network_result, connect_data,
>> + connect_data->interface);
>> + } else {
>> + g_free(data->path);
>
> ...this g_free() can be moved down next to dbus_free(), which is a bit
> more logical and in line with the previous code.
>
>> + if (data->callback)
>> + data->callback(result, data->interface,
>> data->user_data);
>> + }
>> dbus_free(data);
>> }
>>
>
> Cheers,
>
> Patrik
>
>
------------------------------
Message: 3
Date: Tue, 23 Feb 2016 09:23:47 +0200
From: Patrik Flykt <[email protected]>
To: Naveen Singh <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH v3] gsupplicant: Mem leak in wpa_s because
"RemoveNetwork" not called
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
On Mon, 2016-02-22 at 19:44 -0800, Naveen Singh wrote:
> >> - g_free(data->path);
> >> + g_free(data->interface->network_path);
> >> + data->interface->network_path = NULL;
> >>
> >> - if (data->callback)
> >> - data->callback(result, data->interface, data->user_data);
> >> + if (data->network_remove_in_progress == TRUE) {
> >> + data->network_remove_in_progress = FALSE;
> >> + connect_data = dbus_malloc0(sizeof(*connect_data));
> >> + if (!connect_data)
> >> + return;
> >> +
> >> + connect_data->interface = data->interface;
> >> + connect_data->path = data->path;
> >
> > As connect_data "steals" the value of data->path here, it's a good idea
> > to explicitly set data->path to NULL right after to indicate intention.
> > Then...
> >
> Probably doing this is the right thing:
> connect_data->path = g_strdup(data->path);
> and the move g_free(data->path) outside of else as it was in the early code.
> connect_data->path will then eventually be freed in
> interface_select_network_result.
That is the failsafe way of implementing it where it does not matter if
the code is eventually moved around.
Cheers,
Patrik
------------------------------
Message: 4
Date: Tue, 23 Feb 2016 09:37:39 +0000
From: "Zheng, Wu" <[email protected]>
To: Daniel Wagner <[email protected]>, "Tomasz Bursztyka
([email protected])" <[email protected]>
Cc: "[email protected]" <[email protected]>
Subject: RE: nftables
Message-ID:
<2cf57a644018a745b8fe029c7223e16e12f4c...@shsmsx104.ccr.corp.intel.com>
Content-Type: text/plain; charset="us-ascii"
Hi Daniel Wagner,
>Yes, it makes sense to abandon iptables.
At current, if nftable need to be implemented in Connman, should connman
support both iptables and nftables, right?
If system only support iptables, connman need to support it too. Right?
Best Regards
Zheng Wu
-----Original Message-----
From: Daniel Wagner [mailto:[email protected]]
Sent: Monday, February 22, 2016 5:38 PM
To: Zheng, Wu <[email protected]>; Tomasz Bursztyka
<[email protected]>
Cc: [email protected]
Subject: Re: nftables
Hi,
On 02/22/2016 10:30 AM, Zheng, Wu wrote:
> Hi Tomasz Bursztyka and Daniel Wagner,
>
> We want to use nftable in our system.
Yes, it makes sense to abandon iptables.
> Do you have an plan to implement nftable in Connman?
Well, as I said i was playing with the idea to do it as spare time project.
> If so, do you have an plan when do upstream can implement the feature?
I was asking Tomasz on how to use nftables. nftables doesn't enforce any
policies on the chain/table structure such as iptables. So that is first thing
we have to figure out.
Probably, we should also discuss it with the systemd-netwokd guys.
cheers,
daniel
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 4, Issue 29
**************************************
