netdev - Re: [V2 net 05/11] net/mlx5: Fix possible deadlock on mlx5e_tx_timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Y2qZnAv0PBTb2kQ4@x130.lan>
Date:   Tue, 8 Nov 2022 10:02:04 -0800
From:   Saeed Mahameed <saeed@...nel.org>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Saeed Mahameed <saeedm@...dia.com>, netdev@...r.kernel.org,
        Tariq Toukan <tariqt@...dia.com>,
        Moshe Shemesh <moshe@...dia.com>
Subject: Re: [V2 net 05/11] net/mlx5: Fix possible deadlock on
 mlx5e_tx_timeout_work

On 08 Nov 11:19, Paolo Abeni wrote:
>On Mon, 2022-11-07 at 20:24 -0800, Jakub Kicinski wrote:
>> On Sat,  5 Nov 2022 00:10:22 -0700 Saeed Mahameed wrote:
>> > +	/* Once deactivated, new tx_timeout_work won't be initiated. */
>> > +	if (current_work() != &priv->tx_timeout_work)
>> > +		cancel_work_sync(&priv->tx_timeout_work);
>>
>> The work takes rtnl_lock, are there no callers of
>> mlx5e_switch_priv_channels() that are under rtnl_lock()?
>>
>> This patch is definitely going onto my "expecting Fixes"
>> bingo card :S
>
>I think Jakub is right and even mlx5e_close_locked() will deadlock on
>cancel_work_sync() if the work is scheduled but it has not yet acquired
>the rtnl lock.

Yes you are absolutely correct, you can see the deadlock just by looking at
the patch diff and applying common sense that mlx5e_switch_priv_channels()
is being called under rtnl.

>
>IIRC lockdep is not able to catch this kind of situation, so you can
>only observe the deadlock when reaching the critical scenario.
>
>I'm wild guessing than a possible solution would be restrict the
>state_lock scope in mlx5e_tx_timeout_work() around the state check,
>without additional cancel_work operations.
>

Thanks, i will drop the patch for now and send v3 without it.

>Thanks,
>
>Paolo
>