[PATCH] net: ipconfig: Allow DNS to be overwritten by DHCPACK

[PATCH] net: ipconfig: Allow DNS to be overwritten by DHCPACK

[Martin Wetterwald]

Some DHCP server implementations only send the important requested DHCP
options in the final BOOTP reply (DHCPACK).
One example is systemd-networkd.
However, RFC2131, in section 4.3.1 states:

> Once the network address and lease have been determined, the server
> constructs a DHCPOFFER message with the offered configuration
> parameters.
> [...]
> The server MUST return to the client:
> [...]
> o Parameters requested by the client, according to the following
>   rules:
>
>      -- IF the server has been explicitly configured with a default
>         value for the parameter, the server MUST include that value
>         in an appropriate option in the 'option' field, ELSE

I've reported the issue here:
https://github.com/systemd/systemd/issues/27471

Linux PNP DHCP client implementation only takes into account the DNS
servers received in the first BOOTP reply (DHCPOFFER).
This usually isn't an issue as servers are required to put the same
values in the DHCPOFFER and DHCPACK.
However, RFC2131, in section 4.3.2 states:

> Any configuration parameters in the DHCPACK message SHOULD NOT
> conflict with those in the earlier DHCPOFFER message to which the
> client is responding.  The client SHOULD use the parameters in the
> DHCPACK message for configuration.

When making Linux PNP DHCP client (cmdline ip=dhcp) interact with
systemd-networkd DHCP server, an interesting "protocol misunderstanding"
happens:
Because DNS servers were only specified in the DHCPACK and not in the
DHCPOFFER, Linux will not catch the correct DNS servers: in the first
BOOTP reply (DHCPOFFER), it sees that there is no DNS, and sets as
fallback the IP of the DHCP server itself. When the second BOOTP reply
comes (DHCPACK), it's already too late: the kernel will not overwrite
the fallback setting it has set previously.

This patch makes the kernel care more about the latest BOOTP reply
received for DNS servers selection. A subsequent BOOTP reply wins (in
the case of DHCP, this makes DHCPACK win over DHCPOFFER).

Signed-off-by: Martin Wetterwald <martin@wetterwald.eu>
---
 net/ipv4/ipconfig.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
index e90bc0aa85c7..c125095453da 100644
--- a/net/ipv4/ipconfig.c
+++ b/net/ipv4/ipconfig.c
@@ -937,9 +937,11 @@ static void __init ic_do_bootp_ext(u8 *ext)
         servers= *ext/4;
         if (servers > CONF_NAMESERVERS_MAX)
             servers = CONF_NAMESERVERS_MAX;
-        for (i = 0; i < servers; i++) {
-            if (ic_nameservers[i] == NONE)
+        for (i = 0; i < CONF_NAMESERVERS_MAX; i++) {
+            if (i < servers)
                 memcpy(&ic_nameservers[i], ext+1+4*i, 4);
+            else
+                ic_nameservers[i] = NONE;
         }
         break;
     case 12:    /* Host name */
-- 
2.40.1

Re: [PATCH] net: ipconfig: Allow DNS to be overwritten by DHCPACK

[Martin Wetterwald]

On Wed, May 3, 2023 at 12:06 PM Martin Wetterwald <martin@wetterwald.eu> wrote:
> diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
> index e90bc0aa85c7..c125095453da 100644
> --- a/net/ipv4/ipconfig.c
> +++ b/net/ipv4/ipconfig.c
> @@ -937,9 +937,11 @@ static void __init ic_do_bootp_ext(u8 *ext)
>          servers= *ext/4;
>          if (servers > CONF_NAMESERVERS_MAX)
>              servers = CONF_NAMESERVERS_MAX;
> -        for (i = 0; i < servers; i++) {
> -            if (ic_nameservers[i] == NONE)
> +        for (i = 0; i < CONF_NAMESERVERS_MAX; i++) {
> +            if (i < servers)
>                  memcpy(&ic_nameservers[i], ext+1+4*i, 4);
> +            else
> +                ic_nameservers[i] = NONE;
>          }
>          break;
>      case 12:    /* Host name */
> --

I reset the whole DNS array every time because, in case we have a DHCPOFFER
containing more DNS servers than the DHCPACK, we probably don't want to have a
"mix" between DNS servers from the DHCPOFFER and from the DHCPACK at the same
time.
But this could break users having a DHCP server sending only DNS information in
the DHCPOFFER and nothing in the DHCPACK.

Do such kind of DHCP server implementation exist?
Do you see a better way than resetting the whole array every time?

Re: [PATCH] net: ipconfig: Allow DNS to be overwritten by DHCPACK

[Jakub Kicinski]

On Wed, 3 May 2023 12:06:53 +0200 Martin Wetterwald wrote:
> Because DNS servers were only specified in the DHCPACK and not in the
> DHCPOFFER, Linux will not catch the correct DNS servers: in the first
> BOOTP reply (DHCPOFFER), it sees that there is no DNS, and sets as
> fallback the IP of the DHCP server itself. When the second BOOTP reply
> comes (DHCPACK), it's already too late: the kernel will not overwrite
> the fallback setting it has set previously.

Could we not remember that the address present is a fallback and let
the DHCPACK overwrite it?

[PATCH v2] net: ipconfig: Allow DNS to be overwritten by DHCPACK

[Martin Wetterwald]

Some DHCP server implementations only send the important requested DHCP
options in the final BOOTP reply (DHCPACK).
One example is systemd-networkd.
However, RFC2131, in section 4.3.1 states:

> The server MUST return to the client:
> [...]
> o Parameters requested by the client, according to the following
>   rules:
>
>      -- IF the server has been explicitly configured with a default
>         value for the parameter, the server MUST include that value
>         in an appropriate option in the 'option' field, ELSE

I've reported the issue here:
https://github.com/systemd/systemd/issues/27471

Linux PNP DHCP client implementation only takes into account the DNS
servers received in the first BOOTP reply (DHCPOFFER).
This usually isn't an issue as servers are required to put the same
values in the DHCPOFFER and DHCPACK.
However, RFC2131, in section 4.3.2 states:

> Any configuration parameters in the DHCPACK message SHOULD NOT
> conflict with those in the earlier DHCPOFFER message to which the
> client is responding.  The client SHOULD use the parameters in the
> DHCPACK message for configuration.

When making Linux PNP DHCP client (cmdline ip=dhcp) interact with
systemd-networkd DHCP server, an interesting "protocol misunderstanding"
happens:
Because DNS servers were only specified in the DHCPACK and not in the
DHCPOFFER, Linux will not catch the correct DNS servers: in the first
BOOTP reply (DHCPOFFER), it sees that there is no DNS, and sets as
fallback the IP of the DHCP server itself. When the second BOOTP reply
comes (DHCPACK), it's already too late: the kernel will not overwrite
the fallback setting it has set previously.

This patch makes the kernel overwrite its fallback DNS by DNS servers
specified in the DHCPACK if any.
---
v2:
  - Only overwrite DNS servers if it was the fallback DNS.

 net/ipv4/ipconfig.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
index e90bc0aa85c7..aa90273073c1 100644
--- a/net/ipv4/ipconfig.c
+++ b/net/ipv4/ipconfig.c
@@ -179,6 +179,9 @@ static volatile int ic_got_reply __initdata;    /*
Proto(s) that replied */
 #endif
 #ifdef IPCONFIG_DHCP
 static int ic_dhcp_msgtype __initdata;    /* DHCP msg type received */
+
+/* DHCPACK can overwrite DNS if fallback was set upon first BOOTP reply */
+static int ic_nameservers_fallback __initdata;
 #endif


@@ -938,7 +941,12 @@ static void __init ic_do_bootp_ext(u8 *ext)
         if (servers > CONF_NAMESERVERS_MAX)
             servers = CONF_NAMESERVERS_MAX;
         for (i = 0; i < servers; i++) {
+#ifdef IPCONFIG_DHCP
+            if (ic_nameservers[i] == NONE ||
+                ic_nameservers_fallback)
+#else
             if (ic_nameservers[i] == NONE)
+#endif
                 memcpy(&ic_nameservers[i], ext+1+4*i, 4);
         }
         break;
@@ -1158,8 +1166,12 @@ static int __init ic_bootp_recv(struct sk_buff
*skb, struct net_device *dev, str
     ic_addrservaddr = b->iph.saddr;
     if (ic_gateway == NONE && b->relay_ip)
         ic_gateway = b->relay_ip;
-    if (ic_nameservers[0] == NONE)
+    if (ic_nameservers[0] == NONE) {
         ic_nameservers[0] = ic_servaddr;
+#ifdef IPCONFIG_DHCP
+        ic_nameservers_fallback = 1;
+#endif
+    }
     ic_got_reply = IC_BOOTP;

 drop_unlock:
-- 
2.40.1

Re: [PATCH v2] net: ipconfig: Allow DNS to be overwritten by DHCPACK

[Paolo Abeni]

On Thu, 2023-05-04 at 10:00 +0200, Martin Wetterwald wrote:
> Some DHCP server implementations only send the important requested DHCP
> options in the final BOOTP reply (DHCPACK).
> One example is systemd-networkd.
> However, RFC2131, in section 4.3.1 states:
> 
> > The server MUST return to the client:
> > [...]
> > o Parameters requested by the client, according to the following
> >   rules:
> > 
> >      -- IF the server has been explicitly configured with a default
> >         value for the parameter, the server MUST include that value
> >         in an appropriate option in the 'option' field, ELSE
> 
> I've reported the issue here:
> https://github.com/systemd/systemd/issues/27471
> 
> Linux PNP DHCP client implementation only takes into account the DNS
> servers received in the first BOOTP reply (DHCPOFFER).
> This usually isn't an issue as servers are required to put the same
> values in the DHCPOFFER and DHCPACK.
> However, RFC2131, in section 4.3.2 states:
> 
> > Any configuration parameters in the DHCPACK message SHOULD NOT
> > conflict with those in the earlier DHCPOFFER message to which the
> > client is responding.  The client SHOULD use the parameters in the
> > DHCPACK message for configuration.
> 
> When making Linux PNP DHCP client (cmdline ip=dhcp) interact with
> systemd-networkd DHCP server, an interesting "protocol misunderstanding"
> happens:
> Because DNS servers were only specified in the DHCPACK and not in the
> DHCPOFFER, Linux will not catch the correct DNS servers: in the first
> BOOTP reply (DHCPOFFER), it sees that there is no DNS, and sets as
> fallback the IP of the DHCP server itself. When the second BOOTP reply
> comes (DHCPACK), it's already too late: the kernel will not overwrite
> the fallback setting it has set previously.
> 
> This patch makes the kernel overwrite its fallback DNS by DNS servers
> specified in the DHCPACK if any.

As strictly speaking this looks like a change of behavior more than a
fix, I think it should land on net-next so it needs to wait for net-
next to open, see below.

There are a few issues to be addresses. The patch is mangled - context
lines are truncated to 80 bytes, corrupting the patch itself. Please
double check your setup to solve such issue.

Please specify the target tree inside the patch subj.

Please do not reply with new version of this patch inside the same
thread.
	
## Form letter - net-next-closed

The merge window for v6.3 has begun and therefore net-next is closed
for new drivers, features, code refactoring and optimizations.
We are currently accepting bug fixes only.

Please repost when net-next reopens after May 8th.

RFC patches sent for review only are obviously welcome at any time.

See:
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle

Cheers,

Paolo

Re: [PATCH v2] net: ipconfig: Allow DNS to be overwritten by DHCPACK

[Jakub Kicinski]

On Thu, 4 May 2023 10:00:04 +0200 Martin Wetterwald wrote:
> +#ifdef IPCONFIG_DHCP
> +            if (ic_nameservers[i] == NONE ||
> +                ic_nameservers_fallback)
> +#else
>              if (ic_nameservers[i] == NONE)
> +#endif

FWIW I'd vote to move the variable out of the #ifdef and avoid the hairy
code, it's just 4 bytes.