netdev - Re: [Patch bpf v3 8/8] skmsg: increase sk->sk

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]

Date:   Tue, 01 Jun 2021 20:39:38 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Cong Wang <xiyou.wangcong@...il.com>,
        John Fastabend <john.fastabend@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, Cong Wang <cong.wang@...edance.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jakub Sitnicki <jakub@...udflare.com>,
        Lorenz Bauer <lmb@...udflare.com>
Subject: Re: [Patch bpf v3 8/8] skmsg: increase sk->sk_drops when dropping
 packets

Cong Wang wrote:
> On Thu, May 27, 2021 at 10:27 PM John Fastabend
> <john.fastabend@...il.com> wrote:
> >
> > Cong Wang wrote:
> > > From: Cong Wang <cong.wang@...edance.com>
> > >
> > > It is hard to observe packet drops without increasing relevant
> > > drop counters, here we should increase sk->sk_drops which is
> > > a protocol-independent counter. Fortunately psock is always
> > > associated with a struct sock, we can just use psock->sk.
> > >
> > > Suggested-by: John Fastabend <john.fastabend@...il.com>
> > > Cc: Daniel Borkmann <daniel@...earbox.net>
> > > Cc: Jakub Sitnicki <jakub@...udflare.com>
> > > Cc: Lorenz Bauer <lmb@...udflare.com>
> > > Signed-off-by: Cong Wang <cong.wang@...edance.com>
> > > ---
> > >  net/core/skmsg.c | 22 ++++++++++++++--------
> > >  1 file changed, 14 insertions(+), 8 deletions(-)
> > >
> >
> > [...]
> >
> > > @@ -942,7 +948,7 @@ static int sk_psock_verdict_apply(struct sk_psock *psock, struct sk_buff *skb,
> > >       case __SK_DROP:
> > >       default:
> > >  out_free:
> > > -             kfree_skb(skb);
> > > +             sock_drop(psock->sk, skb);
> >
> > I must have missed this on first review.
> >
> > Why should we mark a packet we intentionally drop as sk_drops? I think
> > we should leave it as just kfree_skb() this way sk_drops is just
> > the error cases and if users want this counter they can always add
> > it to the bpf prog itself.
> 
> This is actually a mixed case of error and non-error drops,
> because bpf_sk_redirect_map() could return SK_DROP
> in error cases. And of course users could want to drop packets
> in whatever cases.
> 
> But if you look at packet filter cases, for example UDP one,
> it increases drop counters too when user-defined rules drop
> them:
> 
> 2182         if (sk_filter_trim_cap(sk, skb, sizeof(struct udphdr)))
> 2183                 goto drop;
> 2184
> ...
> 2192 drop:
> 2193         __UDP_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
> 2194         atomic_inc(&sk->sk_drops);
> 2195         kfree_skb(skb);
> 2196         return -1;
> 
> 
> Thanks.

OK same for TCP side for sk_filter_trim_cap() case. Works for me.