[patch net] dpll: fix dpll_xa_ref_*_del() for multiple registrations

[patch net] dpll: fix dpll_xa_ref_*_del() for multiple registrations

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Currently, if there are multiple registrations of the same pin on the
same dpll device, following warnings are observed:
WARNING: CPU: 5 PID: 2212 at drivers/dpll/dpll_core.c:143 dpll_xa_ref_pin_del.isra.0+0x21e/0x230
WARNING: CPU: 5 PID: 2212 at drivers/dpll/dpll_core.c:223 __dpll_pin_unregister+0x2b3/0x2c0

The problem is, that in both dpll_xa_ref_dpll_del() and
dpll_xa_ref_pin_del() registration is only removed from list in case the
reference count drops to zero. That is wrong, the registration has to
be removed always.

To fix this, remove the registration from the list and free
it unconditionally, instead of doing it only when the ref reference
counter reaches zero.

Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/dpll/dpll_core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
index 7f686d179fc9..c751a87c7a8e 100644
--- a/drivers/dpll/dpll_core.c
+++ b/drivers/dpll/dpll_core.c
@@ -129,9 +129,9 @@ static int dpll_xa_ref_pin_del(struct xarray *xa_pins, struct dpll_pin *pin,
 		reg = dpll_pin_registration_find(ref, ops, priv);
 		if (WARN_ON(!reg))
 			return -EINVAL;
+		list_del(&reg->list);
+		kfree(reg);
 		if (refcount_dec_and_test(&ref->refcount)) {
-			list_del(&reg->list);
-			kfree(reg);
 			xa_erase(xa_pins, i);
 			WARN_ON(!list_empty(&ref->registration_list));
 			kfree(ref);
@@ -209,9 +209,9 @@ dpll_xa_ref_dpll_del(struct xarray *xa_dplls, struct dpll_device *dpll,
 		reg = dpll_pin_registration_find(ref, ops, priv);
 		if (WARN_ON(!reg))
 			return;
+		list_del(&reg->list);
+		kfree(reg);
 		if (refcount_dec_and_test(&ref->refcount)) {
-			list_del(&reg->list);
-			kfree(reg);
 			xa_erase(xa_dplls, i);
 			WARN_ON(!list_empty(&ref->registration_list));
 			kfree(ref);
-- 
2.43.2

Re: [patch net] dpll: fix dpll_xa_ref_*_del() for multiple registrations

[Rahul Rameshbabu]

On Wed, 06 Mar, 2024 16:12:40 +0100 Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@nvidia.com>
>
> Currently, if there are multiple registrations of the same pin on the
> same dpll device, following warnings are observed:
> WARNING: CPU: 5 PID: 2212 at drivers/dpll/dpll_core.c:143 dpll_xa_ref_pin_del.isra.0+0x21e/0x230
> WARNING: CPU: 5 PID: 2212 at drivers/dpll/dpll_core.c:223 __dpll_pin_unregister+0x2b3/0x2c0
>
> The problem is, that in both dpll_xa_ref_dpll_del() and
> dpll_xa_ref_pin_del() registration is only removed from list in case the
> reference count drops to zero. That is wrong, the registration has to
> be removed always.

What about the case where you have two functions/netdevs that refer to
the same DPLL device/pin and you only remove a single function? You have
another function/netdev left that now refers to the unregistered DPLL
device/pin.

>
> To fix this, remove the registration from the list and free
> it unconditionally, instead of doing it only when the ref reference
> counter reaches zero.
>
> Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
>  drivers/dpll/dpll_core.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
> index 7f686d179fc9..c751a87c7a8e 100644
> --- a/drivers/dpll/dpll_core.c
> +++ b/drivers/dpll/dpll_core.c
> @@ -129,9 +129,9 @@ static int dpll_xa_ref_pin_del(struct xarray *xa_pins, struct dpll_pin *pin,
>  		reg = dpll_pin_registration_find(ref, ops, priv);
>  		if (WARN_ON(!reg))
>  			return -EINVAL;
> +		list_del(&reg->list);
> +		kfree(reg);
>  		if (refcount_dec_and_test(&ref->refcount)) {
> -			list_del(&reg->list);
> -			kfree(reg);
>  			xa_erase(xa_pins, i);
>  			WARN_ON(!list_empty(&ref->registration_list));
>  			kfree(ref);
> @@ -209,9 +209,9 @@ dpll_xa_ref_dpll_del(struct xarray *xa_dplls, struct dpll_device *dpll,
>  		reg = dpll_pin_registration_find(ref, ops, priv);
>  		if (WARN_ON(!reg))
>  			return;
> +		list_del(&reg->list);
> +		kfree(reg);
>  		if (refcount_dec_and_test(&ref->refcount)) {
> -			list_del(&reg->list);
> -			kfree(reg);
>  			xa_erase(xa_dplls, i);
>  			WARN_ON(!list_empty(&ref->registration_list));
>  			kfree(ref);

Re: [patch net] dpll: fix dpll_xa_ref_*_del() for multiple registrations

[Rahul Rameshbabu]

On Wed, 06 Mar, 2024 07:18:35 -0800 Rahul Rameshbabu <rrameshbabu@nvidia.com> wrote:
> On Wed, 06 Mar, 2024 16:12:40 +0100 Jiri Pirko <jiri@resnulli.us> wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>>
>> Currently, if there are multiple registrations of the same pin on the
>> same dpll device, following warnings are observed:
>> WARNING: CPU: 5 PID: 2212 at drivers/dpll/dpll_core.c:143 dpll_xa_ref_pin_del.isra.0+0x21e/0x230
>> WARNING: CPU: 5 PID: 2212 at drivers/dpll/dpll_core.c:223 __dpll_pin_unregister+0x2b3/0x2c0
>>
>> The problem is, that in both dpll_xa_ref_dpll_del() and
>> dpll_xa_ref_pin_del() registration is only removed from list in case the
>> reference count drops to zero. That is wrong, the registration has to
>> be removed always.
>
> What about the case where you have two functions/netdevs that refer to
> the same DPLL device/pin and you only remove a single function? You have
> another function/netdev left that now refers to the unregistered DPLL
> device/pin.
>
Actually, I see that being registered or not does not impact the use of
existing DPLL device/pin references in other functions. I agree with
this change.
>>
>> To fix this, remove the registration from the list and free
>> it unconditionally, instead of doing it only when the ref reference
>> counter reaches zero.
>>
>> Fixes: 9431063ad323 ("dpll: core: Add DPLL framework base functions")
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>>  drivers/dpll/dpll_core.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>> index 7f686d179fc9..c751a87c7a8e 100644
>> --- a/drivers/dpll/dpll_core.c
>> +++ b/drivers/dpll/dpll_core.c
>> @@ -129,9 +129,9 @@ static int dpll_xa_ref_pin_del(struct xarray *xa_pins, struct dpll_pin *pin,
>>  		reg = dpll_pin_registration_find(ref, ops, priv);
>>  		if (WARN_ON(!reg))
>>  			return -EINVAL;
>> +		list_del(&reg->list);
>> +		kfree(reg);
>>  		if (refcount_dec_and_test(&ref->refcount)) {
>> -			list_del(&reg->list);
>> -			kfree(reg);
>>  			xa_erase(xa_pins, i);
>>  			WARN_ON(!list_empty(&ref->registration_list));
>>  			kfree(ref);
>> @@ -209,9 +209,9 @@ dpll_xa_ref_dpll_del(struct xarray *xa_dplls, struct dpll_device *dpll,
>>  		reg = dpll_pin_registration_find(ref, ops, priv);
>>  		if (WARN_ON(!reg))
>>  			return;
>> +		list_del(&reg->list);
>> +		kfree(reg);
>>  		if (refcount_dec_and_test(&ref->refcount)) {
>> -			list_del(&reg->list);
>> -			kfree(reg);
>>  			xa_erase(xa_dplls, i);
>>  			WARN_ON(!list_empty(&ref->registration_list));
>>  			kfree(ref);

Reviewed-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>

Re: [patch net] dpll: fix dpll_xa_ref_*_del() for multiple registrations

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed,  6 Mar 2024 16:12:40 +0100 you wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Currently, if there are multiple registrations of the same pin on the
> same dpll device, following warnings are observed:
> WARNING: CPU: 5 PID: 2212 at drivers/dpll/dpll_core.c:143 dpll_xa_ref_pin_del.isra.0+0x21e/0x230
> WARNING: CPU: 5 PID: 2212 at drivers/dpll/dpll_core.c:223 __dpll_pin_unregister+0x2b3/0x2c0
> 
> [...]

Here is the summary with links:
  - [net] dpll: fix dpll_xa_ref_*_del() for multiple registrations
    https://git.kernel.org/netdev/net/c/b446631f355e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html