[bug report] tracing/kprobes: Add symbol counting check when module loads

[bug report] tracing/kprobes: Add symbol counting check when module loads

[Dan Carpenter]

Hello Masami Hiramatsu (Google),

Commit 9d8616034f16 ("tracing/kprobes: Add symbol counting check when
module loads") from Jul 5, 2024 (linux-next), leads to the following
Smatch static checker warning:

	kernel/trace/trace_kprobe.c:828 validate_probe_symbol()
	warn: missing error code here? 'try_module_get_by_name()' failed. 'ret' = '0'

kernel/trace/trace_kprobe.c
    815 static int validate_probe_symbol(char *symbol)
    816 {
    817         struct module *mod = NULL;
    818         char *modname = NULL, *p;
    819         int ret = 0;
    820 
    821         p = strchr(symbol, ':');
    822         if (p) {
    823                 modname = symbol;
    824                 symbol = p + 1;
    825                 *p = '\0';
    826                 mod = try_module_get_by_name(modname);
    827                 if (!mod)
--> 828                         goto out;

Is it intentional that "ret = 0;" here?

    829         }
    830 
    831         ret = validate_module_probe_symbol(modname, symbol);
    832 out:
    833         if (p)
    834                 *p = ':';
    835         if (mod)
    836                 module_put(mod);
    837         return ret;
    838 }

regards,
dan carpenter

Re: [bug report] tracing/kprobes: Add symbol counting check when module loads

[Masami Hiramatsu]

On Thu, 18 Jul 2024 13:30:57 -0500
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> Hello Masami Hiramatsu (Google),
> 
> Commit 9d8616034f16 ("tracing/kprobes: Add symbol counting check when
> module loads") from Jul 5, 2024 (linux-next), leads to the following
> Smatch static checker warning:
> 
> 	kernel/trace/trace_kprobe.c:828 validate_probe_symbol()
> 	warn: missing error code here? 'try_module_get_by_name()' failed. 'ret' = '0'
> 
> kernel/trace/trace_kprobe.c
>     815 static int validate_probe_symbol(char *symbol)
>     816 {
>     817         struct module *mod = NULL;
>     818         char *modname = NULL, *p;
>     819         int ret = 0;
>     820 
>     821         p = strchr(symbol, ':');
>     822         if (p) {
>     823                 modname = symbol;
>     824                 symbol = p + 1;
>     825                 *p = '\0';
>     826                 mod = try_module_get_by_name(modname);
>     827                 if (!mod)
> --> 828                         goto out;
> 
> Is it intentional that "ret = 0;" here?

Yes, hm, I dropped an important message here. If you see the commit
9d8616034f16, you can see there was;

/* Return 0 (defer) if the module does not exist yet. */

So it is intentional.

But thanks for reporting. It should be cleaned up.

Thank you,

> 
>     829         }
>     830 
>     831         ret = validate_module_probe_symbol(modname, symbol);
>     832 out:
>     833         if (p)
>     834                 *p = ':';
>     835         if (mod)
>     836                 module_put(mod);
>     837         return ret;
>     838 }
> 
> regards,
> dan carpenter


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>