netdev - Re: [PATCH net-next] udp: expand SKB_DROP_REASON_UDP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <67c9fe2af078b_1bb0a2942a@willemb.c.googlers.com.notmuch>
Date: Thu, 06 Mar 2025 14:57:30 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Eric Dumazet <edumazet@...gle.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: "David S . Miller" <davem@...emloft.net>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>, 
 Willem de Bruijn <willemb@...gle.com>, 
 David Ahern <dsahern@...nel.org>, 
 Simon Horman <horms@...nel.org>, 
 netdev@...r.kernel.org, 
 eric.dumazet@...il.com
Subject: Re: [PATCH net-next] udp: expand SKB_DROP_REASON_UDP_CSUM use

Eric Dumazet wrote:
> On Thu, Mar 6, 2025 at 8:01 PM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > Eric Dumazet wrote:
> > > Use SKB_DROP_REASON_UDP_CSUM in __first_packet_length()
> > > and udp_read_skb() when dropping a packet because of
> > > a wrong UDP checksum.
> > >
> > > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > > ---
> > >  net/ipv4/udp.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > > index 17c7736d8349433ad2d4cbcc9414b2f8112610af..39c3adf333b5f02ca53f768c918c75f2fc7f93ac 100644
> > > --- a/net/ipv4/udp.c
> > > +++ b/net/ipv4/udp.c
> > > @@ -1848,7 +1848,7 @@ static struct sk_buff *__first_packet_length(struct sock *sk,
> > >                       atomic_inc(&sk->sk_drops);
> > >                       __skb_unlink(skb, rcvq);
> > >                       *total += skb->truesize;
> > > -                     kfree_skb(skb);
> > > +                     kfree_skb_reason(skb, SKB_DROP_REASON_UDP_CSUM);
> > >               } else {
> > >                       udp_skb_csum_unnecessary_set(skb);
> > >                       break;
> > > @@ -2002,7 +2002,7 @@ int udp_read_skb(struct sock *sk, skb_read_actor_t recv_actor)
> > >               __UDP_INC_STATS(net, UDP_MIB_CSUMERRORS, is_udplite);
> > >               __UDP_INC_STATS(net, UDP_MIB_INERRORS, is_udplite);
> > >               atomic_inc(&sk->sk_drops);
> > > -             kfree_skb(skb);
> > > +             kfree_skb_reason(skb, SKB_DROP_REASON_UDP_CSUM);
> > >               goto try_again;
> > >       }
> >
> > From a quick search for UDP_MIB_CSUMERRORS, one more case with regular
> > kfree_skb:
> >
> > csum_copy_err:
> >         if (!__sk_queue_drop_skb(sk, &udp_sk(sk)->reader_queue, skb, flags,
> >                                  udp_skb_destructor)) {
> >                 UDP_INC_STATS(sock_net(sk), UDP_MIB_CSUMERRORS, is_udplite);
> >                 UDP_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
> >         }
> >         kfree_skb(skb);
> 
> Right, I was unsure because of the conditional SNMP updates.

That seems to only suppress the update if peeking and the skb was
already dequeued (ENOENT).

Frankly, we probably never intended to increment this counter when
peeking, as it is intended to be a per-datagram counter, not a
per-recvmsg counter.

I think ever erring on the side of extra increments in the unlikely
case of ENOENT is fine.

Cleaner is perhaps to have a kfree_skb_reason inside that branch and
a consume_skb in the else.