Re: [EXTERNAL] Re: [PATCH net v2 1/4] octeon_ep: fix race conditions in ndo_get_stats64

[Eric Dumazet]

On Wed, Dec 18, 2024 at 3:49 PM Larysa Zaremba <larysa.zaremba@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxx> 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.