Re: apparent bug about check_free_strict

--PN7I86YfXljd3gzO
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Nov 18, 2024 at 03:28:57PM +0200, Toomas Soome wrote:
>=20
>=20
> > On 18. Nov 2024, at 14:52, Dan Carpenter <[email protected]> wro=
te:
> >=20
> > On Mon, Nov 18, 2024 at 01:55:30PM +0200, Toomas Soome wrote:
> >> hi!
> >>=20
> >> I did enable illumos kernel memory allocation/free checks (kmem_alloc/=
kmem_free) and apparently I did find something interesting.
> >>=20
> >> The warning is:
> >> /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'
> >> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386=
/smatch: ../../common/os/devcfg.c:8612 e_ddi_retire_device() warn: passing =
freed memory 'dip'
> >> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386=
/smatch: ../../common/os/devcfg.c:8621 e_ddi_retire_device() warn: passing =
freed memory =E2=80=98dip'
> >>=20
> >> The code for first error about pdip is:
> >>=20
> >>  8572          pdip =3D ddi_get_parent(dip);
> >>  8573          ndi_hold_devi(pdip);
> >>  8574     8575          /*
> >>  8576           * Run devfs_clean() in case dip has no constraints and=
 is
> >>  8577           * not in use, so is retireable but there are dv_nodes =
holding
> >>  8578           * ref-count on the dip. Note that devfs_clean() always=
 returns
> >>  8579           * success.
> >>  8580           */
> >>  8581          devnm =3D kmem_alloc(MAXNAMELEN + 1, KM_SLEEP);
> >>  8582          (void) ddi_deviname(dip, devnm);
> >>  8583          (void) devfs_clean(pdip, devnm + 1, DV_CLEAN_FORCE);
> >>  8584          kmem_free(devnm, MAXNAMELEN + 1);
> >>  8585     8586          ndi_devi_enter(pdip);
> >>=20
> >> We get this error about pdip with devfs_clean(), but apparently the =
=E2=80=98freed
> >> state is set with ndi_hold_devi(pdip) call; of course the call itself =
is not
> >> the quilty one, but the construct is =E2=80=94 as soon as I either com=
ment the
> >> ndi_hold_devi() out *or* if I move it down before devfs_clean(), then
> >> the error disappears.
> >>=20
> >> Therefore, it appears that code segment such as:
> >>=20
> >> var =3D f();
> >> g(var);
> >>=20
> >> is causing state of var to be set =E2=80=98freed=E2=80=99 and check_fr=
ee_strict.c is ending up
> >> spitting out the warning about passing freed memory with next function=
 call.
> >>=20
> >=20
> > I don't see anything in ndi_hold_devi() which would mark "pdip" as free=
d.
> >=20
>=20
>=20
> As I wrote, I do not think it is really about the function itself, it is =
about
> the sequence =E2=80=94 when I moved the ndi_hold_devi() down just before =
the =E2=80=98pdip=E2=80=99 was actually called, then this warning did disap=
pear.
>=20
> > I don't know how to run Smatch on this file...  Could you re-run Smatch=
 with the
> > --debug=3D"free" option and save the output to a file?  Maybe send that=
 output
> > along with the whole file or run it against the lates git so I can matc=
h the
> > line numbers up.
> >=20
>=20
> I guess it is a bit more complicated because you will also need the illum=
os specific update to check_free_strict.
>=20
> The command in usr/src/uts/intel/genunix is run as:
>=20
> /code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/sm=
atch --debug=3Dfree -fident -finline -fno-inline-functions -fno-builtin -fn=
o-asm -fdiagnostics-show-option -nodefaultlibs -D__sun -m64 -mtune=3Doptero=
n -Ui386 -U__i386 -fno-strict-aliasing -fno-unit-at-a-time -fno-optimize-si=
bling-calls -O2 -D_ASM_INLINES -ffreestanding -mno-red-zone -mno-mmx -mno-s=
se -msave-args -Wall -Wextra -g -gdwarf-2 -std=3Dgnu99 -msave-args -Werror =
-Wno-missing-braces -Wno-sign-compare -Wno-unknown-pragmas -Wno-unused-para=
meter -Wno-missing-field-initializers -Winline -Wno-unused -Wno-empty-body =
-p=3Dillumos_kernel --disable=3Duninitialized,check_check_deref -Wno-vla -W=
no-one-bit-signed-bitfield -Wno-external-function-has-definition -Wno-old-s=
tyle-definition -Wno-strict-prototypes --fatal-checks --timeout=3D0 --disab=
le=3Dindex_overflow --disable=3Dsigned,all_func_returns -Wno-unused-variabl=
e -Wno-unused-value -Wno-unused-function -Wno-parentheses -Wno-maybe-uninit=
ialized -Wno-clobbered -Wno-empty-body -fno-inline-small-functions -fno-inl=
ine-functions-called-once -fno-ipa-cp -fno-ipa-icf -fno-clone-functions -fn=
o-reorder-functions -fno-reorder-blocks-and-partition -fno-aggressive-loop-=
optimizations --param=3Dmax-inline-insns-single=3D450 -fno-shrink-wrap -min=
direct-branch=3Dthunk-extern -mindirect-branch-register -fno-asynchronous-u=
nwind-tables -fstack-protector-strong -fno-eliminate-unused-debug-symbols -=
fno-eliminate-unused-debug-types -D_KERNEL -ffreestanding -D_SYSCALL32 -D_S=
YSCALL32_IMPL -D_ELF64 -D_DDI_STRICT -Dsun -D__sun -D__SVR4 -DOPTERON_ERRAT=
UM_88 -DOPTERON_ERRATUM_91 -DOPTERON_ERRATUM_93 -DOPTERON_ERRATUM_95 -DOPTE=
RON_ERRATUM_99 -DOPTERON_ERRATUM_100 -DOPTERON_ERRATUM_101 -DOPTERON_ERRATU=
M_108 -DOPTERON_ERRATUM_109 -DOPTERON_ERRATUM_121 -DOPTERON_ERRATUM_122 -DO=
PTERON_ERRATUM_123 -DOPTERON_ERRATUM_131 -DOPTERON_WORKAROUND_6336786 -DOPT=
ERON_ERRATUM_147 -DOPTERON_ERRATUM_172 -DOPTERON_ERRATUM_298 -DOPTERON_ERRA=
TUM_721 -I../../intel -nostdinc -I../../common -I/code/illumos-gate/usr/src=
/common -I/code/illumos-gate/usr/src/uts/common/fs/zfs -I../../i86pc -c -o =
/tmp/cw.GCaq5P/cwICaO5P.o ../../common/os/devcfg.c -mcmodel=3Dkernel
>=20
>=20
> I did put the samples of files into http://132-104-190-90.sta.estpak.ee/s=
match/,
> I still need to clean up a bit my smatch repo, that will take a bit more =
time.
>=20

There's something weird going on.  I don't see any problem with the changes=
 you
have made to check_free_strict.c.  I started to just merge your changes but=
 then
I realized I don't know who to give authorship credit etc but I'd already p=
ushed
whatever changes I had on my end so I just made the situation worse.

You are testing with the latest Smatch right?

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.

regards,
dan carpenter



--PN7I86YfXljd3gzO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename=diff

diff --git a/check_free_strict.c b/check_free_strict.c
index 2838054e476d..be92b33776b7 100644
--- a/check_free_strict.c
+++ b/check_free_strict.c
@@ -47,6 +47,10 @@ struct func_info {
 	param_key_hook *call_back;
 };
 
+static struct func_info illumos_func_table[] = {
+	{ "kmem_free", PARAM_FREED, 0, "$" },
+};
+
 static struct func_info func_table[] = {
 	{ "free", PARAM_FREED, 0, "$" },
 	{ "kfree", PARAM_FREED, 0, "$" },
@@ -460,6 +464,8 @@ static void match_free(struct expression *expr, const char *name, struct symbol
 
 	track_freed_param(arg, &freed);
 	call_free_call_backs_name_sym(name, sym);
+	if (local_debug)
+		sm_msg("%s: expr='%s' name='%s'", __func__, expr_to_str(expr), name);
 	set_state_expr(my_id, arg, &freed);
 }
 
@@ -505,6 +511,8 @@ static void match___skb_pad(struct expression *expr, const char *name, struct sy
 
 	skb = get_argument_from_call_expr(expr->args, 0);
 	track_freed_param(skb, state);
+	if (local_debug)
+		sm_msg("%s: expr='%s' name='%s'", __func__, expr_to_str(expr), name);
 	set_state_expr(my_id, skb, state);
 }
 
@@ -565,6 +573,8 @@ free:
 
 static void set_param_freed(struct expression *expr, int param, char *key, char *value)
 {
+	if (local_debug)
+		sm_msg("%s: expr='%s' param=%d key='%s'", __func__, expr_to_str(expr), param, key);
 	set_param_helper(expr, param, key, value, &freed);
 }
 
@@ -652,17 +662,28 @@ static void match_untracked(struct expression *call, int param)
 
 void check_free_strict(int id)
 {
-	struct func_info *info;
+	struct func_info *info, *table;
 	param_key_hook *cb;
+	size_t array_size;
 	int i;
 
 	my_id = id;
 
-	if (option_project != PROJ_KERNEL)
+	switch (option_project) {
+	case PROJ_KERNEL:
+		table = func_table;
+		array_size = ARRAY_SIZE(func_table);
+		break;
+	case PROJ_ILLUMOS_KERNEL:
+		table = illumos_func_table;
+		array_size = ARRAY_SIZE(illumos_func_table);
+		break;
+	default:
 		return;
+	}
 
-	for (i = 0; i < ARRAY_SIZE(func_table); i++) {
-		info = &func_table[i];
+	for (i = 0; i < array_size; i++) {
+		info = &table[i];
 
 		insert_string(&handled, info->name);
 

--PN7I86YfXljd3gzO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename=code-diff

diff --git a/usr/src/uts/common/os/devcfg.c b/usr/src/uts/common/os/devcfg.c
index cc37b39ec5fb..b74c8bec9782 100644
--- a/usr/src/uts/common/os/devcfg.c
+++ b/usr/src/uts/common/os/devcfg.c
@@ -8532,6 +8539,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)
 {
@@ -8563,7 +8571,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_local_debug_on();
 	ndi_hold_devi(pdip);
+	__smatch_local_debug_off();
 
 	/*
 	 * Run devfs_clean() in case dip has no constraints and is

--PN7I86YfXljd3gzO--