From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id 21FF35A026F for ; Tue, 4 Jul 2023 13:21:30 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1688469689; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=f8ok2ZKTRwgYIye0cgyNivHBwCtXjWfAP2Tyszwiaj0=; b=M28vM9Z9/gTGKiUmUnEk8M29uvFOQQTxYcJwJWHknuqr/Wm1nQ5BEv3K68GJzEfKL1QfMi sE3oRXnq0O1nl+fi2698KCadsxiLhdhS+rDPTivIL7LRElpjPNBnMNVV5fZTDtQvJBnXp/ CExq+v+DYIAmwUNIMXhjmb/VDCHtEEg= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-241-ewGyeXebN1uYZv9tHFwkKQ-1; Tue, 04 Jul 2023 07:21:28 -0400 X-MC-Unique: ewGyeXebN1uYZv9tHFwkKQ-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id A73D53C11C63; Tue, 4 Jul 2023 11:21:27 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.37]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 041DEC00049; Tue, 4 Jul 2023 11:21:26 +0000 (UTC) Date: Tue, 4 Jul 2023 13:21:04 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH] tap: Explicitly drop IPv4 fragments, and give a warning Message-ID: <20230704132104.48106368@elisabeth> In-Reply-To: <20230704043623.1143288-1-david@gibson.dropbear.id.au> References: <20230704043623.1143288-1-david@gibson.dropbear.id.au> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: NC5YCJVUMGWEPQEE3KJK3DHTNSEXNR3V X-Message-ID-Hash: NC5YCJVUMGWEPQEE3KJK3DHTNSEXNR3V X-MailFrom: sbrivio@redhat.com 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: On Tue, 4 Jul 2023 14:36:23 +1000 David Gibson wrote: > We don't handle defragmentation of IP packets coming from the tap side, > and we're unlikely to any time soon (with our large MTU, it's not useful > 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 if we > do receive IP fragments. > > Change the code to explicitly drop fragmented packets, and print a rate > limited warning if we do encounter them. > > Bugzilla: https://bugs.passt.top/show_bug.cgi?id=62 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. 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. > Signed-off-by: David Gibson > --- > tap.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > 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); > > #define TAP_SEQS 128 /* Different L4 tuples in one batch */ > +#define FRAGMENT_MSG_RATE 10 /* # seconds between fragment warnings */ > > /** > * tap_send() - Send frame, with qemu socket header if needed > @@ -543,6 +544,32 @@ static void tap_packet_debug(const struct iphdr *iph, > } > } > > +/** > + * tap4_is_fragment() - Determine if a packet is an IP fragment 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. > + * @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 = now->tv_sec; > + num_dropped = 0; > + } > + return true; > + } > + return false; > +} > + > /** > * tap4_handler() - IPv4 and ARP packet handler for tap file descriptor > * @c: Execution context > @@ -591,6 +618,10 @@ resume: > hlen > l3_len) > continue; > > + /* We don't handle IP fragments, drop them */ > + if (tap4_is_fragment(iph, now)) > + continue; > + > l4_len = l3_len - hlen; > > if (iph->saddr && c->ip4.addr_seen.s_addr != iph->saddr) { -- Stefano