Message ID | 20220314113225.151959-3-imagedong@tencent.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | net: icmp: add skb drop reasons to icmp | expand |
Context | Check | Description |
---|---|---|
netdev/tree_selection | success | Clearly marked for net-next |
netdev/fixes_present | success | Fixes tag not required for -next series |
netdev/subject_prefix | success | Link |
netdev/cover_letter | success | Series has a cover letter |
netdev/patch_count | success | Link |
netdev/header_inline | success | No static functions without inline keyword in header files |
netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
netdev/build_clang | success | Errors and warnings before: 18 this patch: 18 |
netdev/module_param | success | Was 0 now: 0 |
netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
netdev/verify_fixes | success | No Fixes tag |
netdev/build_allmodconfig_warn | success | Errors and warnings before: 7 this patch: 7 |
netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
netdev/source_inline | success | Was 0 now: 0 |
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
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
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; 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; } return 0; }