Messages in this thread | | | From | Menglong Dong <> | Date | Wed, 16 Mar 2022 09:34:09 +0800 | Subject | Re: [PATCH net-next 2/3] net: icmp: add skb drop reasons to ping_queue_rcv_skb() |
| |
On Tue, Mar 15, 2022 at 8:49 PM Paolo Abeni <pabeni@redhat.com> wrote: > > 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.
Ok
> > > 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); >
Good idea, thanks!
Menglong Dong
| |