* [PATCH] RDMA: Fix netdev tracker in ib_device_set_netdev
@ 2024-07-09 21:44 David Ahern
2024-07-10 6:09 ` Leon Romanovsky
0 siblings, 1 reply; 5+ messages in thread
From: David Ahern @ 2024-07-09 21:44 UTC (permalink / raw)
To: linux-rdma; +Cc: jgg, leon, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] RDMA: Fix netdev tracker in ib_device_set_netdev
2024-07-09 21:44 [PATCH] RDMA: Fix netdev tracker in ib_device_set_netdev David Ahern
@ 2024-07-10 6:09 ` Leon Romanovsky
2024-07-10 17:59 ` David Ahern
0 siblings, 1 reply; 5+ messages in thread
From: Leon Romanovsky @ 2024-07-10 6:09 UTC (permalink / raw)
To: David Ahern; +Cc: linux-rdma, jgg
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] RDMA: Fix netdev tracker in ib_device_set_netdev
2024-07-10 6:09 ` Leon Romanovsky
@ 2024-07-10 17:59 ` David Ahern
2024-07-11 10:09 ` Leon Romanovsky
2024-07-11 19:50 ` Jason Gunthorpe
0 siblings, 2 replies; 5+ messages in thread
From: David Ahern @ 2024-07-10 17:59 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: linux-rdma, jgg
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] RDMA: Fix netdev tracker in ib_device_set_netdev
2024-07-10 17:59 ` David Ahern
@ 2024-07-11 10:09 ` Leon Romanovsky
2024-07-11 19:50 ` Jason Gunthorpe
1 sibling, 0 replies; 5+ messages in thread
From: Leon Romanovsky @ 2024-07-11 10:09 UTC (permalink / raw)
To: David Ahern; +Cc: linux-rdma, jgg
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] RDMA: Fix netdev tracker in ib_device_set_netdev
2024-07-10 17:59 ` David Ahern
2024-07-11 10:09 ` Leon Romanovsky
@ 2024-07-11 19:50 ` Jason Gunthorpe
1 sibling, 0 replies; 5+ messages in thread
From: Jason Gunthorpe @ 2024-07-11 19:50 UTC (permalink / raw)
To: David Ahern; +Cc: Leon Romanovsky, linux-rdma
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-07-11 19:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-09 21:44 [PATCH] RDMA: Fix netdev tracker in ib_device_set_netdev David Ahern
2024-07-10 6:09 ` Leon Romanovsky
2024-07-10 17:59 ` David Ahern
2024-07-11 10:09 ` Leon Romanovsky
2024-07-11 19:50 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).