| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d7cf5368-21a4-a551-169a-00a4cb2b3a0d@gmail.com>
Date: Sun, 25 Apr 2021 20:03:49 +0200
From: Heiner Kallweit <hkallweit1@...il.com>
To: Taehee Yoo <ap420073@...il.com>, davem@...emloft.net,
kuba@...nel.org, dsahern@...nel.org, yoshfuji@...ux-ipv6.org,
netdev@...r.kernel.org, j.vosburgh@...il.com, vfalico@...il.com,
andy@...yhouse.net, roopa@...dia.com, nikolay@...dia.com,
ast@...nel.org, andriin@...com, daniel@...earbox.net,
weiwan@...gle.com, cong.wang@...edance.com, bjorn@...nel.org,
herbert@...dor.apana.org.au, bridge@...ts.linux-foundation.org
Subject: Re: [PATCH net v2 1/2] net: core: make bond_get_lowest_level_rcu()
generic
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@...il.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),
>
Powered by blists - more mailing lists