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

Message-ID: <20250727144014.GX1367887@horms.kernel.org>
Date: Sun, 27 Jul 2025 15:40:14 +0100
From: Simon Horman <horms@...nel.org>
To: Mihai Moldovan <ionic@...ic.de>
Cc: 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, Dan Carpenter <dan.carpenter@...aro.org>
Subject: Re: [PATCH v3 04/11] net: qrtr: support identical node ids

+ Dan Carpenter

On Sun, Jul 27, 2025 at 03:09:38PM +0200, Mihai Moldovan wrote:
> * On 7/24/25 15:08, Simon Horman wrote:
> > [...]
> 
> Thank you for the reviews, to both you and Jakub.
> 
> 
> > This will leak holding qrtr_nodes_lock.
> 
> It certainly does, will be fixed in v4.
> 
> 
> > Flagged by Smatch.
> 
> I haven't used smatch before, and probably should do so going forward.
> 
> Curiously, a simple kchecker net/qrtr/ run did not warn about the locking
> issue (albeit it being obvious in the patch), while it did warn about the
> second issue with ret. Am I missing something?

TL;DR: No, I seem to have been able to reproduce what you see.

I ran Smatch, compiled from a recent Git commit, like this:

kchecker net/qrtr/af_qrtr.o

The warnings I saw (new to this patch) are:

net/qrtr/af_qrtr.c:498 qrtr_node_assign() warn: inconsistent returns 'global &qrtr_nodes_lock'.
  Locked on  : 484
  Unlocked on: 498
net/qrtr/af_qrtr.c:613 qrtr_endpoint_post() warn: missing error code 'ret'

That was with Smatch compiled from Git [1]
commit e1d933013098 ("return_efault: don't rely on the cross function DB")

I tried again with the latest head,
commit 2fb2b9093c5d ("sleep_info: The synchronize_srcu() sleeps").
And in that case I no longer see the 1st warning, about locking.
I think this is what you saw too.

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.

[1] https://repo.or.cz/smatch.git

> 
> 
> > But ret is now 0, whereas before this patch it was -EINVAL.
> > This seems both to be an unintentional side effect of this patch,
> > and incorrect.
> 
> True. Will also fixed in v4.
> 
> 
> Mihai