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