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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 06 Mar 2024 07:38:07 -0800
From: Rahul Rameshbabu <rrameshbabu@...dia.com>
To: Rahul Rameshbabu <rrameshbabu@...dia.com>
Cc: Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org,
 arkadiusz.kubalewski@...el.com, vadim.fedorenko@...ux.dev,
 milena.olech@...el.com
Subject: Re: [patch net] dpll: fix dpll_xa_ref_*_del() for multiple
 registrations


On Wed, 06 Mar, 2024 07:18:35 -0800 Rahul Rameshbabu <rrameshbabu@...dia.com> wrote:
> On Wed, 06 Mar, 2024 16:12:40 +0100 Jiri Pirko <jiri@...nulli.us> wrote:
>> From: Jiri Pirko <jiri@...dia.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@...dia.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@...dia.com>