Re: [PATCH net] netlink: fix potential deadlock in netlink_set_err()

[Jiri Pirko <jiri@resnulli.us>]

Thu, Jun 22, 2023 at 10:14:56AM CEST, edumazet@google.com wrote:
>On Thu, Jun 22, 2023 at 10:04 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Wed, Jun 21, 2023 at 05:43:37PM CEST, edumazet@google.com wrote:
>> >syzbot reported a possible deadlock in netlink_set_err() [1]
>> >
>> >A similar issue was fixed in commit 1d482e666b8e ("netlink: disable IRQs
>> >for netlink_lock_table()") in netlink_lock_table()
>> >
>> >This patch adds IRQ safety to netlink_set_err() and __netlink_diag_dump()
>> >which were not covered by cited commit.
>> >
>> >[1]
>> >
>> >WARNING: possible irq lock inversion dependency detected
>> >6.4.0-rc6-syzkaller-00240-g4e9f0ec38852 #0 Not tainted
>> >
>> >syz-executor.2/23011 just changed the state of lock:
>> >ffffffff8e1a7a58 (nl_table_lock){.+.?}-{2:2}, at: netlink_set_err+0x2e/0x3a0 net/netlink/af_netlink.c:1612
>> >but this lock was taken by another, SOFTIRQ-safe lock in the past:
>> > (&local->queue_stop_reason_lock){..-.}-{2:2}
>> >
>> >and interrupts could create inverse lock ordering between them.
>> >
>> >other info that might help us debug this:
>> > Possible interrupt unsafe locking scenario:
>> >
>> >       CPU0                    CPU1
>> >       ----                    ----
>> >  lock(nl_table_lock);
>> >                               local_irq_disable();
>> >                               lock(&local->queue_stop_reason_lock);
>> >                               lock(nl_table_lock);
>> >  <Interrupt>
>> >    lock(&local->queue_stop_reason_lock);
>> >
>> > *** DEADLOCK ***
>> >
>> >Fixes: 1d482e666b8e ("netlink: disable IRQs for netlink_lock_table()")
>>
>> I don't think that this "fixes" tag is correct. The referenced commit
>> is a fix to the same issue on a different codepath, not the one who
>> actually introduced the issue.
>>
>> The code itself looks fine to me.
>
>Note that the 1d482e666b8e had no Fixes: tag, otherwise I would have taken it.

I'm aware it didn't. But that does not implicate this patch should have
that commit as a "Fixes:" tag. Either have the correct one pointing out
which commit introduced the issue or omit the "Fixes:" tag entirely.
That's my point.


>
>I presume that it would make no sense to backport my patch on stable branches
>if the cited commit was not backported yet.
>
>Now, if you think we can be more precise, I will let Johannes do the
>archeology in ieee80211 code.