Re: apparent bug about check_free_strict

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

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

It feels like this email I'm responding to is recent because it's from
Nov 18.  But actually it's from Nov 18 last year...  Sorry for that.

I'm going to have to start again from scratch in some ways because I've
forgotten everything.  I've attached my reproducer script (which doesn't
reproduce anything).  There was a mistake in the reproducer script that
you sent me because the smatch options (particularly -p=illumos_kernel)
need to come before the GCC options...

I guessed the dir to be in and I had to touch the
usr/src/uts/i86pc/sys/priv_names.h file because I don't have a copy of
that but generally the reproducer seems fine.  Without the
-p=illumos_kernel then "ddi_err(DER_PANIC, ..." calls were not parsed
correctly so it generated some false positives but after I fixed that
then there was no output.

(I removed the --debug=free for now).

On Mon, Nov 18, 2024 at 11:17:12PM +0200, Toomas Soome wrote:
> commit 9a427ca57dc8a8b47d021f5f772ac164842bd996 (upstream/master, master)
> Author: Dan Carpenter <dan.carpenter@linaro.org>
> Date:   Thu Nov 14 23:12:46 2024 +0300
> 
>     db: fix a NULL dereference in get_param()
> 
> > 
> > Anyway, I've attached the diff to the latest code below with some debugging.
> > Apply the diff to the lastest Smatch and apply the code-diff to the devcfg.c
> > and then re-run Smatch.
> > 
> 
> Now this is fun. With devcfg.c changes in place, the warnings disappeared. So I removed 'if (local_debug)’ from check_free_strict.c and, I think we have something to point to:
> 
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8573 e_ddi_retire_device() set_param_freed: expr='ndi_hold_devi(pdip)' param=0 key='$'
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8573 e_ddi_retire_device() set_state new [check_free_strict] 'pdip' freed
> ../../common/os/devcfg.c:8573 e_ddi_retire_device() merge [check_free_strict] 'pdip' freed(L 8573) + undefined(L 8573) => merged (freed, undefined, merged)
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8573 e_ddi_retire_device() __set_sm new [check_free_strict] pdip fffff7ffe9523b90 = 'merged' [merged] (freed, undefined, merged)
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: ../../common/os/devcfg.c:8583 e_ddi_retire_device() warn: passing freed memory 'pdip'

What this says is that ndi_hold_devi() is setting pdip to free.  I would
have thought you have a cross function database somewhere.  Your dtrace
output also suggests that it's coming from the DB.  But you've explained
later in the thread that you don't.  It's possible that the ndi_hold_devi()
function is being parsed inline.  One way you can prevent that is by
adding a bunch of semi-colons to the function since it won't inline
functions with a lot of lines of code.

diff --git a/usr/src/uts/common/os/devcfg.c b/usr/src/uts/common/os/devcfg.c
index 787d49468e1c..f7fda31d5c23 100644
--- a/usr/src/uts/common/os/devcfg.c
+++ b/usr/src/uts/common/os/devcfg.c
@@ -1877,6 +1877,8 @@ ddi_remove_child(dev_info_t *dip, int dummy)
 void
 ndi_hold_devi(dev_info_t *dip)
 {
+	;;;;;;;;;;;;;
+
 	mutex_enter(&DEVI(dip)->devi_lock);
 	ASSERT(DEVI(dip)->devi_ref >= 0);
 	DEVI(dip)->devi_ref++;

But I don't see a kmem_free() in the ndi_hold_devi() function and I
also don't see ndi_hold_devi() listed in:
http://132-104-190-90.sta.estpak.ee/smatch/smatch.debug.log
And in my reproducer, Smatch doesn't mark it as a free function.

The fact that you're getting segfaults suggests there is memory
corruption happening somewhere.  That could explain it.  I tried
adding a valgrind before the smatch in my check script.
"valgrind ~/progs/smatch/$RELEASE/smatch --full-path..." but
valgrind didn't find anything.

Maybe you could try again with this diff (but with the correct
include path name obviously).

diff --git a/usr/src/uts/common/os/devcfg.c b/usr/src/uts/common/os/devcfg.c
index 787d49468e1c..a577bcbd49ee 100644
--- a/usr/src/uts/common/os/devcfg.c
+++ b/usr/src/uts/common/os/devcfg.c
@@ -8565,6 +8565,7 @@ e_ddi_retire_finalize(dev_info_t *dip, void *arg)
  * cons_array is a NULL terminated array of node paths for
  * which constraints have already been applied.
  */
+#include "/home/dcarpenter/progs/smatch/devel/check_debug.h"
 int
 e_ddi_retire_device(char *path, char **cons_array)
 {
@@ -8596,7 +8597,9 @@ e_ddi_retire_device(char *path, char **cons_array)
 	RIO_DEBUG((CE_NOTE, "Retire device: found dip = %p.", (void *)dip));
 
 	pdip = ddi_get_parent(dip);
+	__smatch_debug_db_on();
 	ndi_hold_devi(pdip);
+	__smatch_debug_db_off();
 
 	/*
 	 * Run devfs_clean() in case dip has no constraints and is

I don't see any corrupted output when I run it, but maybe you will
get different results.

regards,
dan carpenter


[-- Attachment #2: check.sh --]
[-- Type: application/x-sh, Size: 2227 bytes --]