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

Message-ID: <87bjzk2wtm.fsf@nvidia.com>
Date: Wed, 16 Oct 2024 11:41:28 +0200
From: Petr Machata <petrm@...dia.com>
To: Yuan Can <yuancan@...wei.com>
CC: Petr Machata <petrm@...dia.com>, <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


Yuan Can <yuancan@...wei.com> writes:

> 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]. 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.