Re: [Devel] [PATCH hci-8.1 v2 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 3 potential issue(s) to consider:
- [Medium] printk: WARN_ON_ONCE() inside ve_vprintk_deferred() defeats deferred printk
- [Low] printk: non-task VE_LOG messages are dropped while host routing uses get_exec_env()
- [Low] printk: WARN_ON_ONCE(!in_task()) runs before the ve_is_super() check
--

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

ve/printk: Report potentially leaking usage of ve_printk functions

This commit adds WARN_ON_ONCE(!in_task()) checks to ve_log_state(),
ve_vprintk(), ve_vprintk_deferred() and ve_dev_printk_emit(). When a
VE_LOG message is requested from non-task context, the message is
rejected (or the host log state is returned) instead of relying on
get_exec_env().

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]
Is it intended that the WARN_ON_ONCE(!in_task()) is evaluated before the
ve_is_super() check?

In ve_dev_printk_emit(), any non-task caller that passes dst with VE_LOG
will hit the WARN even when the interrupted task is in ve0, where nothing
could leak to a container. Could the ve_is_super() test be placed first so
the WARN only fires when a container could actually be affected?

The same ordering appears in ve_vprintk() and ve_vprintk_deferred().

Also, the host branch just above still uses get_exec_env():

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

In non-task context with dst == VE_LOG, if a host task was interrupted the
message goes to the host log. If a container task was interrupted, the VE
branch is rejected by the WARN guard, the message is dropped, and r stays
0. Doesn't this make the outcome depend on which task was interrupted,
which is the get_exec_env() unreliability the commit message wants to
avoid? ve_log_state() by contrast falls back to the host log. The same
applies to ve_vprintk() and ve_vprintk_deferred().

> 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 run while printk-side processing already holds locks?

ve_log_state() is called from ve_vprintk_emit(), so a WARN here can fire in
the middle of a printk call. The WARN prints a banner and backtrace through
the normal printk path.

ve_vprintk_emit()
  ve_log_state()
    WARN_ON_ONCE(!in_task())
      __warn() -> printk

Could this recurse or deadlock if the caller is in scheduler, timer or
other non-task context holding such locks? The WARN fires only once, and I
did not find an in-tree caller that triggers this. In the final series
state, the unconditional WARN in ve_log_state() on this path is still
present.

[ ... ]

> @@ -2540,7 +2541,9 @@ static asmlinkage int ve_vprintk(int dst, const char *fmt, va_list 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);

[Severity: Medium]
Does this make any IRQ or NMI caller of ve_printk() that passes VE_LOG
trigger a WARN? See the comments on ve_log_state() above and on
ve_vprintk_deferred() below regarding emitting a WARN from such contexts.

> @@ -2574,7 +2577,9 @@ static asmlinkage int ve_vprintk_deferred(int dst, const char *fmt, va_list 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);

[Severity: Medium]
Does the synchronous WARN_ON_ONCE() inside ve_vprintk_deferred() defeat the
purpose of the deferred variant?

This helper is meant to be safe from scheduler and IRQ contexts. If a
caller outside task context (hardirq, softirq, NMI or scheduler code) calls
it with dst containing VE_LOG and no explicit ve, the WARN goes through
__warn() and the normal printk path. That path prints a banner and
backtrace and may flush consoles or wake klogd.

Could this self-deadlock or recurse when the caller holds scheduler, timer
or logbuf locks? The WARN fires only once. The only in-tree caller I found,
__ve_ratelimit() in lib/ratelimit.c, runs under rs->lock and passes ve, so
I have no concrete deadlocking caller. In the final series state,
ve_vprintk_deferred() still has the check
"if (!ve_provided && WARN_ON_ONCE(!in_task()))", so the concern remains
when no ve is provided.

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

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