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

[Eric Dumazet]

On Thu, Dec 19, 2024 at 5:28 PM Shinas Rasheed <srasheed@xxxxxxxxxxx> wrote:
>
> Hi Eric,
>
> > On Wed, Dec 18, 2024 at 4:25 PM Shinas Rasheed <srasheed@xxxxxxxxxxx>
> > 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.

A precise lock is better, it is easy to grep, if really other drivers
were not able to fix this in a different way.

A synchronize_net() without explicit details is very very weak.

>
> Can we please comment further on the v3 of this patch? https://lore.kernel.org/all/20241218115111.2407958-1-srasheed@xxxxxxxxxxx/
>
> Thanks a lot for your time and comments

I tried to say that adding a synchronize_net() call in ndo_stop() was
not needed.

I do not particularly care for this driver, my concern is that
copy/pasting is going to happen.