RE: [EXTERNAL] Re: [PATCH net-next v2 04/13] net: mana: swap queue sets in mana_set_priv_flags

> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> 
> Pre-existing issues:
> - [High] The boolean flag `apc->channel_changing` is used as an open-coded,
> ad-hoc lock to protect vport reconfiguration against concurrent RDMA access,
> violating core kernel locking rules.

This patch set doesn't change the locking mechanism used by upstream code.

Long

> --
> 
> --- Patch [4]: [PATCH net-next v2 04/13] net: mana: swap queue sets in
> mana_set_priv_flags --- commit
> 922132e276d163d3c58e43d88b85f6b85a403949
> Author: Long Li <[email protected]>
> 
> net: mana: swap queue sets in mana_set_priv_flags
> 
> This commit modifies mana_set_priv_flags to pre-allocate and swap queue
> sets instead of rebuilding them in place. This ensures that if the rebuild fails,
> both the queues and the flag word remain untouched.
> 
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> [ ... ]
> > @@ -884,54 +888,53 @@ static int mana_set_priv_flags(struct net_device
> > *ndev, u32 priv_flags)
> [ ... ]
> > -		/* Block RDMA from grabbing the vport during detach/attach
> */
> > -		mutex_lock(&apc->vport_mutex);
> > -		apc->channel_changing = true;
> > +	/* Block RDMA from acquiring the vport for the duration. */
> > +	mutex_lock(&apc->vport_mutex);
> > +	if (apc->channel_changing) {
> >  		mutex_unlock(&apc->vport_mutex);
> > +		return -EBUSY;
> > +	}
> > +	apc->channel_changing = true;
> > +	mutex_unlock(&apc->vport_mutex);
> 
> [Severity: High]
> This is a pre-existing issue, but in mana_set_priv_flags, is using a boolean
> flag like apc->channel_changing as an open-coded trylock safe?
> 
> It appears to be set on entry here and cleared on exit to protect vport
> reconfiguration against concurrent RDMA access, rather than using a
> standard synchronization primitive. Does this ad-hoc locking pattern evade
> lockdep and risk races since it bypasses proper kernel locking rules?
> 
> [ ... ]
> > clear_flag:
> >  	mutex_lock(&apc->vport_mutex);
> >  	apc->channel_changing = false;
> >  	mutex_unlock(&apc->vport_mutex);
> > -
> >  	return err;
> >  }
> 
> --
> Sashiko AI
> review * https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%
> 2Fsashiko.dev%2F%23%2Fpatchset%2F20260813050418.2906468-1-
> longli%40microsoft.com%3Fpart%3D4&data=05%7C02%7Clongli%40microsoft.
> com%7C12b5de9dce024bffb15908def9c1a4bb%7C72f988bf86f141af91ab2d7c
> d011db47%7C1%7C0%7C639222807267503180%7CUnknown%7CTWFpbGZsb
> 3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkF
> OIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=mmPoxVwzW%2F
> 5aQYtA63m5ts8vN8Y3bM45YQmyRwYiTLA%3D&reserved=0