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:
 <BY3PR18MB472105E5D09B8FE018DBFC15C7052@BY3PR18MB4721.namprd18.prod.outlook.com>
Date: Wed, 18 Dec 2024 15:25:13 +0000
From: Shinas Rasheed <srasheed@...vell.com>
To: Eric Dumazet <edumazet@...gle.com>,
        Larysa Zaremba
	<larysa.zaremba@...el.com>
CC: "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


> > On Wed, Dec 18, 2024 at 03:21:12PM +0100, Eric Dumazet wrote:
> > > On Wed, Dec 18, 2024 at 2:25 PM Larysa Zaremba
> <larysa.zaremba@...el.com> wrote:
> > >
> > > >
> > > > It is hard to know without testing (but testing should not be hard). I
> think the
> > > > phrase "Statistics must persist across routine operations like bringing the
> > > > interface down and up." [0] implies that bringing the interface down
> may not
> > > > necessarily prevent stats calls.
> > >
> > > Please don't  add workarounds to individual drivers.
> > >
> > > I think the core networking stack should handle the possible races.
> > >
> > > Most dev_get_stats() callers are correctly testing dev_isalive() or
> > > are protected by RTNL.
> > >
> > > There are few nested cases that are not properly handled, the
> > > following patch should take care of them.
> > >
> >
> > I was under the impression that .ndo_stop() being called does not mean the
> > device stops being NETREG_REGISTERED, such link would be required to
> solve the
> > original problem  with your patch alone (though it is generally a good
> change).
> > Could you please explain this relation?
> >
> 
> ndo_stop() being called must have no impact on statistics :
> 
> # ip -s link sh dev lo
> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN
> mode DEFAULT group default qlen 1000
>     link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>     RX:  bytes packets errors dropped  missed   mcast
>        3473568   41352      0       0       0       0
>     TX:  bytes packets errors dropped carrier collsns
>        3473568   41352      0       0       0       0
> 
> # ip link set dev lo down  # would call ndo_stop() if loopback had one
> 
> # ip -s link sh dev lo
> 1: lo: <LOOPBACK> mtu 65536 qdisc noqueue state DOWN mode DEFAULT
> group default qlen 1000
>     link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>     RX:  bytes packets errors dropped  missed   mcast
>        3473568   41352      0       0       0       0
>     TX:  bytes packets errors dropped carrier collsns
>        3473568   41352      0       0       0       0
> 
> 
> So perhaps the problem with this driver is that its ndo_stop() is
> doing things it should not.

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.

Thanks