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. [PATCH v2 0/4] session: fix missing snat and firewall updates
(Lukasz Nowak)
2. [PATCH v2 1/4] session: fix snat firewall not cleaning up
correctly (Lukasz Nowak)
3. [PATCH v2 2/4] session: update firewall and snat on ipconfig
change (Lukasz Nowak)
4. [PATCH v2 3/4] session: do not set up SNAT rules without ip
address (Lukasz Nowak)
5. [PATCH v2 4/4] gdhcp: prevent double timeout remove (Lukasz Nowak)
6. [PATCH] nat: Set file offset back to 0 before writing
ip_forward (Daniel Wagner)
7. Re: ip forward bug after kernel 4.5-rc1 with connman 1.32+
(Daniel Wagner)
8. Re: [PATCH v2 1/3] Fix test-mozjs memory leak (Daniel Wagner)
----------------------------------------------------------------------
Message: 1
Date: Mon, 24 Apr 2017 17:05:46 +0100
From: Lukasz Nowak <[email protected]>
To: [email protected]
Subject: [PATCH v2 0/4] session: fix missing snat and firewall updates
Message-ID: <[email protected]>
From: Lukasz Nowak <[email protected]>
Changes since v1:
- two additional patches fixing problems observed after dhcp lease expiration
Two scenarios have been found where session would not update all tables
it manages:
- snat not removed after a disconnect
- snat and firewall mark not updated after ip config change
Lukasz Nowak (4):
session: fix snat firewall not cleaning up correctly
session: update firewall and snat on ipconfig change
session: do not set up SNAT rules without ip address
gdhcp: prevent double timeout remove
gdhcp/client.c | 4 +++-
src/session.c | 29 +++++++++++++++++------------
2 files changed, 20 insertions(+), 13 deletions(-)
--
2.7.4
------------------------------
Message: 2
Date: Mon, 24 Apr 2017 17:05:47 +0100
From: Lukasz Nowak <[email protected]>
To: [email protected]
Subject: [PATCH v2 1/4] session: fix snat firewall not cleaning up
correctly
Message-ID: <[email protected]>
From: Lukasz Nowak <[email protected]>
Session snat firewall rules are not being deleted correctly on
a session disconnect. As a result, after many disconnects,
iptables fills up with old snat rules. This can happen, e.g.
when a cellular modem disconnects frequently, and is assigned
a different ip address every time.
This happens because del_nat_rules() depends on
having an ipconfig index stored in the session structure. But
that index is getting cleared by update_routing_table().
Fix this by correcting clean-up call order. Instead of this:
update_routing_table(session);
del_nat_rules(session);
add_nat_rules(session);
do this:
del_nat_rules(session);
update_routing_table(session);
add_nat_rules(session);
---
src/session.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/src/session.c b/src/session.c
index dadd68f..bb47a13 100644
--- a/src/session.c
+++ b/src/session.c
@@ -466,12 +466,6 @@ static void cleanup_nat_rules(struct connman_session
*session)
del_nat_rules(session);
}
-static void update_nat_rules(struct connman_session *session)
-{
- del_nat_rules(session);
- add_nat_rules(session);
-}
-
static void destroy_policy_config(struct connman_session *session)
{
if (!policy) {
@@ -1678,8 +1672,9 @@ static void update_session_state(struct connman_session
*session)
DBG("session %p state %s", session, state2string(state));
update_firewall(session);
+ del_nat_rules(session);
update_routing_table(session);
- update_nat_rules(session);
+ add_nat_rules(session);
session_notify(session);
}
--
2.7.4
------------------------------
Message: 3
Date: Mon, 24 Apr 2017 17:05:48 +0100
From: Lukasz Nowak <[email protected]>
To: [email protected]
Subject: [PATCH v2 2/4] session: update firewall and snat on ipconfig
change
Message-ID: <[email protected]>
From: Lukasz Nowak <[email protected]>
When an ipconfig changes (e.g. from link-local to dhcp), session
needs to update all the tables:
- routing
- firewall marking
- snat
and not just the routing table.
---
src/session.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/session.c b/src/session.c
index bb47a13..721e4b9 100644
--- a/src/session.c
+++ b/src/session.c
@@ -1900,7 +1900,7 @@ static void ipconfig_changed(struct connman_service
*service,
continue;
if (session->service && session->service == service) {
- update_routing_table(session);
+ update_session_state(session);
if (type == CONNMAN_IPCONFIG_TYPE_IPV4)
ipconfig_ipv4_changed(session);
--
2.7.4
------------------------------
Message: 4
Date: Mon, 24 Apr 2017 17:05:49 +0100
From: Lukasz Nowak <[email protected]>
To: [email protected]
Subject: [PATCH v2 3/4] session: do not set up SNAT rules without ip
address
Message-ID: <[email protected]>
From: Lukasz Nowak <[email protected]>
If a dhcp lease is lost (expires and dhcp server does not respond)
ipconfig object for an interface still exists, but addr is null.
When that happens, add_nat_rules() passes a null pointer to iptables
which asserts and exits the ConnMan process.
Prevent that, by not enabling snat if there is no ip address.
---
src/session.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/session.c b/src/session.c
index 721e4b9..1f80b14 100644
--- a/src/session.c
+++ b/src/session.c
@@ -73,6 +73,7 @@ struct connman_session {
int index;
char *gateway;
bool policy_routing;
+ bool snat_enabled;
};
struct connman_service_info {
@@ -394,6 +395,10 @@ static void del_nat_rules(struct connman_session *session)
{
struct fw_snat *fw_snat;
+ if (!session->snat_enabled)
+ return;
+
+ session->snat_enabled = false;
fw_snat = fw_snat_lookup(session->index);
if (!fw_snat)
@@ -415,19 +420,24 @@ static void add_nat_rules(struct connman_session *session)
ipconfig = __connman_service_get_ip4config(session->service);
index = __connman_ipconfig_get_index(ipconfig);
+ ifname = connman_inet_ifname(index);
+ addr = __connman_ipconfig_get_local(ipconfig);
+
+ if (!addr)
+ return;
+ session->snat_enabled = true;
fw_snat = fw_snat_lookup(index);
if (fw_snat) {
fw_snat_ref(session, fw_snat);
return;
}
- ifname = connman_inet_ifname(index);
- addr = __connman_ipconfig_get_local(ipconfig);
-
err = fw_snat_create(session, index, ifname, addr);
- if (err < 0)
+ if (err < 0) {
DBG("failed to add SNAT rule");
+ session->snat_enabled = false;
+ }
g_free(ifname);
}
--
2.7.4
------------------------------
Message: 5
Date: Mon, 24 Apr 2017 17:05:50 +0100
From: Lukasz Nowak <[email protected]>
To: [email protected]
Subject: [PATCH v2 4/4] gdhcp: prevent double timeout remove
Message-ID: <[email protected]>
From: Lukasz Nowak <[email protected]>
In some circumstances (observed with dhcp lease expiration),
t1_timeout source may be removed twice.
No ill effects have been observed, but it produces a bad line in the log:
GLib-CRITICAL **: Source ID X was not found when attempting to remove it
If the timeout source id got re-used between the calls, this could cause
incorrect behaviour.
Fix that by clearing the timeout index after the first remove call.
---
gdhcp/client.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/gdhcp/client.c b/gdhcp/client.c
index 9deb52e..3e67fcd 100644
--- a/gdhcp/client.c
+++ b/gdhcp/client.c
@@ -1676,8 +1676,10 @@ static gboolean continue_rebound(gpointer user_data)
switch_listening_mode(dhcp_client, L2);
send_request(dhcp_client);
- if (dhcp_client->t2_timeout> 0)
+ if (dhcp_client->t2_timeout> 0) {
g_source_remove(dhcp_client->t2_timeout);
+ dhcp_client->t2_timeout = 0;
+ }
/*recalculate remaining rebind time*/
dhcp_client->T2 >>= 1;
--
2.7.4
------------------------------
Message: 6
Date: Mon, 24 Apr 2017 20:18:26 +0200
From: Daniel Wagner <[email protected]>
To: [email protected]
Cc: [email protected], Daniel Wagner <[email protected]>
Subject: [PATCH] nat: Set file offset back to 0 before writing
ip_forward
Message-ID: <[email protected]>
Kernel versions >= 4.5 have changed the default behavior[1] for file
offset handling on crop/says fs to strict write position handling:
Respect file position when writing sysctl strings. Multiple writes
will append to the sysctl value buffer. Anything past the max length
of the sysctl value buffer will be ignored. Writes to numeric sysctl
entries must always be at file position 0 and the value must be
fully contained in the buffer sent in the write syscall.
We first read from /proc/sys/net/ipv4/ip_forward before writing to the
file. Without the lseek to 0 first the write is silentenly ignored.
Bug report from Neil MacLeod <[email protected]>.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=41662f5cc55335807d39404371cfcbb1909304c4
---
Hi,
I have 'written' this version of the fix. Mostly pimped the commit
message and added error handling to Neil's version.
@Neil: I hope that is okay with you.
cheers,
Daniel
src/nat.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/nat.c b/src/nat.c
index 4c1f8583d2f1..fb557101ea31 100644
--- a/src/nat.c
+++ b/src/nat.c
@@ -54,6 +54,9 @@ static int enable_ip_forward(bool enable)
if (!value) {
if (read(f, &value, sizeof(value)) < 0)
value = 0;
+
+ if (lseek(f, 0, SEEK_SET) < 0)
+ return -errno;
}
if (enable) {
--
2.9.3
------------------------------
Message: 7
Date: Mon, 24 Apr 2017 20:20:49 +0200
From: Daniel Wagner <[email protected]>
To: Neil MacLeod <[email protected]>, [email protected]
Subject: Re: ip forward bug after kernel 4.5-rc1 with connman 1.32+
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
Hi Neil,
On 04/23/2017 03:33 PM, Neil MacLeod wrote:
> Hi
>
> There's a bug in connman 1.32+ caused by a kernel 4.5-rc1 change that means
> ip forwarding cannot be enabled (or if already enabled, then it cannot be
> disabled).
>
> The kernel change is:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=41662f5cc55335807d39404371cfcbb1909304c4
>
> When enabling IP Forwarding, connman 1.32 opens
> /proc/sys/net/ipv4/ip_forward, reads it then writes to it.
>
> Since 4.5-rc1 this behaviour no longer works as the write() silently fails
> preventing ipforward from being enabled/disabled.
>
> The fix is to add an lseek(), as shown here:
> https://github.com/LibreELEC/LibreELEC.tv/pull/1552 and this works with
> kernels regardless of whether they include the above commit.
>
> I haven't checked the rest of the connman codebase to find any other
> open/read/write examples but it would be wise to do so, just in case. You may
> want to add additional error checks around the lseek() as per your personal
> style, I didn't think it was necessary and just needed a good-enough fix for
> now.
>
> To demonstrate the issue, try the following test program on a pre-4.5.y
> kernel, and then again on a 4.5.y or later kernel: http://sprunge.us/OHfj -
> once the lseek() is added the program will produce correct results on all
> kernels.
Excellent bug tracking! Thanks a lot. I have created a patch and just
send out. Added commit message for it and listed you as reported. If you
want the author ship for it, I'll change it :)
Thanks,
Daniel
------------------------------
Message: 8
Date: Mon, 24 Apr 2017 20:35:49 +0200
From: Daniel Wagner <[email protected]>
To: Jeremy Linton <[email protected]>, [email protected]
Cc: [email protected]
Subject: Re: [PATCH v2 1/3] Fix test-mozjs memory leak
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
Hi Jeremy,
Could you prefix this patch with 'unit'?
On 04/19/2017 07:24 PM, Jeremy Linton wrote:
> The mozjs execute calls returns a g_strdup'ed result. This
> means that the unit test needs to free the resulting data.
>
> Signed-off-by: Jeremy Linton <[email protected]>
We don't do SoB in this project.
Patch looks good.
Thanks,
Daniel
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 18, Issue 19
***************************************
