netdev - RE: [EXTERNAL] Re: [PATCH net v2 1/4] octeon_ep: fix race conditions in ndo_get

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:
 <CO1PR18MB472962C9345E15B8F1988E25C7062@CO1PR18MB4729.namprd18.prod.outlook.com>
Date: Thu, 19 Dec 2024 16:28:21 +0000
From: Shinas Rasheed <srasheed@...vell.com>
To: Eric Dumazet <edumazet@...gle.com>
CC: Larysa Zaremba <larysa.zaremba@...el.com>,
        "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        Haseeb Gani <hgani@...vell.com>, Sathesh B
 Edara <sedara@...vell.com>,
        Vimlesh Kumar <vimleshk@...vell.com>,
        "thaller@...hat.com" <thaller@...hat.com>,
        "wizhao@...hat.com"
	<wizhao@...hat.com>,
        "kheib@...hat.com" <kheib@...hat.com>,
        "konguyen@...hat.com" <konguyen@...hat.com>,
        "horms@...nel.org"
	<horms@...nel.org>,
        "einstein.xue@...axg.com" <einstein.xue@...axg.com>,
        Veerasenareddy Burru <vburru@...vell.com>,
        Andrew Lunn
	<andrew+netdev@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub
 Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
        Abhijit Ayarekar
	<aayarekar@...vell.com>,
        Satananda Burla <sburla@...vell.com>
Subject: RE: [EXTERNAL] Re: [PATCH net v2 1/4] octeon_ep: fix race conditions
 in ndo_get_stats64

Hi Eric,

> On Wed, Dec 18, 2024 at 4:25 PM Shinas Rasheed <srasheed@...vell.com>
> wrote:
> 
> > Hi Eric,
> >
> > This patch is not a workaround. In some setups, we were seeing races with
> regards
> > to resource freeing between ndo_stop() and ndo_get_stats(). Hence to sync
> with the view of
> > resources, a synchronize_net() is called in ndo_stop(). Please let me know if
> you see anything wrong here.
> 
> We do not add a synchronize_net() without a very strong explanation
> (details, not a weak sentence in the changelog).
> 
> Where is the opposite barrier in your patch ?
> 
> I am saying you do not need this, unless you can show evidence.
> 
> If your ndo_get_stats() needs to call netif_running(), this would be
> the fix IMO.

The synchronize_net() is supposed to sync all previous calls of ndo_get_stats() and wait for their completion before closing the device.
Again this seems to be the v2 of this patch. In the v3, I have provided the warn log as well in the commit message for reference, in answer to
the changelog comment for more clarification.

As I stated, this is needed because ndo_stop() races with ndo_get_stats(), and a 'lock' or a similar mechanism seems required to alleviate this.
Fixes in the same vein seem to be common, as I do see other drivers utilizing a lock mechanism while retrieving statistics to resolve the same 
(ie; race with resource destruction in ndo_stop()). So, just to state, I'm not trying to do anything new
here.

Can we please comment further on the v3 of this patch? https://lore.kernel.org/all/20241218115111.2407958-1-srasheed@marvell.com/

Thanks a lot for your time and comments