netdev - Re: [PATCH net-next] mlxsw: spectrum_router: fix xa

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] [day] [month] [year] [list]

Message-ID: <69da9a2d-bd4c-4289-a10e-7264c325c44a@intel.com>
Date: Wed, 16 Oct 2024 14:38:44 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Yuan Can <yuancan@...wei.com>, Petr Machata <petrm@...dia.com>
CC: <idosch@...dia.com>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] mlxsw: spectrum_router: fix xa_store() error
 checking

On 10/16/24 04:19, Yuan Can wrote:
> On 2024/10/15 16:06, Petr Machata wrote:
>> Yuan Can <yuancan@...wei.com> writes:
>>
>>> It is meant to use xa_err() to extract the error encoded in the return
>>> value of xa_store().
>>>
>>> Fixes: 44c2fbebe18a ("mlxsw: spectrum_router: Share nexthop counters 
>>> in resilient groups")
>>> Signed-off-by: Yuan Can <yuancan@...wei.com>
>> Reviewed-by: Petr Machata <petrm@...dia.com>
>>
>> What's the consequence of using IS_ERR()/PTR_ERR() vs. xa_err()? From
>> the documentation it looks like IS_ERR() might interpret some valid
>> pointers as errors[0]. 

it is an error to insert error pointers into xarray,
but @nhct is not an error thanks to prior check

this patch correctly checks for error returned from xarray store attempt
which is later (just after the context of the patch) converted via
ERR_PTR(), so:
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>

>> Which would then show as leaks, because we bail
>> out early and never clean up?
> 
> At least the PRT_ERR() will return a wrong error number, though the 
> error number
> 
> seems not used nor printed.
> 
>>
>> I.e. should this aim at net rather than net-next? It looks like it's not
>> just semantics, but has actual observable impact.
> Ok, do I need to send a V2 patch to net branch?
>>
>> [0] "The XArray does not support storing IS_ERR() pointers as some
>>      conflict with value entries or internal entries."
>>
>>> ---
>>>   drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 9 +++------
>>>   1 file changed, 3 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/ 
>>> drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>>> index 800dfb64ec83..7d6d859cef3f 100644
>>> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>>> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>>> @@ -3197,7 +3197,6 @@ mlxsw_sp_nexthop_sh_counter_get(struct mlxsw_sp 
>>> *mlxsw_sp,
>>>   {
>>>       struct mlxsw_sp_nexthop_group *nh_grp = nh->nhgi->nh_grp;
>>>       struct mlxsw_sp_nexthop_counter *nhct;
>>> -    void *ptr;
>>>       int err;
>>>       nhct = xa_load(&nh_grp->nhgi->nexthop_counters, nh->id);
>>> @@ -3210,12 +3209,10 @@ mlxsw_sp_nexthop_sh_counter_get(struct 
>>> mlxsw_sp *mlxsw_sp,
>>>       if (IS_ERR(nhct))
>>>           return nhct;
>>> -    ptr = xa_store(&nh_grp->nhgi->nexthop_counters, nh->id, nhct,
>>> -               GFP_KERNEL);
>>> -    if (IS_ERR(ptr)) {
>>> -        err = PTR_ERR(ptr);
>>> +    err = xa_err(xa_store(&nh_grp->nhgi->nexthop_counters, nh->id, 
>>> nhct,
>>> +                  GFP_KERNEL));
>>> +    if (err)
>>>           goto err_store;
>>> -    }
>