On Mon, Nov 18, 2024 at 03:28:57PM +0200, Toomas Soome wrote:
>
>
> > 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.
>
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 pushed
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
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);
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
