Message ID | 20220316063148.700769-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, 29 lines checked |
netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
netdev/source_inline | success | Was 0 now: 0 |
On 3/16/22 12:31 AM, menglong8.dong@gmail.com wrote: > diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c > index 3ee947557b88..9a1ea6c263f8 100644 > --- a/net/ipv4/ping.c > +++ b/net/ipv4/ping.c > @@ -934,16 +934,24 @@ int ping_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int noblock, > } > EXPORT_SYMBOL_GPL(ping_recvmsg); > > -int ping_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) > +static enum skb_drop_reason __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; > + return SKB_NOT_DROPPED_YET; > +} > + > +int ping_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) > +{ > + return __ping_queue_rcv_skb(sk, skb) ?: -1; > } > EXPORT_SYMBOL_GPL(ping_queue_rcv_skb); > This is a generic proto callback and you are now changing its return code in a way that seems to conflict with existing semantics
On Thu, Mar 17, 2022 at 11:56 AM David Ahern <dsahern@kernel.org> wrote: > > On 3/16/22 12:31 AM, menglong8.dong@gmail.com wrote: > > diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c > > index 3ee947557b88..9a1ea6c263f8 100644 > > --- a/net/ipv4/ping.c > > +++ b/net/ipv4/ping.c > > @@ -934,16 +934,24 @@ int ping_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int noblock, > > } > > EXPORT_SYMBOL_GPL(ping_recvmsg); > > > > -int ping_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) > > +static enum skb_drop_reason __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; > > + return SKB_NOT_DROPPED_YET; > > +} > > + > > +int ping_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) > > +{ > > + return __ping_queue_rcv_skb(sk, skb) ?: -1; > > } > > EXPORT_SYMBOL_GPL(ping_queue_rcv_skb); > > > > This is a generic proto callback and you are now changing its return > code in a way that seems to conflict with existing semantics The return value of ping_queue_rcv_skb() seems not changed. In the previous code, -1 is returned on failure and 0 for success. This logic isn't changed, giving __ping_queue_rcv_skb() != 0 means failure and -1 is returned. Isn't it?
On Thu, 2022-03-17 at 13:25 +0800, Menglong Dong wrote: > On Thu, Mar 17, 2022 at 11:56 AM David Ahern <dsahern@kernel.org> wrote: > > > > On 3/16/22 12:31 AM, menglong8.dong@gmail.com wrote: > > > diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c > > > index 3ee947557b88..9a1ea6c263f8 100644 > > > --- a/net/ipv4/ping.c > > > +++ b/net/ipv4/ping.c > > > @@ -934,16 +934,24 @@ int ping_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int noblock, > > > } > > > EXPORT_SYMBOL_GPL(ping_recvmsg); > > > > > > -int ping_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) > > > +static enum skb_drop_reason __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; > > > + return SKB_NOT_DROPPED_YET; > > > +} > > > + > > > +int ping_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) > > > +{ > > > + return __ping_queue_rcv_skb(sk, skb) ?: -1; > > > } > > > EXPORT_SYMBOL_GPL(ping_queue_rcv_skb); > > > > > > > This is a generic proto callback and you are now changing its return > > code in a way that seems to conflict with existing semantics > > The return value of ping_queue_rcv_skb() seems not changed. > In the previous code, -1 is returned on failure and 0 for success. > This logic isn't changed, giving __ping_queue_rcv_skb() != 0 means > failure and -1 is returned. Isn't it? With this patch, on failure __ping_queue_rcv_skb() returns 'reason' (> 0) and ping_queue_rcv_skb() returns the same value. On success __ping_queue_rcv_skb() returns SKB_NOT_DROPPED_YET (==0) and ping_queue_rcv_skb() return -1. You need to preserve the old ping_queue_rcv_skb() return values, under the same circumstances. Thanks, Paolo
On Thu, Mar 17, 2022 at 4:33 PM Paolo Abeni <pabeni@redhat.com> wrote: > > On Thu, 2022-03-17 at 13:25 +0800, Menglong Dong wrote: > > On Thu, Mar 17, 2022 at 11:56 AM David Ahern <dsahern@kernel.org> wrote: > > > > > > On 3/16/22 12:31 AM, menglong8.dong@gmail.com wrote: > > > > diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c > > > > index 3ee947557b88..9a1ea6c263f8 100644 > > > > --- a/net/ipv4/ping.c > > > > +++ b/net/ipv4/ping.c > > > > @@ -934,16 +934,24 @@ int ping_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int noblock, > > > > } > > > > EXPORT_SYMBOL_GPL(ping_recvmsg); > > > > > > > > -int ping_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) > > > > +static enum skb_drop_reason __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; > > > > + return SKB_NOT_DROPPED_YET; > > > > +} > > > > + > > > > +int ping_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) > > > > +{ > > > > + return __ping_queue_rcv_skb(sk, skb) ?: -1; > > > > } > > > > EXPORT_SYMBOL_GPL(ping_queue_rcv_skb); > > > > > > > > > > This is a generic proto callback and you are now changing its return > > > code in a way that seems to conflict with existing semantics > > > > The return value of ping_queue_rcv_skb() seems not changed. > > In the previous code, -1 is returned on failure and 0 for success. > > This logic isn't changed, giving __ping_queue_rcv_skb() != 0 means > > failure and -1 is returned. Isn't it? > > With this patch, on failure __ping_queue_rcv_skb() returns 'reason' (> > 0) and ping_queue_rcv_skb() returns the same value. > > On success __ping_queue_rcv_skb() returns SKB_NOT_DROPPED_YET (==0) and > ping_queue_rcv_skb() return -1. > > You need to preserve the old ping_queue_rcv_skb() return values, under > the same circumstances. Oops...my mistake....:) Thanks for your explanation! Menglong Dong > > Thanks, > > Paolo >
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c index 3ee947557b88..9a1ea6c263f8 100644 --- a/net/ipv4/ping.c +++ b/net/ipv4/ping.c @@ -934,16 +934,24 @@ int ping_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int noblock, } EXPORT_SYMBOL_GPL(ping_recvmsg); -int ping_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) +static enum skb_drop_reason __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; + return SKB_NOT_DROPPED_YET; +} + +int ping_queue_rcv_skb(struct sock *sk, struct sk_buff *skb) +{ + return __ping_queue_rcv_skb(sk, skb) ?: -1; } EXPORT_SYMBOL_GPL(ping_queue_rcv_skb);