netdev - Re: [PATCH net-next 03/10] net/mlx5: hw counters: Replace IDR+lists with xarray

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: <0dce2c1d2f8adccbfbff39118af9796d84404a67.camel@nvidia.com>
Date: Tue, 27 Aug 2024 11:14:10 +0000
From: Cosmin Ratiu <cratiu@...dia.com>
To: Tariq Toukan <tariqt@...dia.com>, "horms@...nel.org" <horms@...nel.org>
CC: "davem@...emloft.net" <davem@...emloft.net>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, Gal Pressman <gal@...dia.com>, Leon Romanovsky
	<leonro@...dia.com>, "kuba@...nel.org" <kuba@...nel.org>,
	"edumazet@...gle.com" <edumazet@...gle.com>, Saeed Mahameed
	<saeedm@...dia.com>, "pabeni@...hat.com" <pabeni@...hat.com>
Subject: Re: [PATCH net-next 03/10] net/mlx5: hw counters: Replace IDR+lists
 with xarray

On Thu, 2024-08-15 at 14:44 +0100, Simon Horman wrote:
> On Thu, Aug 15, 2024 at 08:46:49AM +0300, Tariq Toukan wrote:
[...]
> > +	u32 last_bulk_id = 0;
> > +	u64 bulk_query_time;
> >  	u32 bulk_base_id;
[...]
> > +	xas_for_each(&xas, counter, U32_MAX) {
[...]
> > +		if (unlikely(counter->id >= last_bulk_id)) {
> > +			/* Start new bulk query. */
> > +			/* First id must be aligned to 4 when using bulk query. */
> > +			bulk_base_id = counter->id & ~0x3;
[...]
> > +			bulk_query_time = jiffies;
[...]
> >  		}
> 
> Hi Cosmin and Tariq,
> 
> It looks like bulk_query_time and bulk_base_id may be uninitialised or
> stale - from a previous loop iteration - if the condition above is not met.
> 
> Flagged by Smatch.

I believe this is a false positive. I snipped parts from the reply
above to focus on the relevant parts:
- last_bulk_id always starts at 0 so
- the if branch will always be executed in the first iteration and
- it will set bulk_query_time and bulk_base_id for future iterations.

I am not familiar with Smatch, is there a way to convince it to
interpret the code correctly (some annotations perhaps)?
The alternatives are to accept the false positive or explicitly
initialize those vars to something, which is suboptimal and would be
working around a tooling failure.

Cosmin.