Re: [Devel] [PATCH hci-8.1 2/6] ve/printk: Report potentially leaking usage of ve_printk functions

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] printk: WARN_ON_ONCE() added to ve_vprintk_deferred() and ve_log_state() on printk-sensitive paths
- [Low] ve: ve_dev_printk_emit() returns uninitialized r when VE_LOG is rejected in non-task context
- [Low] printk: in_task() WARN is evaluated before ve_is_super(), so host-context calls warn spuriously
- [Low] printk: host-log decision still uses get_exec_env() in non-task context
--

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

ve/printk: Report potentially leaking usage of ve_printk functions

This patch adds WARN_ON_ONCE(!in_task()) checks to the ve printk helpers
and to ve_log_state(). The goal is to avoid relying on get_exec_env() in
non-task context, and either reject VE printing or fall back to the host
log state.

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

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c

[ ... ]

> @@ -4952,7 +4952,9 @@ int ve_dev_printk_emit(int dst, int level, const struct device *dev, const char
>  
>  	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
>  		r = dev_vprintk_emit(level, dev, fmt, args);
> -	if ((dst & VE_LOG) && !ve_is_super(get_exec_env()))
> +
> +	/* Report IRQ usage here - printing to VE requires explicit ve reference */
> +	if ((dst & VE_LOG) && !WARN_ON_ONCE(!in_task()) && !ve_is_super(get_exec_env()))
>  		r = ve_dev_vprintk_emit(level, dev, fmt, args2);

[Severity: Low]
Can ve_dev_printk_emit() return an uninitialized value here?

The local variable is declared as "int r;" with no initializer. If
ve_dev_printk_emit(VE_LOG, ...) is called from IRQ, softirq or NMI
context while the interrupted task is in a non-super container, the
first branch is not taken because ve_is_super() is false and VE0_LOG
is not set. The second branch is now skipped because
WARN_ON_ONCE(!in_task()) is true. The function then does "return r"
with r never assigned.

Before this patch every valid dst assigned r in at least one branch.
The sibling functions ve_vprintk() and ve_vprintk_deferred() use
"int r = 0". Should r be initialized the same way here?

[Severity: Low]
Is it intended that WARN_ON_ONCE(!in_task()) is evaluated before
ve_is_super() in this condition?

With the current ordering, ve_dev_printk_emit(VE_LOG, ...) called from
IRQ context while the interrupted task is in the host (VE0) triggers
the warning, even though a host caller cannot leak into a container.
Because the warning is once per site, an early harmless hit could also
suppress a later hit that really would leak. The same ordering appears
in ve_vprintk() and ve_vprintk_deferred() below. Would testing
!ve_is_super(get_exec_env()) first limit the warning to the
potentially leaking case?

[Severity: Low]
Is the host-log decision in the preceding branch still based on
get_exec_env() in non-task context?

	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
		r = dev_vprintk_emit(level, dev, fmt, args);

If ve_dev_printk_emit(VE_LOG, ...) is called from IRQ and the
interrupted task is in the host, the message goes to the host log. If
the interrupted task is in a container, the VE branch is rejected by
the WARN and the message is dropped. The commit message says printing
is either rejected or the host log state is returned, but this branch
still depends on whichever task was interrupted. The same pattern
exists in ve_vprintk() and ve_vprintk_deferred(). Is that routing
intended?

> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -560,7 +560,8 @@ struct log_state *ve_log_state(void)
>  {
>  	struct log_state *log = &init_log_state;
>  #ifdef CONFIG_VE
> -	if (get_exec_env()->log_state)
> +	/* get_exec_env() is reliable only in task context */
> +	if (!WARN_ON_ONCE(!in_task()) && get_exec_env()->log_state)
>  		log = get_exec_env()->log_state;
>  #endif
>  	return log;

[Severity: Medium]
Can this WARN_ON_ONCE() recurse into printk?

ve_log_state() is reached from ve_vprintk_emit() and then
vprintk_emit_log(), so the WARN fires in the middle of the printk emit
path. A WARN prints a message and backtrace through the regular
non-deferred printk, and it can panic under panic_on_warn.

[ ... ]

> @@ -2540,7 +2541,9 @@ static asmlinkage int ve_vprintk(int dst, const char *fmt, va_list args)
>  	va_copy(args2, args);
>  	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
>  		r = vprintk(fmt, args);
> -	if ((dst & VE_LOG) && !ve_is_super(get_exec_env()))
> +
> +	/* Report IRQ usage here - printing to VE requires explicit ve reference */
> +	if ((dst & VE_LOG) && !WARN_ON_ONCE(!in_task()) && !ve_is_super(get_exec_env()))
>  		r = ve_vprintk_emit(0, LOGLEVEL_DEFAULT, NULL, fmt, args2);
>  	va_end(args2);

[ ... ]

> @@ -2574,7 +2577,9 @@ static asmlinkage int ve_vprintk_deferred(int dst, const char *fmt, va_list args
>  	va_copy(args2, args);
>  	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
>  		r = vprintk_emit(0, LOGLEVEL_SCHED, NULL, fmt, args);
> -	if ((dst & VE_LOG) && !ve_is_super(get_exec_env()))
> +
> +	/* Report IRQ usage here - printing to VE requires explicit ve reference */
> +	if ((dst & VE_LOG) && !WARN_ON_ONCE(!in_task()) && !ve_is_super(get_exec_env()))
>  		r = ve_vprintk_emit(0, LOGLEVEL_SCHED, NULL, fmt, args2);
>  	va_end(args2);

[Severity: Medium]
Is a synchronous WARN_ON_ONCE() safe inside ve_vprintk_deferred()?

This helper exists for contexts where regular printk is unsafe, such as
scheduler code or code holding rq or timekeeping locks. If a caller
uses ve_printk_deferred(VE_LOG, ...) from hardirq, softirq or
scheduler-related context, the WARN fires inside
ve_vprintk_deferred() and prints through the non-deferred printk with
a backtrace. Can this deadlock or recurse into printk, or panic under
panic_on_warn?

The WARN added in ve_log_state() has the same concern on the
ve_vprintk_emit() path used here.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261002-ve-printk-irq-v1-0-cb55896d455c@virtuozzo.com?part=2

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