Re: [Patch bpf v3 8/8] skmsg: increase sk->sk_drops when dropping packets

[John Fastabend <john.fastabend@gmail.com>]

Cong Wang wrote:
> On Thu, May 27, 2021 at 10:27 PM John Fastabend
> <john.fastabend@gmail.com> wrote:
> >
> > Cong Wang wrote:
> > > From: Cong Wang <cong.wang@bytedance.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@gmail.com>
> > > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > > Cc: Jakub Sitnicki <jakub@cloudflare.com>
> > > Cc: Lorenz Bauer <lmb@cloudflare.com>
> > > Signed-off-by: Cong Wang <cong.wang@bytedance.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.