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

[Paolo Abeni <pabeni@redhat.com>]

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 <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.
>>>>
>>>> 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