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: [RFC] vpn: Restrict connman-vpnd capabilities (Patrik Flykt)
2. Re: Disabling the network on Disconnect Notification
(Patrik Flykt)
3. Re: nftables (Tomasz Bursztyka)
4. Re: [PATCH v3] gsupplicant: Mem leak in wpa_s because
"RemoveNetwork" not called (Patrik Flykt)
5. RE: nftables (Zheng, Wu)
----------------------------------------------------------------------
Message: 1
Date: Mon, 22 Feb 2016 09:32:58 +0200
From: Patrik Flykt <[email protected]>
To: Andrew Bibb <[email protected]>
Cc: "[email protected]" <[email protected]>
Subject: Re: [RFC] vpn: Restrict connman-vpnd capabilities
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
On Fri, 2016-02-19 at 19:08 -0500, Andrew Bibb wrote:
> You nailed it, permissions on my personal home directory were 700, so
> OpenVPN could never get into ~/.local/share. Change that one to 711
> and Connman with OpenVPN will connect fine (using the default "as
> shipped" /usr/lib/systemd/system/connman-vpn.service file).
Good that it works out of the box! Thanks for taking time figuring this
one out.
Cheers,
Patrik
------------------------------
Message: 2
Date: Mon, 22 Feb 2016 10:18:59 +0200
From: Patrik Flykt <[email protected]>
To: Naveen Singh <[email protected]>
Cc: [email protected]
Subject: Re: Disabling the network on Disconnect Notification
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
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.
> 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...
> >> 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
------------------------------
Message: 3
Date: Mon, 22 Feb 2016 09:54:45 +0100
From: Tomasz Bursztyka <[email protected]>
To: Daniel Wagner <[email protected]>
Cc: "[email protected]" <[email protected]>
Subject: Re: nftables
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Daniel!
Nice to hear you are back at coding :)
Sorry for being late in answering, now that zephyr is out, it already
gets shacked
by political bs vs technical arguments in which I get lost for hours. I
am really not
the right person to fit in (I might be technically right, but I have 0
political influence).
Anyway...
You are right, when you start bare metal kernel with nftables: there is
nothing.
No tables and no chains. It's a good thing because it enables
flexibility, but it
can also be a burden when it comes to integrate with custom table & chains.
I haven't been following the recent (well... the last ~2 years ^^') work
on nftables
but I believe there are still people using the iptables format. The use
the iptables-
nftables compatible tool, and it mimics iptables through nftables.
Verify it, but if
it's the generic way, then it would be as usual in ConnMan.
If not, then you will have to come with a strategy that fits all. And
that's when it
might become tricky. As you noticed, ConnMan will have to avoid conflicts.
I believe it will be a bit easier than with iptables though.
- you pull the current context:
-> if there is nothing, it will be easy ConnMan will just push
whatever it will want.
-> if something is there, integrating will have to play nice.
Basically: finding the
input entry point to jump on a custom ConnMan table (you'll
probably need that
for each IP version), get you rules applied or return if nothing.
Problem start to arise when somebody/something else messes up again with the
context. Like flushing out everything, reinstalling stuff... ConnMan
will have to follow.
It was the same with iptables. It's hopefully much easier now: there are
proper
netlink notification messages for it. Well, let's put this problem apart
for now.
About coding around, it's a bit messy. There is one library, libnftnl.
It's not build
on top of libnl. I am not entirely sure, but I think you can hook your
own netlink
access functions to it. By default it uses libmnl... You'll have to
verify that.
Ask Marcel what he would prefer as well. Afaik, there is still the plan
to move
ConnMan to ell, so keeping it's custom netlink access would make sense then,
I guess.
Cheers,
Tomasz
> Hi Tomasz,
>
> I just chatted with Patrik and told him about my idea to teach ConnMan
> to use nftables instead iptables. There are a few things to figure out
> first though.
>
> If I got that right there is no policy from the kernel on how to
> structure the rule sets. nat/filter tables and input/forward/output
> chains are userland policies. The question is how would ConnMan fit into
> this? As you know, ConnMan tries to avoid conflicts with iptables in tip
> toeing around. Do we have to do the same with nftables?
>
> cheers,
> daniel
>
------------------------------
Message: 4
Date: Mon, 22 Feb 2016 10:58:06 +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"
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...
> + 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: 5
Date: Mon, 22 Feb 2016 09:30:43 +0000
From: "Zheng, Wu" <[email protected]>
To: Tomasz Bursztyka <[email protected]>, "Daniel
Wagner ([email protected])" <[email protected]>
Cc: "[email protected]" <[email protected]>
Subject: RE: nftables
Message-ID:
<2cf57a644018a745b8fe029c7223e16e12f4a...@shsmsx104.ccr.corp.intel.com>
Content-Type: text/plain; charset="us-ascii"
Hi Tomasz Bursztyka and Daniel Wagner,
We want to use nftable in our system.
Do you have an plan to implement nftable in Connman?
If so, do you have an plan when do upstream can implement the feature?
Thanks.
Best Regards
Zheng Wu
-----Original Message-----
From: connman [mailto:[email protected]] On Behalf Of Tomasz
Bursztyka
Sent: Monday, February 22, 2016 4:55 PM
To: Daniel Wagner <[email protected]>
Cc: [email protected]
Subject: Re: nftables
Hi Daniel!
Nice to hear you are back at coding :)
Sorry for being late in answering, now that zephyr is out, it already gets
shacked by political bs vs technical arguments in which I get lost for hours. I
am really not the right person to fit in (I might be technically right, but I
have 0 political influence).
Anyway...
You are right, when you start bare metal kernel with nftables: there is nothing.
No tables and no chains. It's a good thing because it enables flexibility, but
it can also be a burden when it comes to integrate with custom table & chains.
I haven't been following the recent (well... the last ~2 years ^^') work on
nftables but I believe there are still people using the iptables format. The
use the iptables- nftables compatible tool, and it mimics iptables through
nftables.
Verify it, but if
it's the generic way, then it would be as usual in ConnMan.
If not, then you will have to come with a strategy that fits all. And that's
when it might become tricky. As you noticed, ConnMan will have to avoid
conflicts.
I believe it will be a bit easier than with iptables though.
- you pull the current context:
-> if there is nothing, it will be easy ConnMan will just push whatever it
will want.
-> if something is there, integrating will have to play nice.
Basically: finding the
input entry point to jump on a custom ConnMan table (you'll probably need
that
for each IP version), get you rules applied or return if nothing.
Problem start to arise when somebody/something else messes up again with the
context. Like flushing out everything, reinstalling stuff... ConnMan will have
to follow.
It was the same with iptables. It's hopefully much easier now: there are proper
netlink notification messages for it. Well, let's put this problem apart for
now.
About coding around, it's a bit messy. There is one library, libnftnl.
It's not build
on top of libnl. I am not entirely sure, but I think you can hook your own
netlink access functions to it. By default it uses libmnl... You'll have to
verify that.
Ask Marcel what he would prefer as well. Afaik, there is still the plan to move
ConnMan to ell, so keeping it's custom netlink access would make sense then, I
guess.
Cheers,
Tomasz
> Hi Tomasz,
>
> I just chatted with Patrik and told him about my idea to teach ConnMan
> to use nftables instead iptables. There are a few things to figure out
> first though.
>
> If I got that right there is no policy from the kernel on how to
> structure the rule sets. nat/filter tables and input/forward/output
> chains are userland policies. The question is how would ConnMan fit
> into this? As you know, ConnMan tries to avoid conflicts with iptables
> in tip toeing around. Do we have to do the same with nftables?
>
> cheers,
> daniel
>
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 4, Issue 26
**************************************
