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

Commit Message

Cong Wang Sept. 12, 2024, 12:04 a.m. UTC

From: Cong Wang <[e-mail]>

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: [e-mail]
Closes: https://syzkaller.appspot.com/bug?extid=c75d1de73d3b8b76272f
Cc: Wenjia Zhang <[e-mail]>
Cc: Jan Karcher <[e-mail]>
Cc: "D. Wythe" <[e-mail]>
Cc: Tony Lu <[e-mail]>
Cc: Wen Gu <[e-mail]>
Signed-off-by: Cong Wang <[e-mail]>
---
 net/smc/smc_core.c |  6 +++---
 net/smc/smc_pnet.c | 14 +++++++-------
 2 files changed, 10 insertions(+), 10 deletions(-)

Comments

D. Wythe Sept. 12, 2024, 6:20 a.m. UTC | #1

On 9/12/24 8:04 AM, Cong Wang wrote:
> From: Cong Wang <[e-mail]>
> 
> 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: [e-mail]
> Closes: https://syzkaller.appspot.com/bug?extid=c75d1de73d3b8b76272f
> Cc: Wenjia Zhang <[e-mail]>
> Cc: Jan Karcher <[e-mail]>
> Cc: "D. Wythe" <[e-mail]>
> Cc: Tony Lu <[e-mail]>
> Cc: Wen Gu <[e-mail]>
> Signed-off-by: Cong Wang <[e-mail]>


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

Cong Wang Sept. 14, 2024, 12:53 a.m. UTC | #2

On Thu, Sep 12, 2024 at 02:20:47PM +0800, D. Wythe wrote:
> 
> 
> On 9/12/24 8:04 AM, Cong Wang wrote:
> > From: Cong Wang <[e-mail]>
> > 
> > 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: [e-mail]
> > Closes: https://syzkaller.appspot.com/bug?extid=c75d1de73d3b8b76272f
> > Cc: Wenjia Zhang <[e-mail]>
> > Cc: Jan Karcher <[e-mail]>
> > Cc: "D. Wythe" <[e-mail]>
> > Cc: Tony Lu <[e-mail]>
> > Cc: Wen Gu <[e-mail]>
> > Signed-off-by: Cong Wang <[e-mail]>
> 
> 
> 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.

If you look at the code, it is not about sock lock annotations, it is
about RTNL lock which of course has annotations.

And you don't even need to bother sock lock annotations for this specific
case at all (I can't say any other case).

Thanks.

D. Wythe Sept. 14, 2024, 2:28 a.m. UTC | #3

On 9/14/24 8:53 AM, Cong Wang wrote:
> On Thu, Sep 12, 2024 at 02:20:47PM +0800, D. Wythe wrote:
>>
>>
>> On 9/12/24 8:04 AM, Cong Wang wrote:
>>> From: Cong Wang <[e-mail]>
>>>
>>> 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: [e-mail]
>>> Closes: https://syzkaller.appspot.com/bug?extid=c75d1de73d3b8b76272f
>>> Cc: Wenjia Zhang <[e-mail]>
>>> Cc: Jan Karcher <[e-mail]>
>>> Cc: "D. Wythe" <[e-mail]>
>>> Cc: Tony Lu <[e-mail]>
>>> Cc: Wen Gu <[e-mail]>
>>> Signed-off-by: Cong Wang <[e-mail]>
>>
>>
>> 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.
> 
> If you look at the code, it is not about sock lock annotations, it is
> about RTNL lock which of course has annotations.
> 

If so, please explain the deadlock issue mentioned in sysbot and
how it triggers deadlocks.

> And you don't even need to bother sock lock annotations for this specific
> case at all (I can't say any other case).
> 
> Thanks.

Cong Wang Sept. 14, 2024, 3:32 a.m. UTC | #4

On Sat, Sep 14, 2024 at 10:28:15AM +0800, D. Wythe wrote:
> 
> 
> On 9/14/24 8:53 AM, Cong Wang wrote:
> > On Thu, Sep 12, 2024 at 02:20:47PM +0800, D. Wythe wrote:
> > > 
> > > 
> > > On 9/12/24 8:04 AM, Cong Wang wrote:
> > > > From: Cong Wang <[e-mail]>
> > > > 
> > > > 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: [e-mail]
> > > > Closes: https://syzkaller.appspot.com/bug?extid=c75d1de73d3b8b76272f
> > > > Cc: Wenjia Zhang <[e-mail]>
> > > > Cc: Jan Karcher <[e-mail]>
> > > > Cc: "D. Wythe" <[e-mail]>
> > > > Cc: Tony Lu <[e-mail]>
> > > > Cc: Wen Gu <[e-mail]>
> > > > Signed-off-by: Cong Wang <[e-mail]>
> > > 
> > > 
> > > 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.
> > 
> > If you look at the code, it is not about sock lock annotations, it is
> > about RTNL lock which of course has annotations.
> > 
> 
> If so, please explain the deadlock issue mentioned in sysbot and
> how it triggers deadlocks.

Sure, but what questions do you have here? To me, the lockdep output is
self-explained. Please kindly let me know if you have any troubles
understanding it, I am always happy to help.

Thanks.

D. Wythe Sept. 18, 2024, 2:23 a.m. UTC | #5

On 9/14/24 11:32 AM, Cong Wang wrote:
> On Sat, Sep 14, 2024 at 10:28:15AM +0800, D. Wythe wrote:
>>
>>
>> On 9/14/24 8:53 AM, Cong Wang wrote:
>>> On Thu, Sep 12, 2024 at 02:20:47PM +0800, D. Wythe wrote:
>>>>
>>>>
>>>> On 9/12/24 8:04 AM, Cong Wang wrote:
>>>>> From: Cong Wang <[e-mail]>
>>>>>
>>>>> 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: [e-mail]
>>>>> Closes: https://syzkaller.appspot.com/bug?extid=c75d1de73d3b8b76272f
>>>>> Cc: Wenjia Zhang <[e-mail]>
>>>>> Cc: Jan Karcher <[e-mail]>
>>>>> Cc: "D. Wythe" <[e-mail]>
>>>>> Cc: Tony Lu <[e-mail]>
>>>>> Cc: Wen Gu <[e-mail]>
>>>>> Signed-off-by: Cong Wang <[e-mail]>
>>>>
>>>>
>>>> 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.
>>>
>>> If you look at the code, it is not about sock lock annotations, it is
>>> about RTNL lock which of course has annotations.
>>>
>>
>> If so, please explain the deadlock issue mentioned in sysbot and
>> how it triggers deadlocks.
> 
> Sure, but what questions do you have here? To me, the lockdep output is
> self-explained. Please kindly let me know if you have any troubles
> understanding it, I am always happy to help.
> 
> Thanks.

Just explain (https://syzkaller.appspot.com/bug?extid=c75d1de73d3b8b76272f)

-> #1 (sk_lock-AF_INET6){+.+.}-{0:0}:
        lock_sock_nested+0x3a/0xf0 net/core/sock.c:3543
        lock_sock include/net/sock.h:1607 [inline]
        sockopt_lock_sock net/core/sock.c:1061 [inline]
        sockopt_lock_sock+0x54/0x70 net/core/sock.c:1052
        do_ipv6_setsockopt+0x216a/0x47b0 net/ipv6/ipv6_sockglue.c:567
        ipv6_setsockopt+0xe3/0x1a0 net/ipv6/ipv6_sockglue.c:993
        udpv6_setsockopt+0x7d/0xd0 net/ipv6/udp.c:1702
        do_sock_setsockopt+0x222/0x480 net/socket.c:2324
        __sys_setsockopt+0x1a4/0x270 net/socket.c:2347
        __do_sys_setsockopt net/socket.c:2356 [inline]
        __se_sys_setsockopt net/socket.c:2353 [inline]
        __x64_sys_setsockopt+0xbd/0x160 net/socket.c:2353
        do_syscall_x64 arch/x86/entry/common.c:52 [inline]
        do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
        entry_SYSCALL_64_after_hwframe+0x77/0x7f

Why is that udpv6_setsockopt was reported here.

D.

Paolo Abeni Sept. 19, 2024, 9:30 a.m. UTC | #6

Hi,
On 9/18/24 04:23, D. Wythe wrote:
> On 9/14/24 11:32 AM, Cong Wang wrote:
>> On Sat, Sep 14, 2024 at 10:28:15AM +0800, D. Wythe wrote:
>>>
>>>
>>> On 9/14/24 8:53 AM, Cong Wang wrote:
>>>> On Thu, Sep 12, 2024 at 02:20:47PM +0800, D. Wythe wrote:
>>>>>
>>>>>
>>>>> On 9/12/24 8:04 AM, Cong Wang wrote:
>>>>>> From: Cong Wang <[e-mail]>
>>>>>>
>>>>>> 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: [e-mail]
>>>>>> Closes: https://syzkaller.appspot.com/bug?extid=c75d1de73d3b8b76272f
>>>>>> Cc: Wenjia Zhang <[e-mail]>
>>>>>> Cc: Jan Karcher <[e-mail]>
>>>>>> Cc: "D. Wythe" <[e-mail]>
>>>>>> Cc: Tony Lu <[e-mail]>
>>>>>> Cc: Wen Gu <[e-mail]>
>>>>>> Signed-off-by: Cong Wang <[e-mail]>
>>>>>
>>>>>
>>>>> 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.
>>>>
>>>> If you look at the code, it is not about sock lock annotations, it is
>>>> about RTNL lock which of course has annotations.
>>>>
>>>
>>> If so, please explain the deadlock issue mentioned in sysbot and
>>> how it triggers deadlocks.
>>
>> Sure, but what questions do you have here? To me, the lockdep output is
>> self-explained. Please kindly let me know if you have any troubles
>> understanding it, I am always happy to help.
>>
>> Thanks.
> 
> Just explain (https://syzkaller.appspot.com/bug?extid=c75d1de73d3b8b76272f)
> 
> -> #1 (sk_lock-AF_INET6){+.+.}-{0:0}:
>          lock_sock_nested+0x3a/0xf0 net/core/sock.c:3543
>          lock_sock include/net/sock.h:1607 [inline]
>          sockopt_lock_sock net/core/sock.c:1061 [inline]
>          sockopt_lock_sock+0x54/0x70 net/core/sock.c:1052
>          do_ipv6_setsockopt+0x216a/0x47b0 net/ipv6/ipv6_sockglue.c:567
>          ipv6_setsockopt+0xe3/0x1a0 net/ipv6/ipv6_sockglue.c:993
>          udpv6_setsockopt+0x7d/0xd0 net/ipv6/udp.c:1702
>          do_sock_setsockopt+0x222/0x480 net/socket.c:2324
>          __sys_setsockopt+0x1a4/0x270 net/socket.c:2347
>          __do_sys_setsockopt net/socket.c:2356 [inline]
>          __se_sys_setsockopt net/socket.c:2353 [inline]
>          __x64_sys_setsockopt+0xbd/0x160 net/socket.c:2353
>          do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>          do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
>          entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> Why is that udpv6_setsockopt was reported here.

If I read correctly, your doubt is somewhat alike the following: the SMC 
code does not call UDP sockopt-related function, so the above stacktrace 
refers to a non SMC socket and the reported splat is really harmless, as 
no deadlock will really happens (UDP sockets do not acquire nested rtnl 
lock, smc does not acquire nested socket lock).

Still the splat happens we need - or at least we should - address it, 
because this splat prevents syzkaller from finding other possibly more 
significant issues.

One way for addressing the splat would be adding the proper annotation 
to the socket lock. Another way is the present patch, which looks legit 
to me and should give performances benefit (every time we don't need to 
acquire the rtnl lock is a win!)

@Wythe: does the above clarify a bit?

Thanks!

Paolo

D. Wythe Sept. 19, 2024, 3:46 p.m. UTC | #7

On 9/19/24 5:30 PM, Paolo Abeni wrote:
> Hi,
> On 9/18/24 04:23, D. Wythe wrote:
>> On 9/14/24 11:32 AM, Cong Wang wrote:
>>> On Sat, Sep 14, 2024 at 10:28:15AM +0800, D. Wythe wrote:
>>>>
>>>>
>>>> On 9/14/24 8:53 AM, Cong Wang wrote:
>>>>> On Thu, Sep 12, 2024 at 02:20:47PM +0800, D. Wythe wrote:
>>>>>>
>>>>>>
>>>>>> On 9/12/24 8:04 AM, Cong Wang wrote:
>>>>>>> From: Cong Wang <[e-mail]>
>>>>>>>
>>>>>>> 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: [e-mail]
>>>>>>> Closes: https://syzkaller.appspot.com/bug?extid=c75d1de73d3b8b76272f
>>>>>>> Cc: Wenjia Zhang <[e-mail]>
>>>>>>> Cc: Jan Karcher <[e-mail]>
>>>>>>> Cc: "D. Wythe" <[e-mail]>
>>>>>>> Cc: Tony Lu <[e-mail]>
>>>>>>> Cc: Wen Gu <[e-mail]>
>>>>>>> Signed-off-by: Cong Wang <[e-mail]>
>>>>>>
>>>>>>
>>>>>> 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.
>>>>>
>>>>> If you look at the code, it is not about sock lock annotations, it is
>>>>> about RTNL lock which of course has annotations.
>>>>>
>>>>
>>>> If so, please explain the deadlock issue mentioned in sysbot and
>>>> how it triggers deadlocks.
>>>
>>> Sure, but what questions do you have here? To me, the lockdep output is
>>> self-explained. Please kindly let me know if you have any troubles
>>> understanding it, I am always happy to help.
>>>
>>> Thanks.
>>
>> Just explain (https://syzkaller.appspot.com/bug?extid=c75d1de73d3b8b76272f)
>>
>> -> #1 (sk_lock-AF_INET6){+.+.}-{0:0}:
>>          lock_sock_nested+0x3a/0xf0 net/core/sock.c:3543
>>          lock_sock include/net/sock.h:1607 [inline]
>>          sockopt_lock_sock net/core/sock.c:1061 [inline]
>>          sockopt_lock_sock+0x54/0x70 net/core/sock.c:1052
>>          do_ipv6_setsockopt+0x216a/0x47b0 net/ipv6/ipv6_sockglue.c:567
>>          ipv6_setsockopt+0xe3/0x1a0 net/ipv6/ipv6_sockglue.c:993
>>          udpv6_setsockopt+0x7d/0xd0 net/ipv6/udp.c:1702
>>          do_sock_setsockopt+0x222/0x480 net/socket.c:2324
>>          __sys_setsockopt+0x1a4/0x270 net/socket.c:2347
>>          __do_sys_setsockopt net/socket.c:2356 [inline]
>>          __se_sys_setsockopt net/socket.c:2353 [inline]
>>          __x64_sys_setsockopt+0xbd/0x160 net/socket.c:2353
>>          do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>>          do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
>>          entry_SYSCALL_64_after_hwframe+0x77/0x7f
>>
>> Why is that udpv6_setsockopt was reported here.
> 
> If I read correctly, your doubt is somewhat alike the following: the SMC code does not call UDP 
> sockopt-related function, so the above stacktrace refers to a non SMC socket and the reported splat 
> is really harmless, as no deadlock will really happens (UDP sockets do not acquire nested rtnl lock, 
> smc does not acquire nested socket lock).
> 
> Still the splat happens we need - or at least we should - address it, because this splat prevents 
> syzkaller from finding other possibly more significant issues.
> 
> One way for addressing the splat would be adding the proper annotation to the socket lock. Another 
> way is the present patch, which looks legit to me and should give performances benefit (every time 
> we don't need to acquire the rtnl lock is a win!)
> 
> @Wythe: does the above clarify a bit?
> 
> Thanks!
> 
> Paolo


Hi Paolo,

Thanks for your explanation. I did not question the value of this patch,
I just think that it did not fix a deadlock issue as it described. What it really does
is to avoid a false position from syzbot, and also has brought potential performance
benefits, which I totally agree with.


Last week, we also discussed this issue with Eric. In fact, we already have a patch
that addresses this problem by modifying the lockdep class of IPPROTO_SMC. However,
I'm not entirely satisfied with this change because I prefer that IPPROTO_SMC socks remain 
consistent with other AF_INET socks. So, it appears that this patch is the best solution now.

Anyway, I support this patch now. But I believe the description needs to be more accurate.

Thanks,
D. Wythe

Wenjia Zhang Sept. 23, 2024, 8:16 a.m. UTC | #8

On 19.09.24 17:46, D. Wythe wrote:
> 
> 
> On 9/19/24 5:30 PM, Paolo Abeni wrote:
>> Hi,
>> On 9/18/24 04:23, D. Wythe wrote:
>>> On 9/14/24 11:32 AM, Cong Wang wrote:
>>>> On Sat, Sep 14, 2024 at 10:28:15AM +0800, D. Wythe wrote:
>>>>>
>>>>>
>>>>> On 9/14/24 8:53 AM, Cong Wang wrote:
>>>>>> On Thu, Sep 12, 2024 at 02:20:47PM +0800, D. Wythe wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 9/12/24 8:04 AM, Cong Wang wrote:
>>>>>>>> From: Cong Wang <[e-mail]>
>>>>>>>>
>>>>>>>> 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: [e-mail]
>>>>>>>> Closes: 
>>>>>>>> https://syzkaller.appspot.com/bug?extid=c75d1de73d3b8b76272f
>>>>>>>> Cc: Wenjia Zhang <[e-mail]>
>>>>>>>> Cc: Jan Karcher <[e-mail]>
>>>>>>>> Cc: "D. Wythe" <[e-mail]>
>>>>>>>> Cc: Tony Lu <[e-mail]>
>>>>>>>> Cc: Wen Gu <[e-mail]>
>>>>>>>> Signed-off-by: Cong Wang <[e-mail]>
>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> If you look at the code, it is not about sock lock annotations, it is
>>>>>> about RTNL lock which of course has annotations.
>>>>>>
>>>>>
>>>>> If so, please explain the deadlock issue mentioned in sysbot and
>>>>> how it triggers deadlocks.
>>>>
>>>> Sure, but what questions do you have here? To me, the lockdep output is
>>>> self-explained. Please kindly let me know if you have any troubles
>>>> understanding it, I am always happy to help.
>>>>
>>>> Thanks.
>>>
>>> Just explain 
>>> (https://syzkaller.appspot.com/bug?extid=c75d1de73d3b8b76272f)
>>>
>>> -> #1 (sk_lock-AF_INET6){+.+.}-{0:0}:
>>>          lock_sock_nested+0x3a/0xf0 net/core/sock.c:3543
>>>          lock_sock include/net/sock.h:1607 [inline]
>>>          sockopt_lock_sock net/core/sock.c:1061 [inline]
>>>          sockopt_lock_sock+0x54/0x70 net/core/sock.c:1052
>>>          do_ipv6_setsockopt+0x216a/0x47b0 net/ipv6/ipv6_sockglue.c:567
>>>          ipv6_setsockopt+0xe3/0x1a0 net/ipv6/ipv6_sockglue.c:993
>>>          udpv6_setsockopt+0x7d/0xd0 net/ipv6/udp.c:1702
>>>          do_sock_setsockopt+0x222/0x480 net/socket.c:2324
>>>          __sys_setsockopt+0x1a4/0x270 net/socket.c:2347
>>>          __do_sys_setsockopt net/socket.c:2356 [inline]
>>>          __se_sys_setsockopt net/socket.c:2353 [inline]
>>>          __x64_sys_setsockopt+0xbd/0x160 net/socket.c:2353
>>>          do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>>>          do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
>>>          entry_SYSCALL_64_after_hwframe+0x77/0x7f
>>>
>>> Why is that udpv6_setsockopt was reported here.
>>
>> If I read correctly, your doubt is somewhat alike the following: the 
>> SMC code does not call UDP sockopt-related function, so the above 
>> stacktrace refers to a non SMC socket and the reported splat is really 
>> harmless, as no deadlock will really happens (UDP sockets do not 
>> acquire nested rtnl lock, smc does not acquire nested socket lock).
>>
>> Still the splat happens we need - or at least we should - address it, 
>> because this splat prevents syzkaller from finding other possibly more 
>> significant issues.
>>
>> One way for addressing the splat would be adding the proper annotation 
>> to the socket lock. Another way is the present patch, which looks 
>> legit to me and should give performances benefit (every time we don't 
>> need to acquire the rtnl lock is a win!)
>>
>> @Wythe: does the above clarify a bit?
>>
>> Thanks!
>>
>> Paolo
> 
> 
> Hi Paolo,
> 
> Thanks for your explanation. I did not question the value of this patch,
> I just think that it did not fix a deadlock issue as it described. What 
> it really does
> is to avoid a false position from syzbot, and also has brought potential 
> performance
> benefits, which I totally agree with.
> 
> 
> Last week, we also discussed this issue with Eric. In fact, we already 
> have a patch
> that addresses this problem by modifying the lockdep class of 
> IPPROTO_SMC. However,
> I'm not entirely satisfied with this change because I prefer that 
> IPPROTO_SMC socks remain consistent with other AF_INET socks. So, it 
> appears that this patch is the best solution now.
> 
> Anyway, I support this patch now. But I believe the description needs to 
> be more accurate.
> 
> Thanks,
> D. Wythe
> 
> 

I like the idea with the RCU version and it might solve the issue what 
the syzbot reported. However, I also agree with D. Wythe on lack of 
accurate description regarding this issue itself. That means where is 
the knot and how the RCU version solves the knot. That would also help 
people solve the similar problem later.
@Cong Wang, could you please add a bit more description I mentioned above?

Thanks,
Wenjia

ericnetdev dumazet Sept. 23, 2024, 3:48 p.m. UTC | #9

Le jeu. 12 sept. 2024 à 02:05, Cong Wang <[e-mail]> a écrit :
>
> From: Cong Wang <[e-mail]>
>
> 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: [e-mail]
> Closes: https://syzkaller.appspot.com/bug?extid=c75d1de73d3b8b76272f
> Cc: Wenjia Zhang <[e-mail]>
> Cc: Jan Karcher <[e-mail]>
> Cc: "D. Wythe" <[e-mail]>
> Cc: Tony Lu <[e-mail]>
> Cc: Wen Gu <[e-mail]>
> Signed-off-by: Cong Wang <[e-mail]>
> ---
>  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);

It seems smc_vlan_by_tcpsk_walk() depends on RTNL.

We should at least add a READ_ONCE() in is_vlan_dev() :

return READ_ONCE(dev->priv_flags) & IFF_802_1Q_VLAN;


> +       rcu_read_unlock();
>
>  out_rel:
>

Patch

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