Re: [Devel] [PATCH hci-8.1 v2 1/6] ve/printk: Add sanity checks in ve_printk functions

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] printk: ve_vprintk_deferred() raises non-deferred WARN_ON_ONCE() on invalid dst
--

commit 57d6a4cedde45a78dd9af42f6ddde31d3e97b8fb
Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

ve/printk: Add sanity checks in ve_printk functions

This commit adds a WARN_ON_ONCE() check on the dst argument in
ve_dev_printk_emit(), ve_vprintk() and ve_vprintk_deferred(). It also
reorders the VE_LOG test in the second if statement of each function.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-146839

> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c

[ ... ]

> @@ -2567,10 +2569,12 @@ static asmlinkage int ve_vprintk_deferred(int dst, const char *fmt, va_list args
>  	va_list args2;
>  	int r = 0;
>  
> +	WARN_ON_ONCE(!dst || (dst & ~VE_LOG_BOTH));
> +

[Severity: Low]
Can this WARN_ON_ONCE() in ve_vprintk_deferred() cause problems in
printk-unsafe contexts?

ve_vprintk_deferred() emits through vprintk_emit() at LOGLEVEL_SCHED
so that it can be used where plain printk is not safe. The warning
goes through the regular __warn() path, which prints via the normal,
non-deferred printk.

If a caller passes dst == 0 or a value with bits outside VE_LOG_BOTH
from such a context, the warning could recurse into or deadlock on
console or logbuf locks:

ve_ratelimit() (lib/ratelimit.c)
  ve_printk_deferred(dst, ve, KERN_WARNING ...)
    ve_vprintk_deferred(dst, ...)
      WARN_ON_ONCE(!dst || (dst & ~VE_LOG_BOTH));

I did not verify that any current caller passes an invalid dst, so this
is only a possible problem. The warning fires at most once.

Would it make sense to drop the check from the deferred helper or use a
deferred-safe way to report a bad dst?

>  	va_copy(args2, args);
>  	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
>  		r = vprintk_emit(0, LOGLEVEL_SCHED, NULL, fmt, args);
> -	if (!ve_is_super(get_exec_env()) && (dst & VE_LOG))
> +	if ((dst & VE_LOG) && !ve_is_super(get_exec_env()))
>  		r = ve_vprintk_emit(0, LOGLEVEL_SCHED, NULL, fmt, args2);
>  	va_end(args2);

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-ve-printk-irq-v2-0-db1dc4f6ed07@virtuozzo.com?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel