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

[Stanislav Fomichev <stfomichev@gmail.com>]

On 02/21, Sabrina Dubroca wrote:
> 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)

Hmm, and this workqueue work doesn't grab rtnl_lock, interesting..
Let me add netdev_lock_ops just in case, won't hurt.

> > > (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.

SG! I did try to to do this initially (adding extra locking, then
removing extra locking), but it seemed to only make everything more
difficult to review (imo), so I reverted to a simplified way :-)