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

[Simon Horman]

+ 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