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

[Dan Carpenter <dan.carpenter@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 2452 bytes --]

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

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.

regards,
dan carpenter


[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 896 bytes --]

diff --git a/check_inconsistent_locking.c b/check_inconsistent_locking.c
index f3cce559d7a6..e95d9110a1e1 100644
--- a/check_inconsistent_locking.c
+++ b/check_inconsistent_locking.c
@@ -67,8 +67,8 @@ static void check_lock_bool(const char *name, struct symbol *sym)
 	FOR_EACH_PTR(get_all_return_strees(), stree) {
 		orig = __swap_cur_stree(stree);
 
-		if (is_impossible_path())
-			goto swap_stree;
+//		if (is_impossible_path())
+//			goto swap_stree;
 
 		return_sm = get_sm_state(RETURN_ID, "return_ranges", NULL);
 		if (!return_sm)
@@ -145,8 +145,8 @@ static void check_lock(const char *name, struct symbol *sym)
 	FOR_EACH_PTR(get_all_return_strees(), stree) {
 		orig = __swap_cur_stree(stree);
 
-		if (is_impossible_path())
-			goto swap_stree;
+//		if (is_impossible_path())
+//			goto swap_stree;
 
 		return_sm = get_sm_state(RETURN_ID, "return_ranges", NULL);
 		if (!return_sm)