Re: [PATCH 3/3] conf: Be more precise about minimum MTUs

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 19 Feb 2025 14:14:29 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Currently we reject the -m option if given a value less than ETH_MAX_MTU

ETH_MIN_MTU

> (68).  That define is derived from the kernel, but its name is misleading:
> it doesn't really have anything to do with Ethernet per se, but is rather
> the minimum payload any L2 link must be able to handle in order to carry
> IPv4.

Yes, that should be IPV4_MIN_MTU instead, but it was only added as
recently as 4.14 kernels, so I opted for ETH_MIN_MTU. A misnomer as you
pointed out, but safe.

> For IPv6, it's not sufficient: that requires an MTU of at least
> 1280.
> 
> Furthermore, the value of 68 is the minimum IP *fragment* size the link
> must be able to carry.  Since we don't support IP fragmentation, it's not
> sufficient for us.  Instead we should clamp the MTU to 576 for IPv4 - the
> minimum IP datagram size that all hosts must be able to accept.

First off, the only assumption in RFC 791 terms we can _perhaps_ make is
that we are some kind of "module" (also called "node", could be host or
router), not a (full) host. Maybe not even a module. So, with that
regard, we don't need to be prepared to _accept_ (for ourselves as
destination) any particular datagram size.

Second, even if all hosts need to be able to accept 576-byte datagrams,
that doesn't mean that all links need to be able to carry them. The MTU
refers _to the link_, not to what a host is able to accept.

And that's the reason why you can set 68 bytes as MTU on most network
interfaces on Linux. We set sub-576 values ourselves in tests:

$ grep -rn "mtu 256" *
passt_tcp:95:guest	ip link set dev __IFNAME__ mtu 256
passt_vu_tcp:95:guest	ip link set dev __IFNAME__ mtu 256

That is, indeed, all hosts (not "modules") need to be able to accept
(not "forward") datagram sizes of at least 576 bytes... but that's only
assuming you can deliver those datagrams to them.

This is not just a theoretical matter. As late as 2018, I was made
aware of a setup with several (local!) nodes with links between them
having ~380 bytes as MTU.

Sure enough, the reason why I know about this was an issue coming from
the same flawed assumption made in kernel commit c9fefa08190f
("ip6_tunnel: get the min mtu properly in ip6_tnl_xmit"), and fixed by
82a40777de12 ("ip6_tunnel: use the right value for ipv4 min mtu check
in ip6_tnl_xmit").

See also commit b4331a681822 ("vti6: Change minimum MTU to IPV4_MIN_MTU,
vti6 can carry IPv4 too") on the subject of what links can carry vs.
what endpoints should be able to forward.

> Move the verification of the MTU's lower bound to logic specific to the IP
> versions and correct those errors.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c | 20 +++++++++++++++-----
>  ip.h   |  7 +++++++
>  util.h |  3 ---
>  3 files changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index c5ee07b0..e127acc1 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1663,9 +1663,9 @@ void conf(struct ctx *c, int argc, char **argv)
>  			if (errno || *e)
>  				die("Invalid MTU: %s", optarg);
>  
> -			if (mtu && (mtu < ETH_MIN_MTU || mtu > ETH_MAX_MTU)) {
> -				die("MTU %lu out of range (%u..%u)", mtu,
> -				    ETH_MIN_MTU, ETH_MAX_MTU);
> +			if (mtu > ETH_MAX_MTU) {
> +				die("MTU %lu too large (max %u)",
> +				    mtu, ETH_MAX_MTU);
>  			}
>  
>  			c->mtu = mtu;
> @@ -1838,10 +1838,20 @@ void conf(struct ctx *c, int argc, char **argv)
>  	log_conf_parsed = true;		/* Stop printing everything */
>  
>  	nl_sock_init(c, false);
> -	if (!v6_only)
> +	if (!v6_only) {
> +		if (c->mtu < IPV4_MINMAX_DATAGRAM) {

Now, if you want to make this symmetric with the IPv6 case, we could
also move this here... it just unnecessarily adds lines of code, and
this function is already (necessarily) rather long.

> +			die("MTU %"PRIu16" is too small for IPv4 (minimum %u)",
> +			    c->mtu, IPV4_MINMAX_DATAGRAM);
> +		}
>  		c->ifi4 = conf_ip4(ifi4, &c->ip4);
> -	if (!v4_only)
> +	}
> +	if (!v4_only) {
> +		if (c->mtu < IPV6_MIN_MTU) {
> +			die("MTU %"PRIu16" is too small for IPv6 (minimum %u)",
> +			    c->mtu, IPV6_MIN_MTU);

Does the fact that we don't disable IPv6 imply that IPv6 must be
working at all times? In my opinion not.

It's also rather convenient to be able to specify '-m 200' (for
whatever test) without having to give '-4' explicitly.

From a functionality perspective, I think warn() would be a better
choice.

> +		}
>  		c->ifi6 = conf_ip6(ifi6, &c->ip6);
> +	}
>  	if ((*c->ip4.ifname_out && !c->ifi4) ||
>  	    (*c->ip6.ifname_out && !c->ifi6))
>  		die("External interface not usable");
> diff --git a/ip.h b/ip.h
> index 1544dbf4..8f5262fa 100644
> --- a/ip.h
> +++ b/ip.h
> @@ -104,4 +104,11 @@ static const struct in6_addr in6addr_ll_all_nodes = {
>  /* IPv4 Limited Broadcast (RFC 919, Section 7), 255.255.255.255 */
>  static const struct in_addr in4addr_broadcast = { 0xffffffff };
>  
> +/* Minimum IP datagram size all hosts must be prepared to accept (RFC 791) */
> +#define IPV4_MINMAX_DATAGRAM	576
> +
> +#ifndef IPV6_MIN_MTU
> +#define IPV6_MIN_MTU		1280
> +#endif
> +
>  #endif /* IP_H */
> diff --git a/util.h b/util.h
> index 50e96d32..bdca5ee6 100644
> --- a/util.h
> +++ b/util.h
> @@ -34,9 +34,6 @@
>  #ifndef ETH_MAX_MTU
>  #define ETH_MAX_MTU			USHRT_MAX
>  #endif
> -#ifndef ETH_MIN_MTU
> -#define ETH_MIN_MTU			68
> -#endif

I would be fine with using IPV4_MIN_MTU by the way and adding a
fallback for it (there's no such thing as IP_MIN_MTU).

>  #ifndef IP_MAX_MTU
>  #define IP_MAX_MTU			USHRT_MAX
>  #endif

-- 
Stefano