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

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

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


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