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. 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. 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
