| Message ID | ZU3EZKQ3dyLE6T8z@debian.debian (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] packet: add a generic drop reason for receive | expand |
On Fri, Nov 10, 2023 at 6:49 AM Yan Zhai <yan@cloudflare.com> wrote: > > Commit da37845fdce2 ("packet: uses kfree_skb() for errors.") switches > from consume_skb to kfree_skb to improve error handling. However, this > could bring a lot of noises when we monitor real packet drops in > kfree_skb[1], because in tpacket_rcv or packet_rcv only packet clones > can be freed, not actual packets. > > Adding a generic drop reason to allow distinguish these "clone drops". > > [1]: https://lore.kernel.org/netdev/CABWYdi00L+O30Q=Zah28QwZ_5RU-xcxLFUK2Zj08A8MrLk9jzg@mail.gmail.com/ > Fixes: da37845fdce2 ("packet: uses kfree_skb() for errors.") > Signed-off-by: Yan Zhai <yan@cloudflare.com> > --- > include/net/dropreason-core.h | 6 ++++++ > net/packet/af_packet.c | 16 +++++++++++++--- > 2 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h > index 845dce805de7..6ff543fe8a8b 100644 > --- a/include/net/dropreason-core.h > +++ b/include/net/dropreason-core.h > @@ -81,6 +81,7 @@ > FN(IPV6_NDISC_NS_OTHERHOST) \ > FN(QUEUE_PURGE) \ > FN(TC_ERROR) \ > + FN(PACKET_SOCK_ERROR) \ > FNe(MAX) > > /** > @@ -348,6 +349,11 @@ enum skb_drop_reason { > SKB_DROP_REASON_QUEUE_PURGE, > /** @SKB_DROP_REASON_TC_ERROR: generic internal tc error. */ > SKB_DROP_REASON_TC_ERROR, > + /** > + * @SKB_DROP_REASON_PACKET_SOCK_ERROR: generic packet socket errors > + * after its filter matches an incoming packet. > + */ > + SKB_DROP_REASON_PACKET_SOCK_ERROR, > /** > * @SKB_DROP_REASON_MAX: the maximum of core drop reasons, which > * shouldn't be used as a real 'reason' - only for tracing code gen > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index a84e00b5904b..94b8a9d8e038 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -2128,6 +2128,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, > int skb_len = skb->len; > unsigned int snaplen, res; > bool is_drop_n_account = false; > + enum skb_drop_reason drop_reason = SKB_DROP_REASON_NOT_SPECIFIED; > > if (skb->pkt_type == PACKET_LOOPBACK) > goto drop; > @@ -2161,6 +2162,10 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, > res = run_filter(skb, sk, snaplen); > if (!res) > goto drop_n_restore; > + > + /* skb will only be "consumed" not "dropped" before this */ > + drop_reason = SKB_DROP_REASON_PACKET_SOCK_ERROR; > + > if (snaplen > res) > snaplen = res; > > @@ -2230,7 +2235,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, > if (!is_drop_n_account) > consume_skb(skb); > else > - kfree_skb(skb); > + kfree_skb_reason(skb, drop_reason); > return 0; 1) Note that net-next is currently closed. 2) Now we have 0e84afe8ebfb ("net: dropreason: add SKB_CONSUMED reason") it is time we replace the various constructs which do not help readability: if (something) consume_skb(skb); else kfree_skb_reason(skb, drop_reason); By: kfree_skb_reason(skb, drop_reason); (By using drop_reason == SKB_CONSUMED when appropriate)
Fri, Nov 10, 2023 at 10:30:49AM CET, edumazet@google.com wrote: >On Fri, Nov 10, 2023 at 6:49 AM Yan Zhai <yan@cloudflare.com> wrote: [..] >1) Note that net-next is currently closed. I wonder, can't some bot be easily set up to warn about this automatically?
On Fri, Nov 10, 2023 at 4:13 AM Jiri Pirko <jiri@resnulli.us> wrote: > > Fri, Nov 10, 2023 at 10:30:49AM CET, edumazet@google.com wrote: > >On Fri, Nov 10, 2023 at 6:49 AM Yan Zhai <yan@cloudflare.com> wrote: > > [..] > > >1) Note that net-next is currently closed. > > I wonder, can't some bot be easily set up to warn about > this automatically? > It's funny that I actually got notified about an individual recipient mailbox being full.. Side channel :)
On Fri, Nov 10, 2023 at 3:31 AM Eric Dumazet <edumazet@google.com> wrote: > it is time we replace the various constructs which do not help readability: > > if (something) > consume_skb(skb); > else > kfree_skb_reason(skb, drop_reason); > > By: > > kfree_skb_reason(skb, drop_reason); > > (By using drop_reason == SKB_CONSUMED when appropriate) Will send a V2 when net-next reopens
diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h index 845dce805de7..6ff543fe8a8b 100644 --- a/include/net/dropreason-core.h +++ b/include/net/dropreason-core.h @@ -81,6 +81,7 @@ FN(IPV6_NDISC_NS_OTHERHOST) \ FN(QUEUE_PURGE) \ FN(TC_ERROR) \ + FN(PACKET_SOCK_ERROR) \ FNe(MAX) /** @@ -348,6 +349,11 @@ enum skb_drop_reason { SKB_DROP_REASON_QUEUE_PURGE, /** @SKB_DROP_REASON_TC_ERROR: generic internal tc error. */ SKB_DROP_REASON_TC_ERROR, + /** + * @SKB_DROP_REASON_PACKET_SOCK_ERROR: generic packet socket errors + * after its filter matches an incoming packet. + */ + SKB_DROP_REASON_PACKET_SOCK_ERROR, /** * @SKB_DROP_REASON_MAX: the maximum of core drop reasons, which * shouldn't be used as a real 'reason' - only for tracing code gen diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index a84e00b5904b..94b8a9d8e038 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -2128,6 +2128,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, int skb_len = skb->len; unsigned int snaplen, res; bool is_drop_n_account = false; + enum skb_drop_reason drop_reason = SKB_DROP_REASON_NOT_SPECIFIED; if (skb->pkt_type == PACKET_LOOPBACK) goto drop; @@ -2161,6 +2162,10 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, res = run_filter(skb, sk, snaplen); if (!res) goto drop_n_restore; + + /* skb will only be "consumed" not "dropped" before this */ + drop_reason = SKB_DROP_REASON_PACKET_SOCK_ERROR; + if (snaplen > res) snaplen = res; @@ -2230,7 +2235,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, if (!is_drop_n_account) consume_skb(skb); else - kfree_skb(skb); + kfree_skb_reason(skb, drop_reason); return 0; } @@ -2253,6 +2258,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, bool is_drop_n_account = false; unsigned int slot_id = 0; int vnet_hdr_sz = 0; + enum skb_drop_reason drop_reason = SKB_DROP_REASON_NOT_SPECIFIED; /* struct tpacket{2,3}_hdr is aligned to a multiple of TPACKET_ALIGNMENT. * We may add members to them until current aligned size without forcing @@ -2355,6 +2361,10 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, vnet_hdr_sz = 0; } } + + /* skb will only be "consumed" not "dropped" before this */ + drop_reason = SKB_DROP_REASON_PACKET_SOCK_ERROR; + spin_lock(&sk->sk_receive_queue.lock); h.raw = packet_current_rx_frame(po, skb, TP_STATUS_KERNEL, (macoff+snaplen)); @@ -2501,7 +2511,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, if (!is_drop_n_account) consume_skb(skb); else - kfree_skb(skb); + kfree_skb_reason(skb, drop_reason); return 0; drop_n_account: @@ -2510,7 +2520,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, is_drop_n_account = true; sk->sk_data_ready(sk); - kfree_skb(copy_skb); + kfree_skb_reason(copy_skb, drop_reason); goto drop_n_restore; }
Commit da37845fdce2 ("packet: uses kfree_skb() for errors.") switches from consume_skb to kfree_skb to improve error handling. However, this could bring a lot of noises when we monitor real packet drops in kfree_skb[1], because in tpacket_rcv or packet_rcv only packet clones can be freed, not actual packets. Adding a generic drop reason to allow distinguish these "clone drops". [1]: https://lore.kernel.org/netdev/CABWYdi00L+O30Q=Zah28QwZ_5RU-xcxLFUK2Zj08A8MrLk9jzg@mail.gmail.com/ Fixes: da37845fdce2 ("packet: uses kfree_skb() for errors.") Signed-off-by: Yan Zhai <yan@cloudflare.com> --- include/net/dropreason-core.h | 6 ++++++ net/packet/af_packet.c | 16 +++++++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-)