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