From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 085455A026D for ; Wed, 5 Jul 2023 06:35:38 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1688531731; bh=QD9u6n3TfPfvGCwNSQViuV7fer4qKcJ0UIWaxR+M/zo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=lzL6Nah9a/Xb4s/7H3RuzxSjeGdRU64Nwn3SwEZnXb3a2Fhgj+gyXTFEnrGgEKfsm kgvDXch2mjuhhhKLaAVo8NUYfwYcwii3V4f3TGWRNRENVwQqJnHvrC+qXr1wqoV1H8 UuvU6VVHEuwguxyW27NNjDmHR18sEP3HvBHsDPYA= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Qwn0321L7z4wqZ; Wed, 5 Jul 2023 14:35:31 +1000 (AEST) Date: Wed, 5 Jul 2023 14:20:43 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH] tap: Explicitly drop IPv4 fragments, and give a warning Message-ID: References: <20230704043623.1143288-1-david@gibson.dropbear.id.au> <20230704132104.48106368@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="7lO/H/NmM3mBIcV7" Content-Disposition: inline In-Reply-To: Message-ID-Hash: P7RMZMMZE3YG477CRZAPO6FOUMNPPEIM X-Message-ID-Hash: P7RMZMMZE3YG477CRZAPO6FOUMNPPEIM 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: --7lO/H/NmM3mBIcV7 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Do not apply this version of the patch: I just discovered it's entirely breaking ping. Investigating... On Wed, Jul 05, 2023 at 11:04:27AM +1000, David Gibson wrote: > On Tue, Jul 04, 2023 at 01:21:04PM +0200, Stefano Brivio wrote: > > On Tue, 4 Jul 2023 14:36:23 +1000 > > David Gibson wrote: > >=20 > > > We don't handle defragmentation of IP packets coming from the tap sid= e, > > > and we're unlikely to any time soon (with our large MTU, it's not use= ful > > > for practical use cases). Currently, however, we simply ignore the > > > fragmentation flags and treat fragments as though they were whole IP > > > packets. This isn't ideal and can lead to rather cryptic behaviour i= f we > > > do receive IP fragments. > > >=20 > > > Change the code to explicitly drop fragmented packets, and print a ra= te > > > limited warning if we do encounter them. > > >=20 > > > Bugzilla: https://bugs.passt.top/show_bug.cgi?id=3D62 > >=20 > > By the way, I silently replaced those with "Link:" in the past, just in > > case we want to automate something around it one day, to avoid > > differences between references to different bug trackers. >=20 > Oh, ok. I'll keep that in mind. >=20 > > Once upon a time, I wrote some scripting to automatically link HTML > > reports with (Linux kernel) commits to bug trackers, and it was quite > > painful to discover all possible spellings of "Bugzilla" plus a few > > others, hence my thought. But let me know if something speaks against > > this. >=20 > No, that makes sense. >=20 > > > Signed-off-by: David Gibson > > > --- > > > tap.c | 31 +++++++++++++++++++++++++++++++ > > > 1 file changed, 31 insertions(+) > > >=20 > > > diff --git a/tap.c b/tap.c > > > index e3235299..2e6939fa 100644 > > > --- a/tap.c > > > +++ b/tap.c > > > @@ -62,6 +62,7 @@ static PACKET_POOL_NOINIT(pool_tap4, TAP_MSGS, pkt_= buf); > > > static PACKET_POOL_NOINIT(pool_tap6, TAP_MSGS, pkt_buf); > > > =20 > > > #define TAP_SEQS 128 /* Different L4 tuples in one batch */ > > > +#define FRAGMENT_MSG_RATE 10 /* # seconds between fragment warnings= */ > > > =20 > > > /** > > > * tap_send() - Send frame, with qemu socket header if needed > > > @@ -543,6 +544,32 @@ static void tap_packet_debug(const struct iphdr = *iph, > > > } > > > } > > > =20 > > > +/** > > > + * tap4_is_fragment() - Determine if a packet is an IP fragment > >=20 > > This is actually independent from the "tap" "side", it could also be > > e.g. ipv4_is_fragment(), in util.c. Not a strong preference though, I > > guess we can also merge it as it is. >=20 > Well.. the detection of fragments is independent, but the warning > message is specific to tap. I'm inclined to leave it as is, at least > until we have a need for this logic somewhere else, at which point we > can refactor. >=20 > >=20 > > > + * @iph: IPv4 header (length already validated) > > > + * @now: Current timestamp > > > + * > > > + * Return: true if iph is an IP fragment, false otherwise > > > + */ > > > +static bool tap4_is_fragment(const struct iphdr *iph, > > > + const struct timespec *now) > > > +{ > > > + if (iph->frag_off & ~IP_DF) { > > > + /* Ratelimit messages */ > > > + static time_t last_message; > > > + static unsigned num_dropped; > > > + > > > + num_dropped++; > > > + if (now->tv_sec - last_message > FRAGMENT_MSG_RATE) { > > > + warn("Can't process IPv4 fragments (%lu dropped)", num_dropped); > > > + last_message =3D now->tv_sec; > > > + num_dropped =3D 0; > > > + } > > > + return true; > > > + } > > > + return false; > > > +} > > > + > > > /** > > > * tap4_handler() - IPv4 and ARP packet handler for tap file descrip= tor > > > * @c: Execution context > > > @@ -591,6 +618,10 @@ resume: > > > hlen > l3_len) > > > continue; > > > =20 > > > + /* We don't handle IP fragments, drop them */ > > > + if (tap4_is_fragment(iph, now)) > > > + continue; > > > + > > > l4_len =3D l3_len - hlen; > > > =20 > > > if (iph->saddr && c->ip4.addr_seen.s_addr !=3D iph->saddr) { > >=20 >=20 --=20 David Gibson | 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 --7lO/H/NmM3mBIcV7 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmSk75QACgkQzQJF27ox 2Gdw9A/7BTia+gFMhc5ldfqQcTQtUb1IunimaSjZfLpyPh206M5k96HErWMcI2cZ PIO1stlRyjPBAr+/4Ev7/UEe11+SS6W0ginvAyNTs16tLJrxK6aqyCJgyK+CsJHz beZyy8cDZrGHpqlNA90RuszPKiwSsalBy2fOnSiVnH0fa3OIoN977TsG3Ada359a 3vQDEMuSbvLGnS/ci4pxWZLSf+9J3MZAW+r9Va4KfDCSMa8Lc+NIh1Ih/XAfShvk dJTpB1NZde9IT1tkqC6jv1pdUPm1Jo6tQOTIZSKibWuaMlmPVqB8wCBnpmd+MXc+ J3+6pdVVDmQfRpbCB6JL/D7b3mJOYCFmCGxhFmIPIVPGgvUgBkqetGvSO3NJQgzD F9MHTUCOqaHcR6w7dhm9JSGwiuHD2EGrKlcfaJ0c+CrJ+Lc4v0Hs08vIkLAhjlK9 +luKYjdL6ATWnRUtaQ/IyOYsBfh+xqgqDM5CUdrNYQ7kDxmCgRMaBpbwPHP0qWYv wwmLwQhTil8pUjn5NbJCOUtvLKaB3QjJ0wYn8UxUtI8Lq7xYoP/riU0VM5NMHECl 3gYTcl8v89DF782Q0JkoE7Xh1eZ84sMtEI0sGEbZW1Qc/sVN5E7JcNMOgzXL7Yyf axtNts3Y8YLwRHWPlrRCusAR+uz9xaT+VV49hZ4Ig1Tq63frjjg= =Ez4A -----END PGP SIGNATURE----- --7lO/H/NmM3mBIcV7--