Re: [Patch net] smc: use RCU version of lower netdev searching

["D. Wythe" <alibuda@linux.alibaba.com>]

On 9/12/24 8:04 AM, Cong Wang wrote:
> From: Cong Wang <cong.wang@bytedance.com>
> 
> Both netdev_walk_all_lower_dev() and netdev_lower_get_next() have a
> RCU version, which are netdev_walk_all_lower_dev_rcu() and
> netdev_next_lower_dev_rcu(). Switching to the RCU version would
> eliminate the need for RTL lock, thus could amend the deadlock
> complaints from syzbot. And it could also potentially speed up its
> callers like smc_connect().
> 
> Reported-by: syzbot+c75d1de73d3b8b76272f@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=c75d1de73d3b8b76272f
> Cc: Wenjia Zhang <wenjia@linux.ibm.com>
> Cc: Jan Karcher <jaka@linux.ibm.com>
> Cc: "D. Wythe" <alibuda@linux.alibaba.com>
> Cc: Tony Lu <tonylu@linux.alibaba.com>
> Cc: Wen Gu <guwen@linux.alibaba.com>
> Signed-off-by: Cong Wang <cong.wang@bytedance.com>


Haven't looked at your code yet, but the issue you fixed doesn't exist.
The real reason is that we lacks some lockdep annotations for
IPPROTO_SMC.

Thanks,
D. Wythe

> ---
>   net/smc/smc_core.c |  6 +++---
>   net/smc/smc_pnet.c | 14 +++++++-------
>   2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 3b95828d9976..574039b7d456 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -1850,9 +1850,9 @@ int smc_vlan_by_tcpsk(struct socket *clcsock, struct smc_init_info *ini)
>   	}
>   
>   	priv.data = (void *)&ini->vlan_id;
> -	rtnl_lock();
> -	netdev_walk_all_lower_dev(ndev, smc_vlan_by_tcpsk_walk, &priv);
> -	rtnl_unlock();
> +	rcu_read_lock();
> +	netdev_walk_all_lower_dev_rcu(ndev, smc_vlan_by_tcpsk_walk, &priv);
> +	rcu_read_unlock();
>   
>   out_rel:
>   	dst_release(dst);
> diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
> index 2adb92b8c469..b8ee6da08638 100644
> --- a/net/smc/smc_pnet.c
> +++ b/net/smc/smc_pnet.c
> @@ -29,7 +29,6 @@
>   #include "smc_ism.h"
>   #include "smc_core.h"
>   
> -static struct net_device *__pnet_find_base_ndev(struct net_device *ndev);
>   static struct net_device *pnet_find_base_ndev(struct net_device *ndev);
>   
>   static const struct nla_policy smc_pnet_policy[SMC_PNETID_MAX + 1] = {
> @@ -791,7 +790,7 @@ static void smc_pnet_add_base_pnetid(struct net *net, struct net_device *dev,
>   {
>   	struct net_device *base_dev;
>   
> -	base_dev = __pnet_find_base_ndev(dev);
> +	base_dev = pnet_find_base_ndev(dev);
>   	if (base_dev->flags & IFF_UP &&
>   	    !smc_pnetid_by_dev_port(base_dev->dev.parent, base_dev->dev_port,
>   				    ndev_pnetid)) {
> @@ -857,7 +856,7 @@ static int smc_pnet_netdev_event(struct notifier_block *this,
>   		smc_pnet_add_base_pnetid(net, event_dev, ndev_pnetid);
>   		return NOTIFY_OK;
>   	case NETDEV_DOWN:
> -		event_dev = __pnet_find_base_ndev(event_dev);
> +		event_dev = pnet_find_base_ndev(event_dev);
>   		if (!smc_pnetid_by_dev_port(event_dev->dev.parent,
>   					    event_dev->dev_port, ndev_pnetid)) {
>   			/* remove from PNETIDs list */
> @@ -925,7 +924,6 @@ static struct net_device *__pnet_find_base_ndev(struct net_device *ndev)
>   {
>   	int i, nest_lvl;
>   
> -	ASSERT_RTNL();
>   	nest_lvl = ndev->lower_level;
>   	for (i = 0; i < nest_lvl; i++) {
>   		struct list_head *lower = &ndev->adj_list.lower;
> @@ -933,7 +931,9 @@ static struct net_device *__pnet_find_base_ndev(struct net_device *ndev)
>   		if (list_empty(lower))
>   			break;
>   		lower = lower->next;
> -		ndev = netdev_lower_get_next(ndev, &lower);
> +		ndev = netdev_next_lower_dev_rcu(ndev, &lower);
> +		if (!ndev)
> +			break;
>   	}
>   	return ndev;
>   }
> @@ -945,9 +945,9 @@ static struct net_device *__pnet_find_base_ndev(struct net_device *ndev)
>    */
>   static struct net_device *pnet_find_base_ndev(struct net_device *ndev)
>   {
> -	rtnl_lock();
> +	rcu_read_lock();
>   	ndev = __pnet_find_base_ndev(ndev);
> -	rtnl_unlock();
> +	rcu_read_unlock();
>   	return ndev;
>   }
>