linux-kernel - Re: [PATCH v3 04/11] net: qrtr: support identical node ids

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: <d2884803-c2ff-4beb-b982-b8533ab71066@suswa.mountain>
Date: Mon, 4 Aug 2025 13:19:26 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Simon Horman <horms@...nel.org>
Cc: Mihai Moldovan <ionic@...ic.de>, linux-arm-msm@...r.kernel.org,
	Manivannan Sadhasivam <mani@...nel.org>,
	Denis Kenzior <denkenz@...il.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Kuniyuki Iwashima <kuniyu@...gle.com>,
	Paolo Abeni <pabeni@...hat.com>,
	Willem de Bruijn <willemb@...gle.com>,
	"David S . Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH v3 04/11] net: qrtr: support identical node ids

On Mon, Aug 04, 2025 at 10:55:22AM +0100, Simon Horman wrote:
> On Fri, Aug 01, 2025 at 08:25:58PM +0300, Dan Carpenter wrote:
> > On Sun, Jul 27, 2025 at 03:40:14PM +0100, Simon Horman wrote:
> > > + Dan Carpenter
> > > 
> > > On Sun, Jul 27, 2025 at 03:09:38PM +0200, Mihai Moldovan wrote:
> > > > * On 7/24/25 15:08, Simon Horman wrote:
> 
> ...
> 
> > > This seems to a regression in Smatch wrt this particular case for this
> > > code. I bisected Smatch and it looks like it was introduced in commit
> > > d0367cd8a993 ("ranges: use absolute instead implied for possibly_true/false")
> > > 
> > > I CCed Dan in case he wants to dig into this.
> > 
> > The code looks like this:
> > 
> > 	spin_lock_irqsave(&qrtr_nodes_lock, flags);
> > 
> >         if (node->ep->id > QRTR_INDEX_HALF_UNSIGNED_MAX ||
> >             nid > QRTR_INDEX_HALF_UNSIGNED_MAX)
> >                 return -EINVAL;
> > 
> > The problem is that QRTR_INDEX_HALF_UNSIGNED_MAX is U32_MAX and
> > node->ep->id and nid are both u32 type.  The return statement is dead
> > code and I deliberately silenced warnings on impossible paths.
> > 
> > The following patch will enable the warning again and I'll test it tonight
> > to see what happens.  If it's not too painful then I'll delete it
> > properly, but if it's generates a bunch of false positives then, in the
> > end, I'm not overly stressed about bugs in dead code.
> 
> Thanks Dan,
> 
> I think the key point here is that neither Mihai nor I noticed
> the dead code. Thanks for pointing that out.
> 

I did test this over the weekend, btw.  Warning about bugs in dead code
does find some "real" bug and they might actually be real depending
on config.  But it also triggers a bunch of false positives which are
hard to solve:

	r = dma_resv_lock(&resv, NULL);
	if (r)
		r;

The dma_resv_lock() can't fail it you pass NULL as a parameter, so
Smatch says "every return takes the lock", and then "if we fail, we
return without dropping the lock."  It's difficult to solve this.

I guess we would have to say "If we're in an impossible return and it
was the locking function which failed then we didn't take the lock."
That would work, but it's sort of a tricky rule to code.

regards,
dan carpenter