Re: [PATCH net-next v5 03/12] net: hold netdev instance lock during queue operations

[Sabrina Dubroca <sd@queasysnail.net>]

2025-02-20, 09:00:24 -0800, Stanislav Fomichev wrote:
> On 02/20, Sabrina Dubroca wrote:
> > 2025-02-19, 12:27:10 -0800, Stanislav Fomichev wrote:
> > > diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> > > index 533e659b15b3..cf9bd08d04b2 100644
> > > --- a/drivers/net/ethernet/google/gve/gve_main.c
> > > +++ b/drivers/net/ethernet/google/gve/gve_main.c
> > > @@ -1886,7 +1886,7 @@ static void gve_turndown(struct gve_priv *priv)
> > >  			netif_queue_set_napi(priv->dev, idx,
> > >  					     NETDEV_QUEUE_TYPE_TX, NULL);
> > >  
> > > -		napi_disable(&block->napi);
> > > +		napi_disable_locked(&block->napi);
> > 
> > I don't think all the codepaths that can lead to gve_turndown have the
> > required netdev_lock():
> > 
> > gve_resume -> gve_reset_recovery -> gve_turndown
> Good catch, looks like suspend is missing the netdev lock as well, will
> add.
> 
> > gve_user_reset -> gve_reset -> gve_reset_recovery
> I believe this should be covered by patch "net: ethtool: try to protect
> all callback with netdev instance lock", no?
> 
> __dev_ethtool
>   netdev_lock_ops
>   ethtool_reset
>     gve_user_reset

Ah, right, sorry, I missed that.

> Or is there some other reset path I'm missing?

Looking at net/ethtool, maybe cmis_fw_update_reset?
module_flash_fw_work -> ethtool_cmis_fw_update -> cmis_fw_update_reset -> ->reset()

(no idea if it can ever be called for those drivers)

> > (and nit:) There's also a few places in the series (bnxt, ethtool
> > calling __netdev_update_features) where the lockdep
> > annotation/_locked() variant gets introduced before the patch adding
> > the corresponding lock.
> 
> This is mostly about ethtool patch and queue ops patch?

Patch 04 also adds a lockdep annotation to __netdev_update_features,
which gets call (unlocked until the ethtool patch) from ethtool.

> The latter
> converts most of the napi/netif calls to _locked variant leaving
> a small window where some of the paths might be not properly locked.
> Not sure what to do about it, but probably nothing since everything
> is still rtnl_lock-protected and the issue is mostly about (temporary)
> wrong lockdep annotations?

Yes, it's temporary (I didn't check the final bnxt patch to see if it
covers all paths).

> Any other suggestions?

I guess the alternative would be introducing netdev_lock where it
belongs before adding the lockdep annotations/switching to _locked()
variants.

Maybe it's not worth the pain of reworking this patchset if it ends up
in the correct state anyway, I don't know. Probably more a question
for the maintainers, depending on what they prefer.

> Thanks for the review!

Thanks

-- 
Sabrina