[PATCH] RDMA: Fix netdev tracker in ib_device_set_netdev

[PATCH] RDMA: Fix netdev tracker in ib_device_set_netdev

[David Ahern]

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);
 	if (ndev)
 		netdev_hold(ndev, &pdata->netdev_tracker, GFP_ATOMIC);
 	rcu_assign_pointer(pdata->netdev, ndev);
-- 
2.30.2

Re: [PATCH] RDMA: Fix netdev tracker in ib_device_set_netdev

[Leon Romanovsky]

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

Re: [PATCH] RDMA: Fix netdev tracker in ib_device_set_netdev

[David Ahern]

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.

Re: [PATCH] RDMA: Fix netdev tracker in ib_device_set_netdev

[Leon Romanovsky]

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

Re: [PATCH] RDMA: Fix netdev tracker in ib_device_set_netdev

[Jason Gunthorpe]

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