[cip-dev] [PATCH 4.4 0/5] DHCP client support when receiving "delayed" replies

[cip-dev] [PATCH 4.4 0/5] DHCP client support when receiving "delayed" replies

[Patryk Mungai]

Dear All,

When running dhcp tests using the 4.4.y (and 4.4.y-cip kernel as well), I
encountered an issue where the dhcp client in the kernel could not get an
IP address when multiple network devices were enabled. It seems that the
current implementation of the dhcp client in the 4.4 kernel is send dhcp
request via device 1 -> wait <1s for response from server on device 1 ->
if no response, switch to device 2 -> repeat process on device 2 ...etc.
When the dhcp server is slow to respond, this means it is impossible to get
a dhcp address.

This series backported from upstream fixes the issue, is it possible
to apply this to 4.4.y and/or 4.4.y-cip?

Thanks,
Patryk


Geert Uytterhoeven (1):
  net: ipconfig: Fix NULL pointer dereference on RARP/BOOTP/DHCP timeout

Thierry Reding (1):
  net: ipconfig: Fix more use after free

Uwe Kleine-K?nig (3):
  net: ipconfig: Support using "delayed" DHCP replies
  net: ipconfig: drop inter-device timeout
  net: ipconfig: fix use after free

 net/ipv4/ipconfig.c | 61 ++++++++++++++++++++++++-----------------------------
 1 file changed, 28 insertions(+), 33 deletions(-)

-- 
2.7.4

[cip-dev] [PATCH 4.4 1/5] net: ipconfig: Support using "delayed" DHCP replies

[Patryk Mungai]

From: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>

commit 2647cffb2bc6fbed163d377390eb7ca552c7c1cb upstream.

The dhcp code only waits 1s between sending DHCP requests on different
devices and only accepts an answer for the device that sent out the last
request. Only the timeout at the end of a loop is increased iteratively
which favours only the last device. This makes it impossible to work
with a dhcp server that takes little more than 1s connected to a device
that is not the last one.

Instead of also increasing the inter-device timeout, teach the code to
handle delayed replies.

To accomplish that, make *ic_dev track the current ic_device instead of
the current net_device and adapt all users accordingly. The relevant
change then is to reset d to ic_dev on a reply to assert that the
followup request goes through the right device.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Signed-off-by: David S. Miller <davem@davemloft.net>
[Patryk: Backported to 4.4: replaced DBG function with pr_debug]
Signed-off-by: Patryk Mungai <patryk.mungai-ndungu.kx@renesas.com>
---
 net/ipv4/ipconfig.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
index 60f564d..f326433 100644
--- a/net/ipv4/ipconfig.c
+++ b/net/ipv4/ipconfig.c
@@ -195,7 +195,7 @@ struct ic_device {
 };
 
 static struct ic_device *ic_first_dev __initdata;	/* List of open device */
-static struct net_device *ic_dev __initdata;		/* Selected device */
+static struct ic_device *ic_dev __initdata;		/* Selected device */
 
 static bool __init ic_is_init_dev(struct net_device *dev)
 {
@@ -314,8 +314,8 @@ static void __init ic_close_devs(void)
 	while ((d = next)) {
 		next = d->next;
 		dev = d->dev;
-		if (dev != ic_dev && !netdev_uses_dsa(dev)) {
-			DBG(("IP-Config: Downing %s\n", dev->name));
+		if (dev != ic_dev->dev && !netdev_uses_dsa(dev)) {
+			pr_debug("IP-Config: Downing %s\n", dev->name);
 			dev_change_flags(dev, d->flags);
 		}
 		kfree(d);
@@ -379,7 +379,7 @@ static int __init ic_setup_if(void)
 	int err;
 
 	memset(&ir, 0, sizeof(ir));
-	strcpy(ir.ifr_ifrn.ifrn_name, ic_dev->name);
+	strcpy(ir.ifr_ifrn.ifrn_name, ic_dev->dev->name);
 	set_sockaddr(sin, ic_myaddr, 0);
 	if ((err = ic_devinet_ioctl(SIOCSIFADDR, &ir)) < 0) {
 		pr_err("IP-Config: Unable to set interface address (%d)\n",
@@ -403,7 +403,7 @@ static int __init ic_setup_if(void)
 	 * out, we'll try to muddle along.
 	 */
 	if (ic_dev_mtu != 0) {
-		strcpy(ir.ifr_name, ic_dev->name);
+		strcpy(ir.ifr_name, ic_dev->dev->name);
 		ir.ifr_mtu = ic_dev_mtu;
 		if ((err = ic_dev_ioctl(SIOCSIFMTU, &ir)) < 0)
 			pr_err("IP-Config: Unable to set interface mtu to %d (%d)\n",
@@ -574,7 +574,7 @@ ic_rarp_recv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt
 		goto drop_unlock;
 
 	/* We have a winner! */
-	ic_dev = dev;
+	ic_dev = d;
 	if (ic_myaddr == NONE)
 		ic_myaddr = tip;
 	ic_servaddr = sip;
@@ -661,8 +661,6 @@ static struct packet_type bootp_packet_type __initdata = {
 	.func =	ic_bootp_recv,
 };
 
-static __be32 ic_dev_xid;		/* Device under configuration */
-
 /*
  *  Initialize DHCP/BOOTP extension fields in the request.
  */
@@ -1052,12 +1050,6 @@ static int __init ic_bootp_recv(struct sk_buff *skb, struct net_device *dev, str
 		goto drop_unlock;
 	}
 
-	/* Is it a reply for the device we are configuring? */
-	if (b->xid != ic_dev_xid) {
-		net_err_ratelimited("DHCP/BOOTP: Ignoring delayed packet\n");
-		goto drop_unlock;
-	}
-
 	/* Parse extensions */
 	if (ext_len >= 4 &&
 	    !memcmp(b->exten, ic_bootp_cookie, 4)) { /* Check magic cookie */
@@ -1148,7 +1140,7 @@ static int __init ic_bootp_recv(struct sk_buff *skb, struct net_device *dev, str
 	}
 
 	/* We have a winner! */
-	ic_dev = dev;
+	ic_dev = d;
 	ic_myaddr = b->your_ip;
 	ic_servaddr = b->server_ip;
 	ic_addrservaddr = b->iph.saddr;
@@ -1243,9 +1235,6 @@ static int __init ic_dynamic(void)
 	timeout = CONF_BASE_TIMEOUT + (timeout % (unsigned int) CONF_TIMEOUT_RANDOM);
 	for (;;) {
 #ifdef IPCONFIG_BOOTP
-		/* Track the device we are configuring */
-		ic_dev_xid = d->xid;
-
 		if (do_bootp && (d->able & IC_BOOTP))
 			ic_bootp_send_if(d, jiffies - start_jiffies);
 #endif
@@ -1263,6 +1252,8 @@ static int __init ic_dynamic(void)
 		    (ic_proto_enabled & IC_USE_DHCP) &&
 		    ic_dhcp_msgtype != DHCPACK) {
 			ic_got_reply = 0;
+			/* continue on device that got the reply */
+			d = ic_dev;
 			pr_cont(",");
 			continue;
 		}
@@ -1513,7 +1504,7 @@ static int __init ip_auto_config(void)
 #endif /* IPCONFIG_DYNAMIC */
 	} else {
 		/* Device selected manually or only one device -> use it */
-		ic_dev = ic_first_dev->dev;
+		ic_dev = ic_first_dev;
 	}
 
 	addr = root_nfs_parse_addr(root_server_path);
@@ -1548,7 +1539,7 @@ static int __init ip_auto_config(void)
 	pr_info("IP-Config: Complete:\n");
 
 	pr_info("     device=%s, hwaddr=%*phC, ipaddr=%pI4, mask=%pI4, gw=%pI4\n",
-		ic_dev->name, ic_dev->addr_len, ic_dev->dev_addr,
+		ic_dev->dev->name, ic_dev->dev->addr_len, ic_dev->dev->dev_addr,
 		&ic_myaddr, &ic_netmask, &ic_gateway);
 	pr_info("     host=%s, domain=%s, nis-domain=%s\n",
 		utsname()->nodename, ic_domain, utsname()->domainname);
-- 
2.7.4

[cip-dev] [PATCH 4.4 2/5] net: ipconfig: drop inter-device timeout

[Patryk Mungai]

From: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>

commit e068853409aa17208e94f4ca628005cc6f51e043 upstream.

Now that ipconfig learned to handle "delayed replies" in the previous
commit, there is no reason any more to delay sending a first request per
device.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Signed-off-by: David S. Miller <davem@davemloft.net>
[Patryk: cherry-picked to 4.4]
Signed-off-by: Patryk Mungai <patryk.mungai-ndungu.kx@renesas.com>
---
 net/ipv4/ipconfig.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
index f326433..35c8b35 100644
--- a/net/ipv4/ipconfig.c
+++ b/net/ipv4/ipconfig.c
@@ -94,7 +94,6 @@
 /* Define the timeout for waiting for a DHCP/BOOTP/RARP reply */
 #define CONF_OPEN_RETRIES 	2	/* (Re)open devices twice */
 #define CONF_SEND_RETRIES 	6	/* Send six requests per open */
-#define CONF_INTER_TIMEOUT	(HZ)	/* Inter-device timeout: 1 second */
 #define CONF_BASE_TIMEOUT	(HZ*2)	/* Initial timeout: 2 seconds */
 #define CONF_TIMEOUT_RANDOM	(HZ)	/* Maximum amount of randomization */
 #define CONF_TIMEOUT_MULT	*7/4	/* Rate of timeout growth */
@@ -1243,9 +1242,11 @@ static int __init ic_dynamic(void)
 			ic_rarp_send_if(d);
 #endif
 
-		jiff = jiffies + (d->next ? CONF_INTER_TIMEOUT : timeout);
-		while (time_before(jiffies, jiff) && !ic_got_reply)
-			schedule_timeout_uninterruptible(1);
+		if (!d->next) {
+			jiff = jiffies + timeout;
+			while (time_before(jiffies, jiff) && !ic_got_reply)
+				schedule_timeout_uninterruptible(1);
+		}
 #ifdef IPCONFIG_DHCP
 		/* DHCP isn't done until we get a DHCPACK. */
 		if ((ic_got_reply & IC_BOOTP) &&
-- 
2.7.4

[cip-dev] [PATCH 4.4 3/5] net: ipconfig: fix use after free

[Patryk Mungai]

From: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>

commit 9c706a49d660653625d206f6972541c1f60ea2b0 upstream.

ic_close_devs() calls kfree() for all devices's ic_device. Since commit
2647cffb2bc6 ("net: ipconfig: Support using "delayed" DHCP replies")
the active device's ic_device is still used however to print the
ipconfig summary which results in an oops if the memory is already
changed. So delay freeing until after the autoconfig results are
reported.

Fixes: 2647cffb2bc6 ("net: ipconfig: Support using "delayed" DHCP replies")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: David S. Miller <davem@davemloft.net>
[Patryk: cherry-picked to 4.4]
Signed-off-by: Patryk Mungai <patryk.mungai-ndungu.kx@renesas.com>
---
 net/ipv4/ipconfig.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
index 35c8b35..23c9b8a 100644
--- a/net/ipv4/ipconfig.c
+++ b/net/ipv4/ipconfig.c
@@ -1519,14 +1519,6 @@ static int __init ip_auto_config(void)
 		return -1;
 
 	/*
-	 * Close all network devices except the device we've
-	 * autoconfigured and set up routes.
-	 */
-	ic_close_devs();
-	if (ic_setup_if() < 0 || ic_setup_routes() < 0)
-		return -1;
-
-	/*
 	 * Record which protocol was actually used.
 	 */
 #ifdef IPCONFIG_DYNAMIC
@@ -1560,6 +1552,15 @@ static int __init ip_auto_config(void)
 	pr_cont("\n");
 #endif /* !SILENT */
 
+	/*
+	 * Close all network devices except the device we've
+	 * autoconfigured and set up routes.
+	 */
+	ic_close_devs();
+	if (ic_setup_if() < 0 || ic_setup_routes() < 0)
+		return -1;
+
+
 	return 0;
 }
 
-- 
2.7.4

[cip-dev] [PATCH 4.4 4/5] net: ipconfig: Fix more use after free

[Patryk Mungai]

From: Thierry Reding <treding@nvidia.com>

commit d2d371ae5dd6af9a6a3d7f50b753627c42868409 upstream.

While commit 9c706a49d660 ("net: ipconfig: fix use after free") avoids
the use after free, the resulting code still ends up calling both the
ic_setup_if() and ic_setup_routes() after calling ic_close_devs(), and
access to the device is still required.

Move the call to ic_close_devs() to the very end of the function.

Signed-off-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
[Patryk: cherry-picked to 4.4]
Signed-off-by: Patryk Mungai <patryk.mungai-ndungu.kx@renesas.com>
---
 net/ipv4/ipconfig.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
index 23c9b8a..6b78590 100644
--- a/net/ipv4/ipconfig.c
+++ b/net/ipv4/ipconfig.c
@@ -1556,12 +1556,14 @@ static int __init ip_auto_config(void)
 	 * Close all network devices except the device we've
 	 * autoconfigured and set up routes.
 	 */
-	ic_close_devs();
 	if (ic_setup_if() < 0 || ic_setup_routes() < 0)
-		return -1;
+		err = -1;
+	else
+		err = 0;
 
+	ic_close_devs();
 
-	return 0;
+	return err;
 }
 
 late_initcall(ip_auto_config);
-- 
2.7.4

[cip-dev] [PATCH 4.4 5/5] net: ipconfig: Fix NULL pointer dereference on RARP/BOOTP/DHCP timeout

[Patryk Mungai]

From: Geert Uytterhoeven <geert+renesas@glider.be>

commit 1ae292a2457cd692828da2be87cb967260993ad0 upstream.

If no RARP, BOOTP, or DHCP response is received, ic_dev is never set,
causing a NULL pointer dereference in ic_close_devs():

    Sending DHCP requests ...... timed out!
    Unable to handle kernel NULL pointer dereference at virtual address 00000004

To fix this, add a check to avoid dereferencing ic_dev if it is still
NULL.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Fixes: 2647cffb2bc6fbed ("net: ipconfig: Support using "delayed" DHCP replies")
Signed-off-by: David S. Miller <davem@davemloft.net>
[Patryk: cherry-picked to 4.4]
Signed-off-by: Patryk Mungai <patryk.mungai-ndungu.kx@renesas.com>
---
 net/ipv4/ipconfig.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
index 6b78590..ca658af 100644
--- a/net/ipv4/ipconfig.c
+++ b/net/ipv4/ipconfig.c
@@ -313,7 +313,7 @@ static void __init ic_close_devs(void)
 	while ((d = next)) {
 		next = d->next;
 		dev = d->dev;
-		if (dev != ic_dev->dev && !netdev_uses_dsa(dev)) {
+		if ((!ic_dev || dev != ic_dev->dev) && !netdev_uses_dsa(dev)) {
 			pr_debug("IP-Config: Downing %s\n", dev->name);
 			dev_change_flags(dev, d->flags);
 		}
-- 
2.7.4

[cip-dev] [PATCH 4.4 0/5] DHCP client support when receiving "delayed" replies

[Pavel Machek]

Hi!

> When running dhcp tests using the 4.4.y (and 4.4.y-cip kernel as well), I
> encountered an issue where the dhcp client in the kernel could not get an
> IP address when multiple network devices were enabled. It seems that the
> current implementation of the dhcp client in the 4.4 kernel is send dhcp
> request via device 1 -> wait <1s for response from server on device 1 ->
> if no response, switch to device 2 -> repeat process on device 2 ...etc.
> When the dhcp server is slow to respond, this means it is impossible to get
> a dhcp address.
> 
> This series backported from upstream fixes the issue, is it possible
> to apply this to 4.4.y and/or 4.4.y-cip?

Ok, so first patch adds support for using "delayed" DHCP replies, then
there are three more patches to fix up issues it creates.

Which tells me that maybe this is not quite suitable for -stable.

How long do your dhcp servers take to reply? Can you solve the problem
some other way, like for example increasing timeouts?

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.cip-project.org/pipermail/cip-dev/attachments/20190406/635ad643/attachment.sig>

[cip-dev] [PATCH 4.4 0/5] DHCP client support when receiving "delayed" replies

[Sasha Levin]

On Fri, Apr 05, 2019 at 11:47:31AM +0100, Patryk Mungai wrote:
>Dear All,
>
>When running dhcp tests using the 4.4.y (and 4.4.y-cip kernel as well), I
>encountered an issue where the dhcp client in the kernel could not get an
>IP address when multiple network devices were enabled. It seems that the
>current implementation of the dhcp client in the 4.4 kernel is send dhcp
>request via device 1 -> wait <1s for response from server on device 1 ->
>if no response, switch to device 2 -> repeat process on device 2 ...etc.
>When the dhcp server is slow to respond, this means it is impossible to get
>a dhcp address.
>
>This series backported from upstream fixes the issue, is it possible
>to apply this to 4.4.y and/or 4.4.y-cip?

This is something that seems more suited for the CIP kernel as it adds
new functionality.

--
Thanks,
Sasha

[cip-dev] [PATCH 4.4 0/5] DHCP client support when receiving "delayed" replies

[Patryk Mungai Ndungu]

Hello Pavel,

> -----Original Message-----
> From: stable-owner at vger.kernel.org <stable-owner@vger.kernel.org> On
> Behalf Of Pavel Machek
> Sent: 06 April 2019 11:39
> To: Patryk Mungai Ndungu <patryk.mungai-ndungu.kx@renesas.com>
> Cc: stable at vger.kernel.org; davem at davemloft.net; cip-dev at lists.cip-
> project.org
> Subject: Re: [cip-dev] [PATCH 4.4 0/5] DHCP client support when receiving
> "delayed" replies
> 
> Hi!
> 
> > When running dhcp tests using the 4.4.y (and 4.4.y-cip kernel as well), I
> > encountered an issue where the dhcp client in the kernel could not get an
> > IP address when multiple network devices were enabled. It seems that the
> > current implementation of the dhcp client in the 4.4 kernel is send dhcp
> > request via device 1 -> wait <1s for response from server on device 1 ->
> > if no response, switch to device 2 -> repeat process on device 2 ...etc.
> > When the dhcp server is slow to respond, this means it is impossible to get
> > a dhcp address.
> >
> > This series backported from upstream fixes the issue, is it possible
> > to apply this to 4.4.y and/or 4.4.y-cip?
> 
> Ok, so first patch adds support for using "delayed" DHCP replies, then
> there are three more patches to fix up issues it creates.
> 
> Which tells me that maybe this is not quite suitable for -stable.
> 
> How long do your dhcp servers take to reply? 

It varies: the fastest reply I've seen is 0.12931s, the slowest is in the region is just over 1.007s.
In the tests I've run, I measured the time between the kernel sending out the DHCP request and receiving a DHCP offer from the server. After running 50 tests, in around half of them, it takes just over 1s for the DHCP offer to arrive. But this is rarely over 1s +1 jiffie, hence why I think the code is able to cope with it most of the time.

However, the DHCP servers network is not at all loaded (at most only has 4 devices trying to connect to it at once), and yet I've seen this failure multiple times, so I'm not sure what would happen in a loaded network.
I think at least for CIP we need a kernel that is able to cope with however long the server takes to reply.

> Can you solve the problem
> some other way, like for example increasing timeouts?

I've tried increasing CONF_INTER_TIMEOUT to 2Hz and I haven't seen it fail in 50 boots. Though this is an simple workaround, it can prolong boot up time and the DHCP client is still time dependent with regards to listening for a reply on a network device.

Thanks,
Patryk

> 
> Thanks,
> 									Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures)
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[cip-dev] [PATCH 4.4 0/5] DHCP client support when receiving "delayed" replies

[Greg KH]

On Sat, Apr 06, 2019 at 09:53:50AM -0400, Sasha Levin wrote:
> On Fri, Apr 05, 2019 at 11:47:31AM +0100, Patryk Mungai wrote:
> > Dear All,
> > 
> > When running dhcp tests using the 4.4.y (and 4.4.y-cip kernel as well), I
> > encountered an issue where the dhcp client in the kernel could not get an
> > IP address when multiple network devices were enabled. It seems that the
> > current implementation of the dhcp client in the 4.4 kernel is send dhcp
> > request via device 1 -> wait <1s for response from server on device 1 ->
> > if no response, switch to device 2 -> repeat process on device 2 ...etc.
> > When the dhcp server is slow to respond, this means it is impossible to get
> > a dhcp address.
> > 
> > This series backported from upstream fixes the issue, is it possible
> > to apply this to 4.4.y and/or 4.4.y-cip?
> 
> This is something that seems more suited for the CIP kernel as it adds
> new functionality.

I agree, this is a new feature.  If someone wants this, they should use
a newer kernel please.  Like 4.19.y :)

thanks,

greg k-h