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]

Message-ID: <CAM_iQpU0evVG6_M33ZAgirRsTAJzZcMMN-cYfxqHepbC0UN0iQ@mail.gmail.com>
Date:   Sat, 29 May 2021 12:29:05 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     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

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.