Re: apparent bug about check_free_strict

> On 18. Nov 2024, at 14:52, Dan Carpenter <[email protected]> =
wrote:
>=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/sma=
tch: ../../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/sma=
tch: ../../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/sma=
tch: ../../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 =
comment 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_free_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 =
freed.
>=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 disappear.

> 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 =
match the
> line numbers up.
>=20

I guess it is a bit more complicated because you will also need the =
illumos specific update to check_free_strict.

The command in usr/src/uts/intel/genunix is run as:

=
/code/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/sma=
tch --debug=3Dfree -fident -finline -fno-inline-functions -fno-builtin =
-fno-asm -fdiagnostics-show-option -nodefaultlibs -D__sun -m64 =
-mtune=3Dopteron -Ui386 -U__i386 -fno-strict-aliasing =
-fno-unit-at-a-time -fno-optimize-sibling-calls -O2 -D_ASM_INLINES =
-ffreestanding -mno-red-zone -mno-mmx -mno-sse -msave-args -Wall -Wextra =
-g -gdwarf-2 -std=3Dgnu99 -msave-args -Werror -Wno-missing-braces =
-Wno-sign-compare -Wno-unknown-pragmas -Wno-unused-parameter =
-Wno-missing-field-initializers -Winline -Wno-unused -Wno-empty-body =
-p=3Dillumos_kernel --disable=3Duninitialized,check_check_deref -Wno-vla =
-Wno-one-bit-signed-bitfield -Wno-external-function-has-definition =
-Wno-old-style-definition -Wno-strict-prototypes --fatal-checks =
--timeout=3D0 --disable=3Dindex_overflow =
--disable=3Dsigned,all_func_returns -Wno-unused-variable =
-Wno-unused-value -Wno-unused-function -Wno-parentheses =
-Wno-maybe-uninitialized -Wno-clobbered -Wno-empty-body =
-fno-inline-small-functions -fno-inline-functions-called-once =
-fno-ipa-cp -fno-ipa-icf -fno-clone-functions -fno-reorder-functions =
-fno-reorder-blocks-and-partition -fno-aggressive-loop-optimizations =
--param=3Dmax-inline-insns-single=3D450 -fno-shrink-wrap =
-mindirect-branch=3Dthunk-extern -mindirect-branch-register =
-fno-asynchronous-unwind-tables -fstack-protector-strong =
-fno-eliminate-unused-debug-symbols -fno-eliminate-unused-debug-types =
-D_KERNEL -ffreestanding -D_SYSCALL32 -D_SYSCALL32_IMPL -D_ELF64 =
-D_DDI_STRICT -Dsun -D__sun -D__SVR4 -DOPTERON_ERRATUM_88 =
-DOPTERON_ERRATUM_91 -DOPTERON_ERRATUM_93 -DOPTERON_ERRATUM_95 =
-DOPTERON_ERRATUM_99 -DOPTERON_ERRATUM_100 -DOPTERON_ERRATUM_101 =
-DOPTERON_ERRATUM_108 -DOPTERON_ERRATUM_109 -DOPTERON_ERRATUM_121 =
-DOPTERON_ERRATUM_122 -DOPTERON_ERRATUM_123 -DOPTERON_ERRATUM_131 =
-DOPTERON_WORKAROUND_6336786 -DOPTERON_ERRATUM_147 -DOPTERON_ERRATUM_172 =
-DOPTERON_ERRATUM_298 -DOPTERON_ERRATUM_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


I did put the samples of files into =
http://132-104-190-90.sta.estpak.ee/smatch/, I still need to clean up a =
bit my smatch repo, that will take a bit more time.

thanks,
toomas

> Are you using the cross function DB?  If so then you could do:
>=20
> smdb.py return_states ndi_hold_devi | grep -i free
> smdb.py return_states is_leaf_node | grep -i free
>=20
> Somewhere there is a function marking code as freed incorrectly.  But =
there
> the check_free_strict.c file doesn't really have a long list of free =
functions.
> They're at the top:
> https://github.com/error27/smatch/blob/master/check_free_strict.c#L50
>=20
>>=20
>> now the next warning is about code:
>>=20
>>  8609          constraint =3D 1; /* assume constraints allow retire =
*/
>>  8610          (void) e_ddi_retire_notify(dip, &constraint);
>>  8611          if (!is_leaf_node(dip)) {
>>  8612                  ndi_devi_enter(dip);
>>  8613                  ddi_walk_devs(ddi_get_child(dip), =
e_ddi_retire_notify,
>>  8614                      &constraint);
>>  8615                  ndi_devi_exit(dip);
>>  8616          }
>>  8617
>>  8618          /*
>>  8619           * Now finalize the retire
>>  8620           */
>>  8621          (void) e_ddi_retire_finalize(dip, &constraint);
>>  8622          if (!is_leaf_node(dip)) {
>>  8623                  ndi_devi_enter(dip);
>>  8624                  ddi_walk_devs(ddi_get_child(dip), =
e_ddi_retire_finalize,
>>  8625                      &constraint);
>>  8626                  ndi_devi_exit(dip);
>>  8627          }
>>=20
>> Here we do get warning about ndi_devi_enter(), but if I replace dip =
in is_leaf_node() by NULL, we do not get any more warnings about =
=E2=80=98dip=E2=80=99.
>>=20
>=20
> There are two reasons why that is.  1)  Smatch thinks is_leaf_node() =
is freeing
> the parameter.  2)  If you call is_leaf_node(NULL) then it leads to a =
crash and
> the check_free_strict.c code only tries to warn about use after frees =
which are
> reachable.
>=20
>> PS: the line number differences with git is because my branch has =
other change
>> fixing memory leak discovered by smatch:D
>=20
> Fantastic.  :)
>=20
> regards,
> dan carpenter
>=20