From 5ac1235b99d33085d0574ef81b95ceafcf79db34 Mon Sep 17 00:00:00 2001
From: Roy Marples <roy@marples.name>
Date: Mon, 29 Jul 2024 16:17:08 +0100
Subject: [PATCH] DHCP: No longer set interface mtu (#346)

We've been enforcing an interface MTU that is slightly larger
than the minimum for some time.
Instead, log an error than the MTU is smaller than the minimum
to send a BOOTP message.

The DHCP MTU is only used when adding routes as setting the
interface MTU can cause a PHY reset which is bad.

Fixes #345
---
 src/dhcp-common.h |  5 -----
 src/dhcp.c        | 31 ++++++++++++++++++++-----------
 src/if-options.c  |  2 +-
 src/if.c          | 21 ++++++---------------
 src/if.h          |  4 +---
 src/ipv4.h        |  4 ++++
 6 files changed, 32 insertions(+), 35 deletions(-)

diff --git a/src/dhcp-common.h b/src/dhcp-common.h
index 51413140..6eaf3b3c 100644
--- a/src/dhcp-common.h
+++ b/src/dhcp-common.h
@@ -46,11 +46,6 @@
 #define NS_MAXLABEL MAXLABEL
 #endif
 
-/* Max MTU - defines dhcp option length */
-#define	IP_UDP_SIZE		  28
-#define	MTU_MAX			1500 - IP_UDP_SIZE
-#define	MTU_MIN			 576 + IP_UDP_SIZE
-
 #define	OT_REQUEST		(1 << 0)
 #define	OT_UINT8		(1 << 1)
 #define	OT_INT8			(1 << 2)
diff --git a/src/dhcp.c b/src/dhcp.c
index 0267a2a9..d7a7e6f6 100644
--- a/src/dhcp.c
+++ b/src/dhcp.c
@@ -97,6 +97,8 @@
 __CTASSERT(sizeof(struct ip)		== 20);
 __CTASSERT(sizeof(struct udphdr)	== 8);
 __CTASSERT(sizeof(struct bootp)		== 300);
+#define IP_UDP_SIZE	sizeof(struct ip) + sizeof(struct udphdr)
+#define BOOTP_MIN_MTU	IP_UDP_SIZE + sizeof(struct bootp)
 
 struct dhcp_op {
 	uint8_t value;
@@ -676,6 +678,8 @@ dhcp_get_mtu(const struct interface *ifp)
 	    get_option_uint16(ifp->ctx, &mtu,
 			      state->new, state->new_len, DHO_MTU) == -1)
 		return 0;
+	if (mtu < IPV4_MMTU)
+		return IPV4_MMTU;
 	return mtu;
 }
 
@@ -740,19 +744,24 @@ make_message(struct bootp **bootpm, const struct interface *ifp, uint8_t type)
 	uint8_t *auth, auth_len;
 #endif
 
-	if ((mtu = if_getmtu(ifp)) == -1)
+	/* We could take the DHCPv6 approach and work out the
+	 * message length up front rather than this big hammer approach. */
+	if ((mtu = if_getmtu(ifp)) == -1) {
 		logerr("%s: if_getmtu", ifp->name);
-	else if (mtu < MTU_MIN) {
-		if (if_setmtu(ifp, MTU_MIN) == -1)
-			logerr("%s: if_setmtu", ifp->name);
-		mtu = MTU_MIN;
+		return -1;
+	}
+	if ((size_t)mtu < BOOTP_MIN_MTU) {
+		logerr("%s: interface mtu is too small (%d<%zu)",
+		    ifp->name, mtu, BOOTP_MIN_MTU);
+		return -1;
 	}
 
-	if (ifo->options & DHCPCD_BOOTP)
-		bootp = calloc(1, sizeof (*bootp));
-	else
+	if (ifo->options & DHCPCD_BOOTP) {
+		bootp = calloc(1, sizeof(*bootp));
+	} else {
 		/* Make the maximal message we could send */
-		bootp = calloc(1, (size_t)(mtu - IP_UDP_SIZE));
+		bootp = calloc(1, (size_t)mtu - IP_UDP_SIZE);
+	}
 
 	if (bootp == NULL)
 		return -1;
@@ -797,7 +806,7 @@ make_message(struct bootp **bootpm, const struct interface *ifp, uint8_t type)
 		return sizeof(*bootp);
 
 	p = bootp->vend;
-	e = (uint8_t *)bootp + (mtu - IP_UDP_SIZE) - 1; /* -1 for DHO_END */
+	e = (uint8_t *)bootp + ((size_t)mtu - IP_UDP_SIZE - 1/* DHO_END */);
 
 	ul = htonl(MAGIC_COOKIE);
 	memcpy(p, &ul, sizeof(ul));
@@ -924,7 +933,7 @@ make_message(struct bootp **bootpm, const struct interface *ifp, uint8_t type)
 			AREA_CHECK(2);
 			*p++ = DHO_MAXMESSAGESIZE;
 			*p++ = 2;
-			sz = htons((uint16_t)(mtu - IP_UDP_SIZE));
+			sz = htons((uint16_t)((size_t)mtu - IP_UDP_SIZE));
 			memcpy(p, &sz, 2);
 			p += 2;
 		}
diff --git a/src/if-options.c b/src/if-options.c
index ee8644d2..4cdc294b 100644
--- a/src/if-options.c
+++ b/src/if-options.c
@@ -1218,7 +1218,7 @@ parse_option(struct dhcpcd_ctx *ctx, const char *ifname, struct if_options *ifo,
 			if (p == NULL)
 				break;
 			ifo->mtu = (unsigned int)strtou(p, NULL, 0,
-			    MTU_MIN, MTU_MAX, &e);
+			    IPV4_MMTU, UINT_MAX, &e);
 			if (e) {
 				logerrx("invalid MTU %s", p);
 				return -1;
diff --git a/src/if.c b/src/if.c
index 3ed9f6b3..5da7d6d7 100644
--- a/src/if.c
+++ b/src/if.c
@@ -852,27 +852,18 @@ if_loopback(struct dhcpcd_ctx *ctx)
 }
 
 int
-if_domtu(const struct interface *ifp, short int mtu)
+if_getmtu(const struct interface *ifp)
 {
-	int r;
-	struct ifreq ifr;
-
 #ifdef __sun
-	if (mtu == 0)
-		return if_mtu_os(ifp);
-#endif
+	return if_mtu_os(ifp);
+#else
+	struct ifreq ifr = { .ifr_mtu = 0 };
 
-	memset(&ifr, 0, sizeof(ifr));
 	strlcpy(ifr.ifr_name, ifp->name, sizeof(ifr.ifr_name));
-	ifr.ifr_mtu = mtu;
-	if (mtu != 0)
-		r = if_ioctl(ifp->ctx, SIOCSIFMTU, &ifr, sizeof(ifr));
-	else
-		r = pioctl(ifp->ctx, SIOCGIFMTU, &ifr, sizeof(ifr));
-
-	if (r == -1)
+	if (pioctl(ifp->ctx, SIOCGIFMTU, &ifr, sizeof(ifr)) == -1)
 		return -1;
 	return ifr.ifr_mtu;
+#endif
 }
 
 #ifdef ALIAS_ADDR
diff --git a/src/if.h b/src/if.h
index df5f921e..5b7848f0 100644
--- a/src/if.h
+++ b/src/if.h
@@ -178,9 +178,7 @@ struct interface *if_find(struct if_head *, const char *);
 struct interface *if_findindex(struct if_head *, unsigned int);
 struct interface *if_loopback(struct dhcpcd_ctx *);
 void if_free(struct interface *);
-int if_domtu(const struct interface *, short int);
-#define if_getmtu(ifp) if_domtu((ifp), 0)
-#define if_setmtu(ifp, mtu) if_domtu((ifp), (mtu))
+int if_getmtu(const struct interface *);
 int if_carrier(struct interface *, const void *);
 bool if_roaming(struct interface *);
 
diff --git a/src/ipv4.h b/src/ipv4.h
index 4f64534a..2d8de7d0 100644
--- a/src/ipv4.h
+++ b/src/ipv4.h
@@ -58,6 +58,10 @@
     (((uint32_t)(A) & 0x000000ff) << 24))
 #endif /* BYTE_ORDER */
 
+#ifndef IPV4_MMTU
+#define IPV4_MMTU 68
+#endif
+
 #ifdef __sun
    /* Solaris lacks these defines.
     * While it supports DaD, to seems to only expose IFF_DUPLICATE
-- 
2.47.2