[PATCH net-next] net: dst: fix missing initialization of rt_uncached

[PATCH net-next] net: dst: fix missing initialization of rt_uncached

[Maxime Bizon]

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(-)

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);
 }
 
-- 
2.34.1

Re: [PATCH net-next] net: dst: fix missing initialization of rt_uncached

[Eric Dumazet]

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.

Re: [PATCH net-next] net: dst: fix missing initialization of rt_uncached

[Leon Romanovsky]

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

Re: [PATCH net-next] net: dst: fix missing initialization of rt_uncached

[Leon Romanovsky]

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

Re: [PATCH net-next] net: dst: fix missing initialization of rt_uncached

[Eric Dumazet]

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.

Re: [PATCH net-next] net: dst: fix missing initialization of rt_uncached

[David Ahern]

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.

Re: [PATCH net-next] net: dst: fix missing initialization of rt_uncached

[Jakub Kicinski]

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.

Re: [PATCH net-next] net: dst: fix missing initialization of rt_uncached

[Leon Romanovsky]

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