Messages in this thread | | | Subject | Re: [PATCH net-next 2/3] net: icmp: add skb drop reasons to ping_queue_rcv_skb() | From | Paolo Abeni <> | Date | Tue, 15 Mar 2022 13:49:29 +0100 |
| |
Hello,
On Mon, 2022-03-14 at 19:32 +0800, menglong8.dong@gmail.com wrote: > From: Menglong Dong <imagedong@tencent.com> > > In order to get the reasons of skb drops, replace sock_queue_rcv_skb() > used in ping_queue_rcv_skb() with sock_queue_rcv_skb_reason(). > Meanwhile, use kfree_skb_reason() instead of kfree_skb(). > > As we can see in ping_rcv(), 'skb' will be freed if '-1' is returned > by ping_queue_rcv_skb(). In order to get the drop reason of 'skb', > make ping_queue_rcv_skb() return the drop reason. > > As ping_queue_rcv_skb() is used as 'ping_prot.backlog_rcv()', we can't > change its return type. (Seems ping_prot.backlog_rcv() is not used?) > Therefore, make it return 'drop_reason * -1' to keep the origin logic. > > Signed-off-by: Menglong Dong <imagedong@tencent.com> > --- > net/ipv4/ping.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c > index 3ee947557b88..cd4eb211431a 100644 > --- a/net/ipv4/ping.c > +++ b/net/ipv4/ping.c > @@ -936,12 +936,13 @@ EXPORT_SYMBOL_GPL(ping_recvmsg); > > int ping_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) > { > + enum skb_drop_reason reason;
Please insert an empty line between variable declaration and code.
> pr_debug("ping_queue_rcv_skb(sk=%p,sk->num=%d,skb=%p)\n", > inet_sk(sk), inet_sk(sk)->inet_num, skb); > - if (sock_queue_rcv_skb(sk, skb) < 0) { > - kfree_skb(skb); > + if (sock_queue_rcv_skb_reason(sk, skb, &reason) < 0) { > + kfree_skb_reason(skb, reason); > pr_debug("ping_queue_rcv_skb -> failed\n"); > - return -1; > + return -reason;
This changes the return value for the release callback. Such callback has a long and non trivial call chain via sk_backlog_rcv.
It *should* be safe, but have you considered factoring out an __ping_queue_rcv_skb() variant returning the full drop reason, use it in the next patch and build ping_queue_rcv_skb() on top of such helper to that backlog_rcv() return code will not change?
The above should additionally avoid the IMHO not so nice:
reason = ping_queue_rcv_skb(sk, skb2) * -1;
in the next patch - it will become:
reason = __ping_queue_rcv_skb(sk, skb2);
Thanks!
Paolo
| |