mbox series

[net,v2,0/2] net: fix lockdep false positive splat

Message ID 20210425155742.30057-1-ap420073@gmail.com
Headers show
Series net: fix lockdep false positive splat | expand

Message

Taehee Yoo April 25, 2021, 3:57 p.m. UTC 
This patchset is to fix lockdep false positive splat in multicast part of
the bridge.

When mdb or multicast config is changed, it acquires multicast_lock,
which is a per-interface(bridge) lock.
So, spin_lock_nested() should be used instead of spin_lock() because
interfaces can be recursive.
The spin_lock_nested() needs 'subclass' parameter.

The first patch adds a new helper function, which returns the nest_level
variable under RCU. The nest_level variable can be used as 'subclass'
parameter of spin_lock_nested().
The second patch fix lockdep false positive splat in the bridge multicast
part by using netdev_get_nest_level_rcu().

v2:
 - Remove dupliate Subject

Taehee Yoo (2):
  net: core: make bond_get_lowest_level_rcu() generic
  net: bridge: fix lockdep multicast_lock false positive splat

 drivers/net/bonding/bond_main.c |  45 +---------
 include/linux/netdevice.h       |   1 +
 net/bridge/br_mdb.c             |  12 +--
 net/bridge/br_multicast.c       | 146 ++++++++++++++++++++------------
 net/bridge/br_multicast_eht.c   |  18 ++--
 net/bridge/br_private.h         |  48 +++++++++++
 net/core/dev.c                  |  44 ++++++++++
 7 files changed, 204 insertions(+), 110 deletions(-)

Comments

Heiner Kallweit April 25, 2021, 6:03 p.m. UTC | #1 
On 25.04.2021 17:57, Taehee Yoo wrote:
> The purpose of bond_get_lowest_level_rcu() is to get nested_level under
> RCU. Because dev->nested_level is protected by RTNL, so it shouldn't be
> used without RTNL. But bonding module needs this value under RCU without
> RTNL.
> So, this function was added.
> 
> But, there is another module, which needs this function.
> So, make this function generic.
> the new name is netdev_get_nest_level_rcu().
> 
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> ---
> 
> v2:
>  - No change
> 
>  drivers/net/bonding/bond_main.c | 45 +--------------------------------
>  include/linux/netdevice.h       |  1 +
>  net/core/dev.c                  | 44 ++++++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 83ef62db6285..a9feb039ffa6 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -3754,47 +3754,6 @@ static void bond_fold_stats(struct rtnl_link_stats64 *_res,
>  	}
>  }
>  
> -#ifdef CONFIG_LOCKDEP
> -static int bond_get_lowest_level_rcu(struct net_device *dev)
> -{
> -	struct net_device *ldev, *next, *now, *dev_stack[MAX_NEST_DEV + 1];
> -	struct list_head *niter, *iter, *iter_stack[MAX_NEST_DEV + 1];
> -	int cur = 0, max = 0;
> -
> -	now = dev;
> -	iter = &dev->adj_list.lower;
> -
> -	while (1) {
> -		next = NULL;
> -		while (1) {
> -			ldev = netdev_next_lower_dev_rcu(now, &iter);
> -			if (!ldev)
> -				break;
> -
> -			next = ldev;
> -			niter = &ldev->adj_list.lower;
> -			dev_stack[cur] = now;
> -			iter_stack[cur++] = iter;
> -			if (max <= cur)
> -				max = cur;
> -			break;
> -		}
> -
> -		if (!next) {
> -			if (!cur)
> -				return max;
> -			next = dev_stack[--cur];
> -			niter = iter_stack[cur];
> -		}
> -
> -		now = next;
> -		iter = niter;
> -	}
> -
> -	return max;
> -}
> -#endif
> -
>  static void bond_get_stats(struct net_device *bond_dev,
>  			   struct rtnl_link_stats64 *stats)
>  {
> @@ -3806,9 +3765,7 @@ static void bond_get_stats(struct net_device *bond_dev,
>  
>  
>  	rcu_read_lock();
> -#ifdef CONFIG_LOCKDEP
> -	nest_level = bond_get_lowest_level_rcu(bond_dev);
> -#endif
> +	nest_level = netdev_get_nest_level_rcu(bond_dev);
>  
>  	spin_lock_nested(&bond->stats_lock, nest_level);
>  	memcpy(stats, &bond->bond_stats, sizeof(*stats));
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 87a5d186faff..507c06bf5dba 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -4699,6 +4699,7 @@ int netdev_walk_all_lower_dev(struct net_device *dev,
>  			      int (*fn)(struct net_device *lower_dev,
>  					struct netdev_nested_priv *priv),
>  			      struct netdev_nested_priv *priv);
> +int netdev_get_nest_level_rcu(struct net_device *dev);
>  int netdev_walk_all_lower_dev_rcu(struct net_device *dev,
>  				  int (*fn)(struct net_device *lower_dev,
>  					    struct netdev_nested_priv *priv),
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 15fe36332fb8..efc2bf88eafd 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7709,6 +7709,50 @@ static int __netdev_update_lower_level(struct net_device *dev,
>  	return 0;
>  }
>  
> +int netdev_get_nest_level_rcu(struct net_device *dev)
> +{
> +#ifdef CONFIG_LOCKDEP
> +	struct net_device *ldev, *next, *now, *dev_stack[MAX_NEST_DEV + 1];
> +	struct list_head *niter, *iter, *iter_stack[MAX_NEST_DEV + 1];
> +	int cur = 0, max = 0;
> +
> +	now = dev;
> +	iter = &dev->adj_list.lower;
> +
> +	while (1) {
> +		next = NULL;
> +		while (1) {
> +			ldev = netdev_next_lower_dev_rcu(now, &iter);
> +			if (!ldev)
> +				break;
> +
> +			next = ldev;
> +			niter = &ldev->adj_list.lower;
> +			dev_stack[cur] = now;
> +			iter_stack[cur++] = iter;
> +			if (max <= cur)
> +				max = cur;
> +			break;

This looks odd. Why a while loop if it's left in the first iteration
anyway? The whole loop looks unnecessarily complex. The following
should do the same, just in a simpler way (untested!)

        while (1) {
                ldev = netdev_next_lower_dev_rcu(now, &iter);
                if (ldev) {
                        dev_stack[cur] = now;
                        iter_stack[cur++] = iter;
                        if (max <= cur)
                                max = cur;
                        now = ldev;
                        iter = &ldev->adj_list.lower;
                } else {
                        if (!cur)
                                break;
                        now = dev_stack[--cur];
                        iter = iter_stack[cur];
                }
        }

I know that you just copied the original function.
Simplifying the function should be something for a
follow-up patch.

> +		}
> +
> +		if (!next) {
> +			if (!cur)
> +				return max;
> +			next = dev_stack[--cur];
> +			niter = iter_stack[cur];
> +		}
> +
> +		now = next;
> +		iter = niter;
> +	}
> +
> +	return max;
> +#else
> +	return 0;
> +#endif
> +}
> +EXPORT_SYMBOL_GPL(netdev_get_nest_level_rcu);
> +
>  int netdev_walk_all_lower_dev_rcu(struct net_device *dev,
>  				  int (*fn)(struct net_device *dev,
>  					    struct netdev_nested_priv *priv),
>
Taehee Yoo April 26, 2021, 3:24 p.m. UTC | #2 
On 4/26/21 3:03 AM, Heiner Kallweit wrote:

Hi Heiner,
Thank you for the review!

 > On 25.04.2021 17:57, Taehee Yoo wrote:

 >> The purpose of bond_get_lowest_level_rcu() is to get nested_level under

 >> RCU. Because dev->nested_level is protected by RTNL, so it shouldn't be

 >> used without RTNL. But bonding module needs this value under RCU without

 >> RTNL.

 >> So, this function was added.

 >>

 >> But, there is another module, which needs this function.

 >> So, make this function generic.

 >> the new name is netdev_get_nest_level_rcu().

 >>

 >> Signed-off-by: Taehee Yoo <ap420073@gmail.com>

 >> ---

 >>

 >> v2:

 >>   - No change

 >>

 >>   drivers/net/bonding/bond_main.c | 45 +--------------------------------

 >>   include/linux/netdevice.h       |  1 +

 >>   net/core/dev.c                  | 44 ++++++++++++++++++++++++++++++++

 >>   3 files changed, 46 insertions(+), 44 deletions(-)

 >>

 >> diff --git a/drivers/net/bonding/bond_main.c 

b/drivers/net/bonding/bond_main.c
 >> index 83ef62db6285..a9feb039ffa6 100644

 >> --- a/drivers/net/bonding/bond_main.c

 >> +++ b/drivers/net/bonding/bond_main.c

 >> @@ -3754,47 +3754,6 @@ static void bond_fold_stats(struct 

rtnl_link_stats64 *_res,
 >>   	}

 >>   }

 >>

 >> -#ifdef CONFIG_LOCKDEP

 >> -static int bond_get_lowest_level_rcu(struct net_device *dev)

 >> -{

 >> -	struct net_device *ldev, *next, *now, *dev_stack[MAX_NEST_DEV + 1];

 >> -	struct list_head *niter, *iter, *iter_stack[MAX_NEST_DEV + 1];

 >> -	int cur = 0, max = 0;

 >> -

 >> -	now = dev;

 >> -	iter = &dev->adj_list.lower;

 >> -

 >> -	while (1) {

 >> -		next = NULL;

 >> -		while (1) {

 >> -			ldev = netdev_next_lower_dev_rcu(now, &iter);

 >> -			if (!ldev)

 >> -				break;

 >> -

 >> -			next = ldev;

 >> -			niter = &ldev->adj_list.lower;

 >> -			dev_stack[cur] = now;

 >> -			iter_stack[cur++] = iter;

 >> -			if (max <= cur)

 >> -				max = cur;

 >> -			break;

 >> -		}

 >> -

 >> -		if (!next) {

 >> -			if (!cur)

 >> -				return max;

 >> -			next = dev_stack[--cur];

 >> -			niter = iter_stack[cur];

 >> -		}

 >> -

 >> -		now = next;

 >> -		iter = niter;

 >> -	}

 >> -

 >> -	return max;

 >> -}

 >> -#endif

 >> -

 >>   static void bond_get_stats(struct net_device *bond_dev,

 >>   			   struct rtnl_link_stats64 *stats)

 >>   {

 >> @@ -3806,9 +3765,7 @@ static void bond_get_stats(struct net_device 

*bond_dev,
 >>

 >>

 >>   	rcu_read_lock();

 >> -#ifdef CONFIG_LOCKDEP

 >> -	nest_level = bond_get_lowest_level_rcu(bond_dev);

 >> -#endif

 >> +	nest_level = netdev_get_nest_level_rcu(bond_dev);

 >>

 >>   	spin_lock_nested(&bond->stats_lock, nest_level);

 >>   	memcpy(stats, &bond->bond_stats, sizeof(*stats));

 >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h

 >> index 87a5d186faff..507c06bf5dba 100644

 >> --- a/include/linux/netdevice.h

 >> +++ b/include/linux/netdevice.h

 >> @@ -4699,6 +4699,7 @@ int netdev_walk_all_lower_dev(struct 

net_device *dev,
 >>   			      int (*fn)(struct net_device *lower_dev,

 >>   					struct netdev_nested_priv *priv),

 >>   			      struct netdev_nested_priv *priv);

 >> +int netdev_get_nest_level_rcu(struct net_device *dev);

 >>   int netdev_walk_all_lower_dev_rcu(struct net_device *dev,

 >>   				  int (*fn)(struct net_device *lower_dev,

 >>   					    struct netdev_nested_priv *priv),

 >> diff --git a/net/core/dev.c b/net/core/dev.c

 >> index 15fe36332fb8..efc2bf88eafd 100644

 >> --- a/net/core/dev.c

 >> +++ b/net/core/dev.c

 >> @@ -7709,6 +7709,50 @@ static int __netdev_update_lower_level(struct 

net_device *dev,
 >>   	return 0;

 >>   }

 >>

 >> +int netdev_get_nest_level_rcu(struct net_device *dev)

 >> +{

 >> +#ifdef CONFIG_LOCKDEP

 >> +	struct net_device *ldev, *next, *now, *dev_stack[MAX_NEST_DEV + 1];

 >> +	struct list_head *niter, *iter, *iter_stack[MAX_NEST_DEV + 1];

 >> +	int cur = 0, max = 0;

 >> +

 >> +	now = dev;

 >> +	iter = &dev->adj_list.lower;

 >> +

 >> +	while (1) {

 >> +		next = NULL;

 >> +		while (1) {

 >> +			ldev = netdev_next_lower_dev_rcu(now, &iter);

 >> +			if (!ldev)

 >> +				break;

 >> +

 >> +			next = ldev;

 >> +			niter = &ldev->adj_list.lower;

 >> +			dev_stack[cur] = now;

 >> +			iter_stack[cur++] = iter;

 >> +			if (max <= cur)

 >> +				max = cur;

 >> +			break;

 >

 > This looks odd. Why a while loop if it's left in the first iteration

 > anyway? The whole loop looks unnecessarily complex. The following

 > should do the same, just in a simpler way (untested!)

 >

 >          while (1) {

 >                  ldev = netdev_next_lower_dev_rcu(now, &iter);

 >                  if (ldev) {

 >                          dev_stack[cur] = now;

 >                          iter_stack[cur++] = iter;

 >                          if (max <= cur)

 >                                  max = cur;

 >                          now = ldev;

 >                          iter = &ldev->adj_list.lower;

 >                  } else {

 >                          if (!cur)

 >                                  break;

 >                          now = dev_stack[--cur];

 >                          iter = iter_stack[cur];

 >                  }

 >          }

 >

 > I know that you just copied the original function.

 > Simplifying the function should be something for a

 > follow-up patch.

 >

 >> +		}

 >> +

 >> +		if (!next) {

 >> +			if (!cur)

 >> +				return max;

 >> +			next = dev_stack[--cur];

 >> +			niter = iter_stack[cur];

 >> +		}

 >> +

 >> +		now = next;

 >> +		iter = niter;

 >> +	}

 >> +

 >> +	return max;

 >> +#else

 >> +	return 0;

 >> +#endif

 >> +}

 >> +EXPORT_SYMBOL_GPL(netdev_get_nest_level_rcu);

 >> +

 >>   int netdev_walk_all_lower_dev_rcu(struct net_device *dev,

 >>   				  int (*fn)(struct net_device *dev,

 >>   					    struct netdev_nested_priv *priv),

 >>

 >


I think you're right.
Your logic is more simple and stable.
If I send a v3 patch, I will use your logic after some tests.

Thanks a lot!
Taehee