Re: apparent bug about check_free_strict

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