Re: [Devel] [PATCH hci-8.1 v3 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 1 potential issue(s) to consider:
- [Low] ve/printk: host-log branch still keyed on get_exec_env() from IRQ/softirq
--

commit 8d09395a5f1fa2043bb21991f8eb2152c9a5ad97
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_LOG branches of
ve_vprintk(), ve_vprintk_deferred() and ve_dev_printk_emit(), and to
ve_log_state(). The goal is to avoid relying on get_exec_env() in
non-task context, so that nothing leaks to a random container.

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 the host-log branch still keyed on get_exec_env() in softirq or IRQ
context?

Only the VE_LOG branch is guarded by in_task().  The preceding host
branch is unchanged:

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

With dst == VE_LOG in softirq, the result depends on which task was
interrupted:

    - host task interrupted: the message goes to the host log, and then
      the VE branch only produces a WARN, even though that branch would
      have been skipped anyway.
    - container task interrupted: neither branch runs, so the message is
      silently dropped.

In both cases r stays 0, so the caller cannot tell what happened.

Nothing leaks to a container, but the outcome is not deterministic.
Would it make sense to check in_task() once before either branch and
apply a single rule for non-task context?

ve_dev_printk_emit() takes no ve argument, so it always relies on
get_exec_env().  The in-tree caller net/core/dev.c:__ve_netdev_printk()
passes no ve and can run from softirq/NAPI context, so it reaches this
path.

The later patches in the series ("Specify ve in printks" and "Use
ve_printk with specified ve") only help callers that pass an explicit ve.
This netdev path and ve_dev_printk_emit() are not covered by them.

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

[ ... ]

> @@ -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);

[Severity: Low]
Does ve_vprintk() have the same problem?

The host-log decision still uses ve_is_super(get_exec_env()) without an
in_task() check.  In the series-final state it reads:

    if (!ve)
        ve = get_exec_env();

    va_copy(args2, args);
    if (ve_is_super(ve) || (dst & VE0_LOG))
        r = vprintk(fmt, args);

    if ((dst & VE_LOG)) {
        if (!ve_provided && WARN_ON_ONCE(!in_task()))
            goto done;

So a VE_LOG-only message from softirq with ve == NULL is either printed
to the host log or dropped, depending on the interrupted task.  The WARN
can also fire when a host task was interrupted, although the VE branch
would have been skipped anyway.

Could the host-branch decision be made deterministic for non-task
context with no explicit ve?

> @@ -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);

[Severity: Low]
The same pattern appears in ve_vprintk_deferred().  The host branch
ve_is_super(get_exec_env()) || (dst & VE0_LOG) is unguarded, and the
final-state code only guards the VE_LOG branch with
"if (!ve_provided && WARN_ON_ONCE(!in_task())) goto done;".

Can the same non-deterministic host-log versus drop outcome occur here
when this is called from IRQ or softirq with ve == NULL?

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

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