| Message ID | 20240709214455.17823-1-dsahern@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RDMA: Fix netdev tracker in ib_device_set_netdev | expand |
On Tue, Jul 09, 2024 at 03:44:55PM -0600, David Ahern wrote: > If a netdev has already been assigned, ib_device_set_netdev needs to release > the reference on the older but it is mistakenly being called for the new > netdev. Fix it and in the process use netdev_put to be symmetrical with > the netdev_hold. > > Fixes: 09f530f0c6d6 ("RDMA: Add netdevice_tracker to ib_device_set_netdev()") > Signed-off-by: David Ahern <dsahern@kernel.org> > --- > drivers/infiniband/core/device.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c > index 55aa7aa32d4a..7ddaec923569 100644 > --- a/drivers/infiniband/core/device.c > +++ b/drivers/infiniband/core/device.c > @@ -2167,7 +2167,7 @@ int ib_device_set_netdev(struct ib_device *ib_dev, struct net_device *ndev, > } > > if (old_ndev) > - netdev_tracker_free(ndev, &pdata->netdev_tracker); > + netdev_put(old_ndev, &pdata->netdev_tracker); It should stay netdev_tracker_free() and not netdev_put(). We are calling to __dev_put(old_ndev) later in the function. Thanks > if (ndev) > netdev_hold(ndev, &pdata->netdev_tracker, GFP_ATOMIC); > rcu_assign_pointer(pdata->netdev, ndev); > -- > 2.30.2
On 7/10/24 12:09 AM, Leon Romanovsky wrote: >> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c >> index 55aa7aa32d4a..7ddaec923569 100644 >> --- a/drivers/infiniband/core/device.c >> +++ b/drivers/infiniband/core/device.c >> @@ -2167,7 +2167,7 @@ int ib_device_set_netdev(struct ib_device *ib_dev, struct net_device *ndev, >> } >> >> if (old_ndev) >> - netdev_tracker_free(ndev, &pdata->netdev_tracker); >> + netdev_put(old_ndev, &pdata->netdev_tracker); > > It should stay netdev_tracker_free() and not netdev_put(). We are > calling to __dev_put(old_ndev) later in the function. > missed that and KASAN and refcount debugging did not complain ... Anyways, why have the 2 split apart? ie., why not remove the __dev_put and just do netdev_put here? old_ndev is not needed in between calls. Asymmetric calls like this are always confusing.
On Wed, Jul 10, 2024 at 10:59:15AM -0700, David Ahern wrote: > On 7/10/24 12:09 AM, Leon Romanovsky wrote: > >> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c > >> index 55aa7aa32d4a..7ddaec923569 100644 > >> --- a/drivers/infiniband/core/device.c > >> +++ b/drivers/infiniband/core/device.c > >> @@ -2167,7 +2167,7 @@ int ib_device_set_netdev(struct ib_device *ib_dev, struct net_device *ndev, > >> } > >> > >> if (old_ndev) > >> - netdev_tracker_free(ndev, &pdata->netdev_tracker); > >> + netdev_put(old_ndev, &pdata->netdev_tracker); > > > > It should stay netdev_tracker_free() and not netdev_put(). We are > > calling to __dev_put(old_ndev) later in the function. > > > > missed that and KASAN and refcount debugging did not complain ... > > Anyways, why have the 2 split apart? ie., why not remove the __dev_put > and just do netdev_put here? old_ndev is not needed in between calls. > Asymmetric calls like this are always confusing. You probably can combine them, but to do so instead of __dev_put() and not netdev_tracker_free(). Thanks
On Wed, Jul 10, 2024 at 10:59:15AM -0700, David Ahern wrote: > On 7/10/24 12:09 AM, Leon Romanovsky wrote: > >> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c > >> index 55aa7aa32d4a..7ddaec923569 100644 > >> --- a/drivers/infiniband/core/device.c > >> +++ b/drivers/infiniband/core/device.c > >> @@ -2167,7 +2167,7 @@ int ib_device_set_netdev(struct ib_device *ib_dev, struct net_device *ndev, > >> } > >> > >> if (old_ndev) > >> - netdev_tracker_free(ndev, &pdata->netdev_tracker); > >> + netdev_put(old_ndev, &pdata->netdev_tracker); > > > > It should stay netdev_tracker_free() and not netdev_put(). We are > > calling to __dev_put(old_ndev) later in the function. > > > > missed that and KASAN and refcount debugging did not complain ... > > Anyways, why have the 2 split apart? ie., why not remove the __dev_put > and just do netdev_put here? old_ndev is not needed in between calls. > Asymmetric calls like this are always confusing. Maybe it is Ok like this: @@ -2166,15 +2166,14 @@ int ib_device_set_netdev(struct ib_device *ib_dev, struct net_device *ndev, return 0; } + rcu_assign_pointer(pdata->netdev, ndev); if (old_ndev) - netdev_tracker_free(ndev, &pdata->netdev_tracker); + netdev_put(old_ndev, &pdata->netdev_tracker); if (ndev) netdev_hold(ndev, &pdata->netdev_tracker, GFP_ATOMIC); - rcu_assign_pointer(pdata->netdev, ndev); spin_unlock_irqrestore(&pdata->netdev_lock, flags); add_ndev_hash(pdata); - __dev_put(old_ndev); return 0; } Don't like that we drop the ref and leave the pdata->netdev assigned to something with no ref, even though it is OK for RCU reasons.. Jason
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c index 55aa7aa32d4a..7ddaec923569 100644 --- a/drivers/infiniband/core/device.c +++ b/drivers/infiniband/core/device.c @@ -2167,7 +2167,7 @@ int ib_device_set_netdev(struct ib_device *ib_dev, struct net_device *ndev, } if (old_ndev) - netdev_tracker_free(ndev, &pdata->netdev_tracker); + netdev_put(old_ndev, &pdata->netdev_tracker); if (ndev) netdev_hold(ndev, &pdata->netdev_tracker, GFP_ATOMIC); rcu_assign_pointer(pdata->netdev, ndev);
If a netdev has already been assigned, ib_device_set_netdev needs to release the reference on the older but it is mistakenly being called for the new netdev. Fix it and in the process use netdev_put to be symmetrical with the netdev_hold. Fixes: 09f530f0c6d6 ("RDMA: Add netdevice_tracker to ib_device_set_netdev()") Signed-off-by: David Ahern <dsahern@kernel.org> --- drivers/infiniband/core/device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)