From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 03819C433F5 for ; Wed, 16 Feb 2022 16:04:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235893AbiBPQFA (ORCPT ); Wed, 16 Feb 2022 11:05:00 -0500 Received: from mxb-00190b01.gslb.pphosted.com ([23.128.96.19]:55086 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233585AbiBPQFA (ORCPT ); Wed, 16 Feb 2022 11:05:00 -0500 Received: from mail.netfilter.org (mail.netfilter.org [217.70.188.207]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 8794A2A8D14; Wed, 16 Feb 2022 08:04:47 -0800 (PST) Received: from netfilter.org (unknown [78.30.32.163]) by mail.netfilter.org (Postfix) with ESMTPSA id 4AD41601C6; Wed, 16 Feb 2022 17:04:06 +0100 (CET) Date: Wed, 16 Feb 2022 17:04:40 +0100 From: Pablo Neira Ayuso To: Florian Westphal Cc: Gal Pressman , netfilter-devel@vger.kernel.org, davem@davemloft.net, netdev@vger.kernel.org, kuba@kernel.org, kevmitch@arista.com Subject: Re: [PATCH net-next 01/14] netfilter: conntrack: mark UDP zero checksum as CHECKSUM_UNNECESSARY Message-ID: References: <20220209133616.165104-1-pablo@netfilter.org> <20220209133616.165104-2-pablo@netfilter.org> <7eed8111-42d7-63e1-d289-346a596fc933@nvidia.com> <20220216152842.GA20500@breakpoint.cc> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20220216152842.GA20500@breakpoint.cc> Precedence: bulk List-ID: X-Mailing-List: netfilter-devel@vger.kernel.org On Wed, Feb 16, 2022 at 04:28:42PM +0100, Florian Westphal wrote: > Gal Pressman wrote: > > [ CC patch author ] > > > > The udp_error function verifies the checksum of incoming UDP packets if > > > one is set. This has the desirable side effect of setting skb->ip_summed > > > to CHECKSUM_COMPLETE, signalling that this verification need not be > > > repeated further up the stack. > > > > > > Conversely, when the UDP checksum is empty, which is perfectly legal (at least > > > inside IPv4), udp_error previously left no trace that the checksum had been > > > deemed acceptable. > > > > > > This was a problem in particular for nf_reject_ipv4, which verifies the > > > checksum in nf_send_unreach() before sending ICMP_DEST_UNREACH. It makes > > > no accommodation for zero UDP checksums unless they are already marked > > > as CHECKSUM_UNNECESSARY. > > > > > > This commit ensures packets with empty UDP checksum are marked as > > > CHECKSUM_UNNECESSARY, which is explicitly recommended in skbuff.h. > > > > > > Signed-off-by: Kevin Mitchell > > > Acked-by: Florian Westphal > > > Signed-off-by: Pablo Neira Ayuso > > > --- > > > net/netfilter/nf_conntrack_proto_udp.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/net/netfilter/nf_conntrack_proto_udp.c b/net/netfilter/nf_conntrack_proto_udp.c > > > index 3b516cffc779..12f793d8fe0c 100644 > > > --- a/net/netfilter/nf_conntrack_proto_udp.c > > > +++ b/net/netfilter/nf_conntrack_proto_udp.c > > > @@ -63,8 +63,10 @@ static bool udp_error(struct sk_buff *skb, > > > } > > > > > > /* Packet with no checksum */ > > > - if (!hdr->check) > > > + if (!hdr->check) { > > > + skb->ip_summed = CHECKSUM_UNNECESSARY; > > > return false; > > > + } > > > > > > /* Checksum invalid? Ignore. > > > * We skip checking packets on the outgoing path > > > > Hey, > > > > I think this patch broke geneve tunnels, or possibly all udp tunnels? > > > > A simple test that creates two geneve tunnels and runs tcp iperf fails > > and results in checksum errors (TcpInCsumErrors). > > > > Any idea how to solve that? Maybe 'skb->csum_level' needs some adjustments? > > Probably better to revert and patch nf_reject instead to > handle 0 udp csum? Agreed.