netdev - RE: [PATCH net 2/2] ice: Fix call trace with null VSI during VF reset

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <PH7PR11MB5819695C8B5C6E5F7D0500E1996A9@PH7PR11MB5819.namprd11.prod.outlook.com>
Date:   Wed, 17 Aug 2022 13:31:00 +0000
From:   "Sokolowski, Jan" <jan.sokolowski@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "Jaron, MichalX" <michalx.jaron@...el.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Jagielski, Jedrzej" <jedrzej.jagielski@...el.com>,
        "Szlosek, Marek" <marek.szlosek@...el.com>
Subject: RE: [PATCH net 2/2] ice: Fix call trace with null VSI during VF reset

I'd like to send this response in Michal Jaron's name, as he currently cannot respond to this email.

Generally you are right, it is better idea to try to keep the system in a consistent state than adding "if NULL return;" but I don't think it will work here. This "if NULL return;" is here because of race between two resets and I don't think we can guarantee that this race will be not present if we flush the service work before reset. The problem is that vf reset is called in the same time from vf on vm and from pf. When reset from vf is called and reset form pf don't clear rings yet we must go into ice_reset_vf and clear those rings without triggering second reset. If we don't clear rings there we may trigger page_frag_cache_drain crash related to writing data to unmapped queues. In such cases if there are no vsis we don't need to do this and this WARN_ON is not necessary, but we need to check it anyway.

-----Original Message-----
From: Jakub Kicinski <kuba@...nel.org> 
Sent: Saturday, August 13, 2022 2:13 AM
To: Nguyen, Anthony L <anthony.l.nguyen@...el.com>
Cc: davem@...emloft.net; pabeni@...hat.com; edumazet@...gle.com; Jaron, MichalX <michalx.jaron@...el.com>; netdev@...r.kernel.org; Jagielski, Jedrzej <jedrzej.jagielski@...el.com>; Szlosek, Marek <marek.szlosek@...el.com>
Subject: Re: [PATCH net 2/2] ice: Fix call trace with null VSI during VF reset

On Thu, 11 Aug 2022 09:17:14 -0700 Tony Nguyen wrote:
> This WARN_ON() is unnecessary and causes call trace, despite that
> call trace, driver still works. There is no need for this warn
> because this piece of code is responsible for disabling VF's Tx/Rx
> queues when VF is disabled, but when VF is already removed there
> is no need to do reset or disable queues.

Can't you flush the service work when disabling VFs instead?
Seems better to try to keep the system in a consistent state
than add "if NULL return;" in random places :S