> On 18. Nov 2024, at 14:52, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote: > > On Mon, Nov 18, 2024 at 01:55:30PM +0200, Toomas Soome wrote: >> hi! >> >> I did enable illumos kernel memory allocation/free checks (kmem_alloc/kmem_free) and apparently I did find something interesting. >> >> 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 ‘dip' >> >> The code for first error about pdip is: >> >> 8572 pdip = 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 = 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); >> >> We get this error about pdip with devfs_clean(), but apparently the ‘freed >> state is set with ndi_hold_devi(pdip) call; of course the call itself is not >> the quilty one, but the construct is — as soon as I either comment the >> ndi_hold_devi() out *or* if I move it down before devfs_clean(), then >> the error disappears. >> >> Therefore, it appears that code segment such as: >> >> var = f(); >> g(var); >> >> is causing state of var to be set ‘freed’ and check_free_strict.c is ending up >> spitting out the warning about passing freed memory with next function call. >> > > I don't see anything in ndi_hold_devi() which would mark "pdip" as freed. > As I wrote, I do not think it is really about the function itself, it is about the sequence — when I moved the ndi_hold_devi() down just before the ‘pdip’ 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="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. > 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/smatch --debug=free -fident -finline -fno-inline-functions -fno-builtin -fno-asm -fdiagnostics-show-option -nodefaultlibs -D__sun -m64 -mtune=opteron -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=gnu99 -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=illumos_kernel --disable=uninitialized,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=0 --disable=index_overflow --disable=signed,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=max-inline-insns-single=450 -fno-shrink-wrap -mindirect-branch=thunk-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=kernel 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: > > smdb.py return_states ndi_hold_devi | grep -i free > smdb.py return_states is_leaf_node | grep -i free > > 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 > >> >> now the next warning is about code: >> >> 8609 constraint = 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 } >> >> 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 ‘dip’. >> > > 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. > >> PS: the line number differences with git is because my branch has other change >> fixing memory leak discovered by smatch:D > > Fantastic. :) > > regards, > dan carpenter >
