Message-ID: <a4cf0e03-98b7-49a5-bbb6-040fc41aa2d6@linux.ibm.com>
Date: Mon, 23 Sep 2024 10:16:14 +0200
From: Wenjia Zhang <wenjia@...ux.ibm.com>
To: Cong Wang <xiyou.wangcong@...il.com>,
        "D. Wythe"
 <alibuda@...ux.alibaba.com>,
        Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, Cong Wang <cong.wang@...edance.com>,
        syzbot+c75d1de73d3b8b76272f@...kaller.appspotmail.com,
        Jan Karcher <jaka@...ux.ibm.com>, Tony Lu <tonylu@...ux.alibaba.com>,
        Wen Gu <guwen@...ux.alibaba.com>
Subject: Re: [Patch net] smc: use RCU version of lower netdev searching



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 <cong.wang@...edance.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@...kaller.appspotmail.com
>>>>>>>> Closes: 
>>>>>>>> https://syzkaller.appspot.com/bug?extid=c75d1de73d3b8b76272f
>>>>>>>> Cc: Wenjia Zhang <wenjia@...ux.ibm.com>
>>>>>>>> Cc: Jan Karcher <jaka@...ux.ibm.com>
>>>>>>>> Cc: "D. Wythe" <alibuda@...ux.alibaba.com>
>>>>>>>> Cc: Tony Lu <tonylu@...ux.alibaba.com>
>>>>>>>> Cc: Wen Gu <guwen@...ux.alibaba.com>
>>>>>>>> Signed-off-by: Cong Wang <cong.wang@...edance.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
> 
> 

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

Powered by blists - more mailing lists