On Thu, May 27, 2021 at 10:27 PM John Fastabend
[off-list ref] wrote:

Cong Wang wrote:

quoted

From: Cong Wang <redacted>

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 <redacted>
Signed-off-by: Cong Wang <redacted>
---
 net/core/skmsg.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

[...]

quoted

@@ -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.