Re: [PATCH net-next] mlxsw: spectrum_router: fix xa_store() error checking

[Petr Machata <petrm@nvidia.com>]

Yuan Can <yuancan@huawei.com> writes:

> On 2024/10/15 16:06, Petr Machata wrote:
>> Yuan Can <yuancan@huawei.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@huawei.com>
>>
>> Reviewed-by: Petr Machata <petrm@nvidia.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]. 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.

What I'm saying is that if IS_ERR overestimates what is an error, we
bail out from mlxsw_sp_nexthop_sh_counter_get() with a failure, but
xa_store() actually succeeded, and the corresponding xa_erase is never
called, causing a leak.

(If IS_ERR underestimates what is an error, fails to store the allocated
counter, and counter sharing stops working. This will waste HW
resources, though I think it should still behave correctly overall.)

Anyway, it looks to me like a net material.

>>
>> 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?

Yes please.