From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202502 header.b=kyBQdv/i; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 7C4225A061F for ; Thu, 20 Feb 2025 11:07:05 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202502; t=1740046016; bh=/6KZtXbhOMMbRmqzD1K+i7+0WRAPYfCTlHTZ3desdSo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=kyBQdv/iQyoHcl2Og7SHBAP3osPvtqzr/x7QRBArt+IOHsULBZlpU60yIeEpf16t4 I4sFHMG3tT51kTy4D0NQsshqcGQnp/GVKwl3reklu6j+MJD55+oq7YxyHEqBETss/s K+BkuYuUwdQHR7/2CIwOqFEcR8IK/efkAdZkns/rsqBhZWzdtNa4mQR4WxBqNR0ijv B6/kVQADL5BnOTLwLzE+/SL1OO3LOYUPFWnAMdso6NOCmZO+OHcwRaWtEEUg81Z+0k 9+sTjEWdvqF7Re79kafw98Wi1NG662iwovMo5vY3kuFMxJraL1ewR7p89umCgPBOHl IKZ3flvF4gIAA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Yz87N065pz4x2g; Thu, 20 Feb 2025 21:06:56 +1100 (AEDT) Date: Thu, 20 Feb 2025 21:06:52 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 3/3] conf: Be more precise about minimum MTUs Message-ID: References: <20250219031429.3708026-1-david@gibson.dropbear.id.au> <20250219031429.3708026-4-david@gibson.dropbear.id.au> <20250219063728.309bf1ac@elisabeth> <20250220074540.318bee27@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="SL6f0sVVblR7s5TD" Content-Disposition: inline In-Reply-To: <20250220074540.318bee27@elisabeth> Message-ID-Hash: 6S34BZL7NPIL3W7FEQXGQDITCM2F26GU X-Message-ID-Hash: 6S34BZL7NPIL3W7FEQXGQDITCM2F26GU X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --SL6f0sVVblR7s5TD Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Feb 20, 2025 at 07:45:40AM +0100, Stefano Brivio wrote: > On Thu, 20 Feb 2025 14:55:30 +1100 > David Gibson wrote: >=20 > > On Wed, Feb 19, 2025 at 06:37:28AM +0100, Stefano Brivio wrote: > > > On Wed, 19 Feb 2025 14:14:29 +1100 > > > David Gibson wrote: > > > =20 > > > > Currently we reject the -m option if given a value less than ETH_MA= X_MTU =20 > > >=20 > > > ETH_MIN_MTU > > > =20 > > > > (68). That define is derived from the kernel, but its name is misl= eading: > > > > 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. =20 > > >=20 > > > 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 y= ou > > > pointed out, but safe. =20 > >=20 > > Ah, thanks, I hadn't realised that newer kernels had better named > > constants. When I respin I'll use matching names. > >=20 > > > > For IPv6, it's not sufficient: that requires an MTU of at least > > > > 1280. > > > >=20 > > > > 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. =20 > > >=20 > > > 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. > > >=20 > > > Second, even if all hosts need to be able to accept 576-byte datagram= s, > > > that doesn't mean that all links need to be able to carry them. The M= TU > > > refers _to the link_, not to what a host is able to accept. =20 > >=20 > > Ah... yes. I was thinking that that requirement implied that a link > > which can't fragment was useless if it couldn't carry 576-byte > > datagrams, but thinking over your examples here I realise I was > > mistaken. > >=20 > > > 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: > > >=20 > > > $ 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 > > >=20 > > > 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 on= ly > > > assuming you can deliver those datagrams to them. > > >=20 > > > 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. > > >=20 > > > 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"). > > >=20 > > > See also commit b4331a681822 ("vti6: Change minimum MTU to IPV4_MIN_M= TU, > > > vti6 can carry IPv4 too") on the subject of what links can carry vs. > > > what endpoints should be able to forward. > > > =20 > > > > Move the verification of the MTU's lower bound to logic specific to= the IP > > > > versions and correct those errors. > > > >=20 > > > > Signed-off-by: David Gibson > > > > --- > > > > conf.c | 20 +++++++++++++++----- > > > > ip.h | 7 +++++++ > > > > util.h | 3 --- > > > > 3 files changed, 22 insertions(+), 8 deletions(-) > > > >=20 > > > > 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 **arg= v) > > > > if (errno || *e) > > > > die("Invalid MTU: %s", optarg); > > > > =20 > > > > - 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); > > > > } > > > > =20 > > > > c->mtu =3D mtu; > > > > @@ -1838,10 +1838,20 @@ void conf(struct ctx *c, int argc, char **a= rgv) > > > > log_conf_parsed =3D true; /* Stop printing everything */ > > > > =20 > > > > nl_sock_init(c, false); > > > > - if (!v6_only) > > > > + if (!v6_only) { > > > > + if (c->mtu < IPV4_MINMAX_DATAGRAM) { =20 > > >=20 > > > 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. =20 > >=20 > > Sorry, I'm not following what change you're suggesting (or discussing?). >=20 > The exact change I quoted: moving the check on the minimum MTU to here: >=20 > if (c->mtu < IPV4_MINMAX_DATAGRAM) { =20 >=20 > compared to doing it earlier in conf(). But... the diff you're commenting on is already doing exactly that. What am I missing? > > > > + die("MTU %"PRIu16" is too small for IPv4 (minimum %u)", > > > > + c->mtu, IPV4_MINMAX_DATAGRAM); > > > > + } > > > > c->ifi4 =3D 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); =20 > > >=20 > > > Does the fact that we don't disable IPv6 imply that IPv6 must be > > > working at all times? In my opinion not. > > >=20 > > > It's also rather convenient to be able to specify '-m 200' (for > > > whatever test) without having to give '-4' explicitly. > > > =20 > > > >From a functionality perspective, I think warn() would be a better = =20 > > > choice. =20 > >=20 > > warn() and disable the relevant protocol. That makes sense, I'll make > > that change. >=20 > I don't think it makes sense to disable IPv4, highlighting quote: >=20 > > > Does the fact that we don't disable IPv6 imply that IPv6 must be > > > working at all times? In my opinion not. >=20 > ...you can advertise a small MTU for whatever reason. The guest might > configure it or not. The guest might change it later on. We have no way > to re-enable IPv6 once it's disabled, though. Ah... good point. > So let's just do what the user says, I would suggest, and warn them > that it *might* not work. There is zero functionality gained by > disabling IPv6. Ok, I'll send a v3 which does that. --=20 David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson --SL6f0sVVblR7s5TD Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAme2/rsACgkQzQJF27ox 2GcF0w/9Eebh5UmSmxxUaLshpUeQEJFAIKRD32R9ydUxra2QCWvhDpU2+L3M8xsf N+mYZ0StXIrTzzCjMNMVXwtqYd03gVhowlrwbsm4kOPMJWp+dJVDVrIIPNpB62b4 8uu1lHQs8zfpayfVr0lDZBYAmjC0xdQM0GNoUTNFH86yZhlDP1ZP0+wOOtQxyQm0 XOiBzCQXGCpbml/et7xfibxPn0NN3SWxafMuaa9LAqUJl0Lw56OG4TKlKnMNpUPT vSUEoClAUkldtzv+c9itdEQPBR+2CxsO5OMbIG5Hb69qEAjjqtL7BSIex2/O9O4Y 6uFRk91D1ValxIUx0/iAKEIzBgflTl/UvCcZutp4XaalAyeKLfWtdr0MN06QYrW2 PdSP0e17fOFn0ilN+WRhRG9ZqydQyleiVVRu6GqMWpeKDOwYS6fiZNjCTAg19PA8 k+BgUUNNH6MO9LOG73qSKzqhBPrKz6RMEn9ns3kXltOd7PKxhsRmiPO6VL/iCooE 5amDlaRFvm7KDIlbLYWJmIS93XhgFrTqVUGK3jV3O+RCiKFBWelNdayl+P8OuKlL OI6SB7MdQsOr3qKXdcbErF5xN2iRKTIyeNVdLiDvGU8e98QbIkmnGFTlxWNTadt+ YDuqag7WehwYrv7quQaeu+nhzvTlh7Z9ANJC7sYk/tUNDQSHVNY= =6oU9 -----END PGP SIGNATURE----- --SL6f0sVVblR7s5TD--