| Message ID | 20230418165426.1869051-1-mbizon@freebox.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: dst: fix missing initialization of rt_uncached | expand |
On Tue, Apr 18, 2023 at 6:55 PM Maxime Bizon <mbizon@freebox.fr> wrote: > > xfrm_alloc_dst() followed by xfrm4_dst_destroy(), without a > xfrm4_fill_dst() call in between, causes the following BUG: Reviewed-by: Eric Dumazet <edumazet@google.com> Thanks.
On Tue, Apr 18, 2023 at 06:54:26PM +0200, Maxime Bizon wrote: > xfrm_alloc_dst() followed by xfrm4_dst_destroy(), without a > xfrm4_fill_dst() call in between, causes the following BUG: > > BUG: spinlock bad magic on CPU#0, fbxhostapd/732 > lock: 0x890b7668, .magic: 890b7668, .owner: <none>/-1, .owner_cpu: 0 > CPU: 0 PID: 732 Comm: fbxhostapd Not tainted 6.3.0-rc6-next-20230414-00613-ge8de66369925-dirty #9 > Hardware name: Marvell Kirkwood (Flattened Device Tree) > unwind_backtrace from show_stack+0x10/0x14 > show_stack from dump_stack_lvl+0x28/0x30 > dump_stack_lvl from do_raw_spin_lock+0x20/0x80 > do_raw_spin_lock from rt_del_uncached_list+0x30/0x64 > rt_del_uncached_list from xfrm4_dst_destroy+0x3c/0xbc > xfrm4_dst_destroy from dst_destroy+0x5c/0xb0 > dst_destroy from rcu_process_callbacks+0xc4/0xec > rcu_process_callbacks from __do_softirq+0xb4/0x22c > __do_softirq from call_with_stack+0x1c/0x24 > call_with_stack from do_softirq+0x60/0x6c > do_softirq from __local_bh_enable_ip+0xa0/0xcc > > Patch "net: dst: Prevent false sharing vs. dst_entry:: __refcnt" moved > rt_uncached and rt_uncached_list fields from rtable struct to dst > struct, so they are more zeroed by memset_after(xdst, 0, u.dst) in > xfrm_alloc_dst(). > > Note that rt_uncached (list_head) was never properly initialized at > alloc time, but xfrm[46]_dst_destroy() is written in such a way that > it was not an issue thanks to the memset: > > if (xdst->u.rt.dst.rt_uncached_list) > rt_del_uncached_list(&xdst->u.rt); > > The route code does it the other way around: rt_uncached_list is > assumed to be valid IIF rt_uncached list_head is not empty: > > void rt_del_uncached_list(struct rtable *rt) > { > if (!list_empty(&rt->dst.rt_uncached)) { > struct uncached_list *ul = rt->dst.rt_uncached_list; > > spin_lock_bh(&ul->lock); > list_del_init(&rt->dst.rt_uncached); > spin_unlock_bh(&ul->lock); > } > } > > This patch adds mandatory rt_uncached list_head initialization in > generic dst_init(), and adapt xfrm[46]_dst_destroy logic to match the > rest of the code. > > Fixes: d288a162dd1c ("net: dst: Prevent false sharing vs. dst_entry:: __refcnt") > Signed-off-by: Maxime Bizon <mbizon@freebox.fr> > --- > net/core/dst.c | 1 + > net/ipv4/xfrm4_policy.c | 4 +--- > net/ipv6/route.c | 1 - > net/ipv6/xfrm6_policy.c | 4 +--- > 4 files changed, 3 insertions(+), 7 deletions(-) It should go to net. Right now -rc7 is broken. Also the change is not complete, you need to delete INIT_LIST_HEAD(..rt_uncached) from rt_dst_alloc and rt_dst_clone too. Thanks
On Wed, Apr 19, 2023 at 11:58:02AM +0300, Leon Romanovsky wrote: > On Tue, Apr 18, 2023 at 06:54:26PM +0200, Maxime Bizon wrote: > > xfrm_alloc_dst() followed by xfrm4_dst_destroy(), without a > > xfrm4_fill_dst() call in between, causes the following BUG: > > > > BUG: spinlock bad magic on CPU#0, fbxhostapd/732 > > lock: 0x890b7668, .magic: 890b7668, .owner: <none>/-1, .owner_cpu: 0 > > CPU: 0 PID: 732 Comm: fbxhostapd Not tainted 6.3.0-rc6-next-20230414-00613-ge8de66369925-dirty #9 > > Hardware name: Marvell Kirkwood (Flattened Device Tree) > > unwind_backtrace from show_stack+0x10/0x14 > > show_stack from dump_stack_lvl+0x28/0x30 > > dump_stack_lvl from do_raw_spin_lock+0x20/0x80 > > do_raw_spin_lock from rt_del_uncached_list+0x30/0x64 > > rt_del_uncached_list from xfrm4_dst_destroy+0x3c/0xbc > > xfrm4_dst_destroy from dst_destroy+0x5c/0xb0 > > dst_destroy from rcu_process_callbacks+0xc4/0xec > > rcu_process_callbacks from __do_softirq+0xb4/0x22c > > __do_softirq from call_with_stack+0x1c/0x24 > > call_with_stack from do_softirq+0x60/0x6c > > do_softirq from __local_bh_enable_ip+0xa0/0xcc > > > > Patch "net: dst: Prevent false sharing vs. dst_entry:: __refcnt" moved > > rt_uncached and rt_uncached_list fields from rtable struct to dst > > struct, so they are more zeroed by memset_after(xdst, 0, u.dst) in > > xfrm_alloc_dst(). > > > > Note that rt_uncached (list_head) was never properly initialized at > > alloc time, but xfrm[46]_dst_destroy() is written in such a way that > > it was not an issue thanks to the memset: > > > > if (xdst->u.rt.dst.rt_uncached_list) > > rt_del_uncached_list(&xdst->u.rt); > > > > The route code does it the other way around: rt_uncached_list is > > assumed to be valid IIF rt_uncached list_head is not empty: > > > > void rt_del_uncached_list(struct rtable *rt) > > { > > if (!list_empty(&rt->dst.rt_uncached)) { > > struct uncached_list *ul = rt->dst.rt_uncached_list; > > > > spin_lock_bh(&ul->lock); > > list_del_init(&rt->dst.rt_uncached); > > spin_unlock_bh(&ul->lock); > > } > > } > > > > This patch adds mandatory rt_uncached list_head initialization in > > generic dst_init(), and adapt xfrm[46]_dst_destroy logic to match the > > rest of the code. > > > > Fixes: d288a162dd1c ("net: dst: Prevent false sharing vs. dst_entry:: __refcnt") > > Signed-off-by: Maxime Bizon <mbizon@freebox.fr> > > --- > > net/core/dst.c | 1 + > > net/ipv4/xfrm4_policy.c | 4 +--- > > net/ipv6/route.c | 1 - > > net/ipv6/xfrm6_policy.c | 4 +--- > > 4 files changed, 3 insertions(+), 7 deletions(-) > > It should go to net. Right now -rc7 is broken. > > Also the change is not complete, you need to delete INIT_LIST_HEAD(..rt_uncached) > from rt_dst_alloc and rt_dst_clone too. It will be nice to give a credit to kbuild. https://lore.kernel.org/all/202304162125.18b7bcdd-oliver.sang@intel.com Thanks > > Thanks
On Wed, Apr 19, 2023 at 11:03 AM Leon Romanovsky <leon@kernel.org> wrote: > > On Wed, Apr 19, 2023 at 11:58:02AM +0300, Leon Romanovsky wrote: > > On Tue, Apr 18, 2023 at 06:54:26PM +0200, Maxime Bizon wrote: > > > xfrm_alloc_dst() followed by xfrm4_dst_destroy(), without a > > > xfrm4_fill_dst() call in between, causes the following BUG: > > > > > > BUG: spinlock bad magic on CPU#0, fbxhostapd/732 > > > lock: 0x890b7668, .magic: 890b7668, .owner: <none>/-1, .owner_cpu: 0 > > > CPU: 0 PID: 732 Comm: fbxhostapd Not tainted 6.3.0-rc6-next-20230414-00613-ge8de66369925-dirty #9 > > > Hardware name: Marvell Kirkwood (Flattened Device Tree) > > > unwind_backtrace from show_stack+0x10/0x14 > > > show_stack from dump_stack_lvl+0x28/0x30 > > > dump_stack_lvl from do_raw_spin_lock+0x20/0x80 > > > do_raw_spin_lock from rt_del_uncached_list+0x30/0x64 > > > rt_del_uncached_list from xfrm4_dst_destroy+0x3c/0xbc > > > xfrm4_dst_destroy from dst_destroy+0x5c/0xb0 > > > dst_destroy from rcu_process_callbacks+0xc4/0xec > > > rcu_process_callbacks from __do_softirq+0xb4/0x22c > > > __do_softirq from call_with_stack+0x1c/0x24 > > > call_with_stack from do_softirq+0x60/0x6c > > > do_softirq from __local_bh_enable_ip+0xa0/0xcc > > > > > > Patch "net: dst: Prevent false sharing vs. dst_entry:: __refcnt" moved > > > rt_uncached and rt_uncached_list fields from rtable struct to dst > > > struct, so they are more zeroed by memset_after(xdst, 0, u.dst) in > > > xfrm_alloc_dst(). > > > > > > Note that rt_uncached (list_head) was never properly initialized at > > > alloc time, but xfrm[46]_dst_destroy() is written in such a way that > > > it was not an issue thanks to the memset: > > > > > > if (xdst->u.rt.dst.rt_uncached_list) > > > rt_del_uncached_list(&xdst->u.rt); > > > > > > The route code does it the other way around: rt_uncached_list is > > > assumed to be valid IIF rt_uncached list_head is not empty: > > > > > > void rt_del_uncached_list(struct rtable *rt) > > > { > > > if (!list_empty(&rt->dst.rt_uncached)) { > > > struct uncached_list *ul = rt->dst.rt_uncached_list; > > > > > > spin_lock_bh(&ul->lock); > > > list_del_init(&rt->dst.rt_uncached); > > > spin_unlock_bh(&ul->lock); > > > } > > > } > > > > > > This patch adds mandatory rt_uncached list_head initialization in > > > generic dst_init(), and adapt xfrm[46]_dst_destroy logic to match the > > > rest of the code. > > > > > > Fixes: d288a162dd1c ("net: dst: Prevent false sharing vs. dst_entry:: __refcnt") > > > Signed-off-by: Maxime Bizon <mbizon@freebox.fr> > > > --- > > > net/core/dst.c | 1 + > > > net/ipv4/xfrm4_policy.c | 4 +--- > > > net/ipv6/route.c | 1 - > > > net/ipv6/xfrm6_policy.c | 4 +--- > > > 4 files changed, 3 insertions(+), 7 deletions(-) > > > > It should go to net. Right now -rc7 is broken. > > > > Also the change is not complete, you need to delete INIT_LIST_HEAD(..rt_uncached) > > from rt_dst_alloc and rt_dst_clone too. > > It will be nice to give a credit to kbuild. > https://lore.kernel.org/all/202304162125.18b7bcdd-oliver.sang@intel.com > It seems Maxime found the issue before kbuild. But feel free to add additional tags, sure.
On Tue, Apr 18, 2023 at 06:54:26PM +0200, Maxime Bizon wrote: > xfrm_alloc_dst() followed by xfrm4_dst_destroy(), without a > xfrm4_fill_dst() call in between, causes the following BUG: > > BUG: spinlock bad magic on CPU#0, fbxhostapd/732 > lock: 0x890b7668, .magic: 890b7668, .owner: <none>/-1, .owner_cpu: 0 > CPU: 0 PID: 732 Comm: fbxhostapd Not tainted 6.3.0-rc6-next-20230414-00613-ge8de66369925-dirty #9 > Hardware name: Marvell Kirkwood (Flattened Device Tree) > unwind_backtrace from show_stack+0x10/0x14 > show_stack from dump_stack_lvl+0x28/0x30 > dump_stack_lvl from do_raw_spin_lock+0x20/0x80 > do_raw_spin_lock from rt_del_uncached_list+0x30/0x64 > rt_del_uncached_list from xfrm4_dst_destroy+0x3c/0xbc > xfrm4_dst_destroy from dst_destroy+0x5c/0xb0 > dst_destroy from rcu_process_callbacks+0xc4/0xec > rcu_process_callbacks from __do_softirq+0xb4/0x22c > __do_softirq from call_with_stack+0x1c/0x24 > call_with_stack from do_softirq+0x60/0x6c > do_softirq from __local_bh_enable_ip+0xa0/0xcc > > Patch "net: dst: Prevent false sharing vs. dst_entry:: __refcnt" moved > rt_uncached and rt_uncached_list fields from rtable struct to dst > struct, so they are more zeroed by memset_after(xdst, 0, u.dst) in > xfrm_alloc_dst(). > > Note that rt_uncached (list_head) was never properly initialized at > alloc time, but xfrm[46]_dst_destroy() is written in such a way that > it was not an issue thanks to the memset: > > if (xdst->u.rt.dst.rt_uncached_list) > rt_del_uncached_list(&xdst->u.rt); > > The route code does it the other way around: rt_uncached_list is > assumed to be valid IIF rt_uncached list_head is not empty: > > void rt_del_uncached_list(struct rtable *rt) > { > if (!list_empty(&rt->dst.rt_uncached)) { > struct uncached_list *ul = rt->dst.rt_uncached_list; > > spin_lock_bh(&ul->lock); > list_del_init(&rt->dst.rt_uncached); > spin_unlock_bh(&ul->lock); > } > } > > This patch adds mandatory rt_uncached list_head initialization in > generic dst_init(), and adapt xfrm[46]_dst_destroy logic to match the > rest of the code. > > Fixes: d288a162dd1c ("net: dst: Prevent false sharing vs. dst_entry:: __refcnt") > Signed-off-by: Maxime Bizon <mbizon@freebox.fr> > --- > net/core/dst.c | 1 + > net/ipv4/xfrm4_policy.c | 4 +--- > net/ipv6/route.c | 1 - > net/ipv6/xfrm6_policy.c | 4 +--- > 4 files changed, 3 insertions(+), 7 deletions(-) > This fixes the bug introduced by the Fixes commit, so: Reviewed-by: David Ahern <dsahern@kernel.org> uncached_list is defined twice - net/ipv{4,6}/route.c. Since uncached_list was moved from protocol local structs to dst_entry, the definition for uncached_list should be moved there as well.
On Wed, 19 Apr 2023 11:58:02 +0300 Leon Romanovsky wrote:
> It should go to net. Right now -rc7 is broken.
That's not true, right? The bad commit is in net-next only.
On Wed, Apr 19, 2023 at 03:41:23PM -0700, Jakub Kicinski wrote: > On Wed, 19 Apr 2023 11:58:02 +0300 Leon Romanovsky wrote: > > It should go to net. Right now -rc7 is broken. > > That's not true, right? The bad commit is in net-next only. Sorry, I was wrong, it is net-next. My mistake was to combine two bugs in the same bucket as they came together in our tests. This rt_uncached thing and XFRM fix which I sent to ipsec-rc. Thanks
diff --git a/net/core/dst.c b/net/core/dst.c index 3247e84045ca..79d9306ad1ee 100644 --- a/net/core/dst.c +++ b/net/core/dst.c @@ -67,6 +67,7 @@ void dst_init(struct dst_entry *dst, struct dst_ops *ops, #endif dst->lwtstate = NULL; rcuref_init(&dst->__rcuref, initial_ref); + INIT_LIST_HEAD(&dst->rt_uncached); dst->__use = 0; dst->lastuse = jiffies; dst->flags = flags; diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c index 47861c8b7340..9403bbaf1b61 100644 --- a/net/ipv4/xfrm4_policy.c +++ b/net/ipv4/xfrm4_policy.c @@ -91,7 +91,6 @@ static int xfrm4_fill_dst(struct xfrm_dst *xdst, struct net_device *dev, xdst->u.rt.rt_gw6 = rt->rt_gw6; xdst->u.rt.rt_pmtu = rt->rt_pmtu; xdst->u.rt.rt_mtu_locked = rt->rt_mtu_locked; - INIT_LIST_HEAD(&xdst->u.rt.dst.rt_uncached); rt_add_uncached_list(&xdst->u.rt); return 0; @@ -121,8 +120,7 @@ static void xfrm4_dst_destroy(struct dst_entry *dst) struct xfrm_dst *xdst = (struct xfrm_dst *)dst; dst_destroy_metrics_generic(dst); - if (xdst->u.rt.dst.rt_uncached_list) - rt_del_uncached_list(&xdst->u.rt); + rt_del_uncached_list(&xdst->u.rt); xfrm_dst_destroy(xdst); } diff --git a/net/ipv6/route.c b/net/ipv6/route.c index 35085fc0cf15..e3aec46bd466 100644 --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -334,7 +334,6 @@ static const struct rt6_info ip6_blk_hole_entry_template = { static void rt6_info_init(struct rt6_info *rt) { memset_after(rt, 0, dst); - INIT_LIST_HEAD(&rt->dst.rt_uncached); } /* allocate dst with ip6_dst_ops */ diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c index 2b493f8d0091..eecc5e59da17 100644 --- a/net/ipv6/xfrm6_policy.c +++ b/net/ipv6/xfrm6_policy.c @@ -89,7 +89,6 @@ static int xfrm6_fill_dst(struct xfrm_dst *xdst, struct net_device *dev, xdst->u.rt6.rt6i_gateway = rt->rt6i_gateway; xdst->u.rt6.rt6i_dst = rt->rt6i_dst; xdst->u.rt6.rt6i_src = rt->rt6i_src; - INIT_LIST_HEAD(&xdst->u.rt6.dst.rt_uncached); rt6_uncached_list_add(&xdst->u.rt6); return 0; @@ -121,8 +120,7 @@ static void xfrm6_dst_destroy(struct dst_entry *dst) if (likely(xdst->u.rt6.rt6i_idev)) in6_dev_put(xdst->u.rt6.rt6i_idev); dst_destroy_metrics_generic(dst); - if (xdst->u.rt6.dst.rt_uncached_list) - rt6_uncached_list_del(&xdst->u.rt6); + rt6_uncached_list_del(&xdst->u.rt6); xfrm_dst_destroy(xdst); }
xfrm_alloc_dst() followed by xfrm4_dst_destroy(), without a xfrm4_fill_dst() call in between, causes the following BUG: BUG: spinlock bad magic on CPU#0, fbxhostapd/732 lock: 0x890b7668, .magic: 890b7668, .owner: <none>/-1, .owner_cpu: 0 CPU: 0 PID: 732 Comm: fbxhostapd Not tainted 6.3.0-rc6-next-20230414-00613-ge8de66369925-dirty #9 Hardware name: Marvell Kirkwood (Flattened Device Tree) unwind_backtrace from show_stack+0x10/0x14 show_stack from dump_stack_lvl+0x28/0x30 dump_stack_lvl from do_raw_spin_lock+0x20/0x80 do_raw_spin_lock from rt_del_uncached_list+0x30/0x64 rt_del_uncached_list from xfrm4_dst_destroy+0x3c/0xbc xfrm4_dst_destroy from dst_destroy+0x5c/0xb0 dst_destroy from rcu_process_callbacks+0xc4/0xec rcu_process_callbacks from __do_softirq+0xb4/0x22c __do_softirq from call_with_stack+0x1c/0x24 call_with_stack from do_softirq+0x60/0x6c do_softirq from __local_bh_enable_ip+0xa0/0xcc Patch "net: dst: Prevent false sharing vs. dst_entry:: __refcnt" moved rt_uncached and rt_uncached_list fields from rtable struct to dst struct, so they are more zeroed by memset_after(xdst, 0, u.dst) in xfrm_alloc_dst(). Note that rt_uncached (list_head) was never properly initialized at alloc time, but xfrm[46]_dst_destroy() is written in such a way that it was not an issue thanks to the memset: if (xdst->u.rt.dst.rt_uncached_list) rt_del_uncached_list(&xdst->u.rt); The route code does it the other way around: rt_uncached_list is assumed to be valid IIF rt_uncached list_head is not empty: void rt_del_uncached_list(struct rtable *rt) { if (!list_empty(&rt->dst.rt_uncached)) { struct uncached_list *ul = rt->dst.rt_uncached_list; spin_lock_bh(&ul->lock); list_del_init(&rt->dst.rt_uncached); spin_unlock_bh(&ul->lock); } } This patch adds mandatory rt_uncached list_head initialization in generic dst_init(), and adapt xfrm[46]_dst_destroy logic to match the rest of the code. Fixes: d288a162dd1c ("net: dst: Prevent false sharing vs. dst_entry:: __refcnt") Signed-off-by: Maxime Bizon <mbizon@freebox.fr> --- net/core/dst.c | 1 + net/ipv4/xfrm4_policy.c | 4 +--- net/ipv6/route.c | 1 - net/ipv6/xfrm6_policy.c | 4 +--- 4 files changed, 3 insertions(+), 7 deletions(-)