| Message ID | 20231028171610.28596-1-jhs@mojatatu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 40cb2fdfed342e7e578d551a073687789f698d89 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,1/1] net, sched: Fix SKB_NOT_DROPPED_YET splat under debug config | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/apply | success | Patch already applied to net |
On Sat, Oct 28, 2023 at 01:16:10PM -0400, Jamal Hadi Salim wrote: > Getting the following splat [1] with CONFIG_DEBUG_NET=y and this > reproducer [2]. Problem seems to be that classifiers clear 'struct > tcf_result::drop_reason', thereby triggering the warning in > __kfree_skb_reason() due to reason being 'SKB_NOT_DROPPED_YET' (0). > > Fixed by disambiguating a legit error from a verdict with a bogus drop_reason > > [1] > WARNING: CPU: 0 PID: 181 at net/core/skbuff.c:1082 kfree_skb_reason+0x38/0x130 > Modules linked in: > CPU: 0 PID: 181 Comm: mausezahn Not tainted 6.6.0-rc6-custom-ge43e6d9582e0 #682 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc37 04/01/2014 > RIP: 0010:kfree_skb_reason+0x38/0x130 > [...] > Call Trace: > <IRQ> > __netif_receive_skb_core.constprop.0+0x837/0xdb0 > __netif_receive_skb_one_core+0x3c/0x70 > process_backlog+0x95/0x130 > __napi_poll+0x25/0x1b0 > net_rx_action+0x29b/0x310 > __do_softirq+0xc0/0x29b > do_softirq+0x43/0x60 > </IRQ> > > [2] > > ip link add name veth0 type veth peer name veth1 > ip link set dev veth0 up > ip link set dev veth1 up > tc qdisc add dev veth1 clsact > tc filter add dev veth1 ingress pref 1 proto all flower dst_mac 00:11:22:33:44:55 action drop > mausezahn veth0 -a own -b 00:11:22:33:44:55 -q -c 1 > > Ido reported: > > [...] getting the following splat [1] with CONFIG_DEBUG_NET=y and this > reproducer [2]. Problem seems to be that classifiers clear 'struct > tcf_result::drop_reason', thereby triggering the warning in > __kfree_skb_reason() due to reason being 'SKB_NOT_DROPPED_YET' (0). [...] > > [1] > WARNING: CPU: 0 PID: 181 at net/core/skbuff.c:1082 kfree_skb_reason+0x38/0x130 > Modules linked in: > CPU: 0 PID: 181 Comm: mausezahn Not tainted 6.6.0-rc6-custom-ge43e6d9582e0 #682 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc37 04/01/2014 > RIP: 0010:kfree_skb_reason+0x38/0x130 > [...] > Call Trace: > <IRQ> > __netif_receive_skb_core.constprop.0+0x837/0xdb0 > __netif_receive_skb_one_core+0x3c/0x70 > process_backlog+0x95/0x130 > __napi_poll+0x25/0x1b0 > net_rx_action+0x29b/0x310 > __do_softirq+0xc0/0x29b > do_softirq+0x43/0x60 > </IRQ> > > [2] > #!/bin/bash > > ip link add name veth0 type veth peer name veth1 > ip link set dev veth0 up > ip link set dev veth1 up > tc qdisc add dev veth1 clsact > tc filter add dev veth1 ingress pref 1 proto all flower dst_mac 00:11:22:33:44:55 action drop > mausezahn veth0 -a own -b 00:11:22:33:44:55 -q -c 1 > > What happens is that inside most classifiers the tcf_result is copied over > from a filter template e.g. *res = f->res which then implicitly overrides > the prior SKB_DROP_REASON_TC_{INGRESS,EGRESS} default drop code which was > set via sch_handle_{ingress,egress}() for kfree_skb_reason(). > > Commit text above copied verbatim from Daniel. The general idea of the patch > is not very different from what Ido originally posted but instead done at the > cls_api codepath. > > Fixes: 54a59aed395c ("net, sched: Make tc-related drop reason more flexible") > Reported-by: Ido Schimmel <idosch@idosch.org> > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> > Link: https://lore.kernel.org/netdev/ZTjY959R+AFXf3Xy@shredder Hi Jamal, FWIIW, I think it would be nicer to fix this the classifiers so they don't do this, which by my reading is what Daniel's patch did. But, I don't feel strongly about this and I do tend to think the approach taken in this patch is a nice clean fix for net. Reviewed-by: Simon Horman <horms@kernel.org> BTW, this patch is marked as Not Applicable in patchwork. I am unsure why.
On Sat, Nov 4, 2023 at 9:11 AM Simon Horman <horms@kernel.org> wrote: > > On Sat, Oct 28, 2023 at 01:16:10PM -0400, Jamal Hadi Salim wrote: > > Getting the following splat [1] with CONFIG_DEBUG_NET=y and this > > reproducer [2]. Problem seems to be that classifiers clear 'struct > > tcf_result::drop_reason', thereby triggering the warning in > > __kfree_skb_reason() due to reason being 'SKB_NOT_DROPPED_YET' (0). > > > > Fixed by disambiguating a legit error from a verdict with a bogus drop_reason > > > > [1] > > WARNING: CPU: 0 PID: 181 at net/core/skbuff.c:1082 kfree_skb_reason+0x38/0x130 > > Modules linked in: > > CPU: 0 PID: 181 Comm: mausezahn Not tainted 6.6.0-rc6-custom-ge43e6d9582e0 #682 > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc37 04/01/2014 > > RIP: 0010:kfree_skb_reason+0x38/0x130 > > [...] > > Call Trace: > > <IRQ> > > __netif_receive_skb_core.constprop.0+0x837/0xdb0 > > __netif_receive_skb_one_core+0x3c/0x70 > > process_backlog+0x95/0x130 > > __napi_poll+0x25/0x1b0 > > net_rx_action+0x29b/0x310 > > __do_softirq+0xc0/0x29b > > do_softirq+0x43/0x60 > > </IRQ> > > > > [2] > > > > ip link add name veth0 type veth peer name veth1 > > ip link set dev veth0 up > > ip link set dev veth1 up > > tc qdisc add dev veth1 clsact > > tc filter add dev veth1 ingress pref 1 proto all flower dst_mac 00:11:22:33:44:55 action drop > > mausezahn veth0 -a own -b 00:11:22:33:44:55 -q -c 1 > > > > Ido reported: > > > > [...] getting the following splat [1] with CONFIG_DEBUG_NET=y and this > > reproducer [2]. Problem seems to be that classifiers clear 'struct > > tcf_result::drop_reason', thereby triggering the warning in > > __kfree_skb_reason() due to reason being 'SKB_NOT_DROPPED_YET' (0). [...] > > > > [1] > > WARNING: CPU: 0 PID: 181 at net/core/skbuff.c:1082 kfree_skb_reason+0x38/0x130 > > Modules linked in: > > CPU: 0 PID: 181 Comm: mausezahn Not tainted 6.6.0-rc6-custom-ge43e6d9582e0 #682 > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc37 04/01/2014 > > RIP: 0010:kfree_skb_reason+0x38/0x130 > > [...] > > Call Trace: > > <IRQ> > > __netif_receive_skb_core.constprop.0+0x837/0xdb0 > > __netif_receive_skb_one_core+0x3c/0x70 > > process_backlog+0x95/0x130 > > __napi_poll+0x25/0x1b0 > > net_rx_action+0x29b/0x310 > > __do_softirq+0xc0/0x29b > > do_softirq+0x43/0x60 > > </IRQ> > > > > [2] > > #!/bin/bash > > > > ip link add name veth0 type veth peer name veth1 > > ip link set dev veth0 up > > ip link set dev veth1 up > > tc qdisc add dev veth1 clsact > > tc filter add dev veth1 ingress pref 1 proto all flower dst_mac 00:11:22:33:44:55 action drop > > mausezahn veth0 -a own -b 00:11:22:33:44:55 -q -c 1 > > > > What happens is that inside most classifiers the tcf_result is copied over > > from a filter template e.g. *res = f->res which then implicitly overrides > > the prior SKB_DROP_REASON_TC_{INGRESS,EGRESS} default drop code which was > > set via sch_handle_{ingress,egress}() for kfree_skb_reason(). > > > > Commit text above copied verbatim from Daniel. The general idea of the patch > > is not very different from what Ido originally posted but instead done at the > > cls_api codepath. > > > > Fixes: 54a59aed395c ("net, sched: Make tc-related drop reason more flexible") > > Reported-by: Ido Schimmel <idosch@idosch.org> > > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> > > Link: https://lore.kernel.org/netdev/ZTjY959R+AFXf3Xy@shredder > > Hi Jamal, > > FWIIW, I think it would be nicer to fix this the classifiers so they don't > do this, which by my reading is what Daniel's patch did. > I dont believe it was nicer tbh. In any case we are going to send cleanups to do this with skb cb. > But, I don't feel strongly about this and I do tend to think the > approach taken in this patch is a nice clean fix for net. > > Reviewed-by: Simon Horman <horms@kernel.org> > > BTW, this patch is marked as Not Applicable in patchwork. > I am unsure why. Weird. cheers, jamal
On Sat, Nov 04, 2023 at 11:46:22AM -0400, Jamal Hadi Salim wrote: > On Sat, Nov 4, 2023 at 9:11 AM Simon Horman <horms@kernel.org> wrote: > > > > On Sat, Oct 28, 2023 at 01:16:10PM -0400, Jamal Hadi Salim wrote: > > > Getting the following splat [1] with CONFIG_DEBUG_NET=y and this > > > reproducer [2]. Problem seems to be that classifiers clear 'struct > > > tcf_result::drop_reason', thereby triggering the warning in > > > __kfree_skb_reason() due to reason being 'SKB_NOT_DROPPED_YET' (0). > > > > > > Fixed by disambiguating a legit error from a verdict with a bogus drop_reason > > > > > > [1] > > > WARNING: CPU: 0 PID: 181 at net/core/skbuff.c:1082 kfree_skb_reason+0x38/0x130 > > > Modules linked in: > > > CPU: 0 PID: 181 Comm: mausezahn Not tainted 6.6.0-rc6-custom-ge43e6d9582e0 #682 > > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc37 04/01/2014 > > > RIP: 0010:kfree_skb_reason+0x38/0x130 > > > [...] > > > Call Trace: > > > <IRQ> > > > __netif_receive_skb_core.constprop.0+0x837/0xdb0 > > > __netif_receive_skb_one_core+0x3c/0x70 > > > process_backlog+0x95/0x130 > > > __napi_poll+0x25/0x1b0 > > > net_rx_action+0x29b/0x310 > > > __do_softirq+0xc0/0x29b > > > do_softirq+0x43/0x60 > > > </IRQ> > > > > > > [2] > > > > > > ip link add name veth0 type veth peer name veth1 > > > ip link set dev veth0 up > > > ip link set dev veth1 up > > > tc qdisc add dev veth1 clsact > > > tc filter add dev veth1 ingress pref 1 proto all flower dst_mac 00:11:22:33:44:55 action drop > > > mausezahn veth0 -a own -b 00:11:22:33:44:55 -q -c 1 > > > > > > Ido reported: > > > > > > [...] getting the following splat [1] with CONFIG_DEBUG_NET=y and this > > > reproducer [2]. Problem seems to be that classifiers clear 'struct > > > tcf_result::drop_reason', thereby triggering the warning in > > > __kfree_skb_reason() due to reason being 'SKB_NOT_DROPPED_YET' (0). [...] > > > > > > [1] > > > WARNING: CPU: 0 PID: 181 at net/core/skbuff.c:1082 kfree_skb_reason+0x38/0x130 > > > Modules linked in: > > > CPU: 0 PID: 181 Comm: mausezahn Not tainted 6.6.0-rc6-custom-ge43e6d9582e0 #682 > > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc37 04/01/2014 > > > RIP: 0010:kfree_skb_reason+0x38/0x130 > > > [...] > > > Call Trace: > > > <IRQ> > > > __netif_receive_skb_core.constprop.0+0x837/0xdb0 > > > __netif_receive_skb_one_core+0x3c/0x70 > > > process_backlog+0x95/0x130 > > > __napi_poll+0x25/0x1b0 > > > net_rx_action+0x29b/0x310 > > > __do_softirq+0xc0/0x29b > > > do_softirq+0x43/0x60 > > > </IRQ> > > > > > > [2] > > > #!/bin/bash > > > > > > ip link add name veth0 type veth peer name veth1 > > > ip link set dev veth0 up > > > ip link set dev veth1 up > > > tc qdisc add dev veth1 clsact > > > tc filter add dev veth1 ingress pref 1 proto all flower dst_mac 00:11:22:33:44:55 action drop > > > mausezahn veth0 -a own -b 00:11:22:33:44:55 -q -c 1 > > > > > > What happens is that inside most classifiers the tcf_result is copied over > > > from a filter template e.g. *res = f->res which then implicitly overrides > > > the prior SKB_DROP_REASON_TC_{INGRESS,EGRESS} default drop code which was > > > set via sch_handle_{ingress,egress}() for kfree_skb_reason(). > > > > > > Commit text above copied verbatim from Daniel. The general idea of the patch > > > is not very different from what Ido originally posted but instead done at the > > > cls_api codepath. > > > > > > Fixes: 54a59aed395c ("net, sched: Make tc-related drop reason more flexible") > > > Reported-by: Ido Schimmel <idosch@idosch.org> > > > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> > > > Link: https://lore.kernel.org/netdev/ZTjY959R+AFXf3Xy@shredder > > > > Hi Jamal, > > > > FWIIW, I think it would be nicer to fix this the classifiers so they don't > > do this, which by my reading is what Daniel's patch did. > > > > I dont believe it was nicer tbh. I thought you might say that :) > In any case we are going to send cleanups to do this with skb cb. Thanks, I'll look out for the new patch. > > But, I don't feel strongly about this and I do tend to think the > > approach taken in this patch is a nice clean fix for net. > > > > Reviewed-by: Simon Horman <horms@kernel.org> > > > > BTW, this patch is marked as Not Applicable in patchwork. > > I am unsure why. > > Weird.
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Sat, 28 Oct 2023 13:16:10 -0400 you wrote: > Getting the following splat [1] with CONFIG_DEBUG_NET=y and this > reproducer [2]. Problem seems to be that classifiers clear 'struct > tcf_result::drop_reason', thereby triggering the warning in > __kfree_skb_reason() due to reason being 'SKB_NOT_DROPPED_YET' (0). > > Fixed by disambiguating a legit error from a verdict with a bogus drop_reason > > [...] Here is the summary with links: - [net,1/1] net, sched: Fix SKB_NOT_DROPPED_YET splat under debug config https://git.kernel.org/netdev/net/c/40cb2fdfed34 You are awesome, thank you!
diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 9d3f26bf0440..c39252d61ebb 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -1098,7 +1098,7 @@ int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions, } } else if (TC_ACT_EXT_CMP(ret, TC_ACT_GOTO_CHAIN)) { if (unlikely(!rcu_access_pointer(a->goto_chain))) { - net_warn_ratelimited("can't go to NULL chain!\n"); + tcf_set_drop_reason(res, SKB_DROP_REASON_TC_ERROR); return TC_ACT_SHOT; } tcf_action_goto_chain_exec(a, res); diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index 1daeb2182b70..1976bd163986 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -1658,6 +1658,7 @@ static inline int __tcf_classify(struct sk_buff *skb, int act_index, u32 *last_executed_chain) { + u32 orig_reason = res->drop_reason; #ifdef CONFIG_NET_CLS_ACT const int max_reclassify_loop = 16; const struct tcf_proto *first_tp; @@ -1712,8 +1713,14 @@ static inline int __tcf_classify(struct sk_buff *skb, goto reset; } #endif - if (err >= 0) + if (err >= 0) { + /* Policy drop or drop reason is over-written by + * classifiers with a bogus value(0) */ + if (err == TC_ACT_SHOT && + res->drop_reason == SKB_NOT_DROPPED_YET) + tcf_set_drop_reason(res, orig_reason); return err; + } } if (unlikely(n)) {
Getting the following splat [1] with CONFIG_DEBUG_NET=y and this reproducer [2]. Problem seems to be that classifiers clear 'struct tcf_result::drop_reason', thereby triggering the warning in __kfree_skb_reason() due to reason being 'SKB_NOT_DROPPED_YET' (0). Fixed by disambiguating a legit error from a verdict with a bogus drop_reason [1] WARNING: CPU: 0 PID: 181 at net/core/skbuff.c:1082 kfree_skb_reason+0x38/0x130 Modules linked in: CPU: 0 PID: 181 Comm: mausezahn Not tainted 6.6.0-rc6-custom-ge43e6d9582e0 #682 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc37 04/01/2014 RIP: 0010:kfree_skb_reason+0x38/0x130 [...] Call Trace: <IRQ> __netif_receive_skb_core.constprop.0+0x837/0xdb0 __netif_receive_skb_one_core+0x3c/0x70 process_backlog+0x95/0x130 __napi_poll+0x25/0x1b0 net_rx_action+0x29b/0x310 __do_softirq+0xc0/0x29b do_softirq+0x43/0x60 </IRQ> [2] ip link add name veth0 type veth peer name veth1 ip link set dev veth0 up ip link set dev veth1 up tc qdisc add dev veth1 clsact tc filter add dev veth1 ingress pref 1 proto all flower dst_mac 00:11:22:33:44:55 action drop mausezahn veth0 -a own -b 00:11:22:33:44:55 -q -c 1 Ido reported: [...] getting the following splat [1] with CONFIG_DEBUG_NET=y and this reproducer [2]. Problem seems to be that classifiers clear 'struct tcf_result::drop_reason', thereby triggering the warning in __kfree_skb_reason() due to reason being 'SKB_NOT_DROPPED_YET' (0). [...] [1] WARNING: CPU: 0 PID: 181 at net/core/skbuff.c:1082 kfree_skb_reason+0x38/0x130 Modules linked in: CPU: 0 PID: 181 Comm: mausezahn Not tainted 6.6.0-rc6-custom-ge43e6d9582e0 #682 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc37 04/01/2014 RIP: 0010:kfree_skb_reason+0x38/0x130 [...] Call Trace: <IRQ> __netif_receive_skb_core.constprop.0+0x837/0xdb0 __netif_receive_skb_one_core+0x3c/0x70 process_backlog+0x95/0x130 __napi_poll+0x25/0x1b0 net_rx_action+0x29b/0x310 __do_softirq+0xc0/0x29b do_softirq+0x43/0x60 </IRQ> [2] #!/bin/bash ip link add name veth0 type veth peer name veth1 ip link set dev veth0 up ip link set dev veth1 up tc qdisc add dev veth1 clsact tc filter add dev veth1 ingress pref 1 proto all flower dst_mac 00:11:22:33:44:55 action drop mausezahn veth0 -a own -b 00:11:22:33:44:55 -q -c 1 What happens is that inside most classifiers the tcf_result is copied over from a filter template e.g. *res = f->res which then implicitly overrides the prior SKB_DROP_REASON_TC_{INGRESS,EGRESS} default drop code which was set via sch_handle_{ingress,egress}() for kfree_skb_reason(). Commit text above copied verbatim from Daniel. The general idea of the patch is not very different from what Ido originally posted but instead done at the cls_api codepath. Fixes: 54a59aed395c ("net, sched: Make tc-related drop reason more flexible") Reported-by: Ido Schimmel <idosch@idosch.org> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> Link: https://lore.kernel.org/netdev/ZTjY959R+AFXf3Xy@shredder --- net/sched/act_api.c | 2 +- net/sched/cls_api.c | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-)