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

[Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>]

On 10/6/26 23:59, no-reply@sashiko.virtuozzo.com wrote:
> 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?

This is correct - interrupted task is a random value, we report
incorrect logic, not specific situation.

> 
> 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().

It is fine if message goes to host.
We will drop a warning so we'll be able to get printk location
based on the call trace.

> 
>> 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.

Replied in v1.

> 
> [ ... ]
> 
>> @@ -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.

All of the above is incorrect due to the nature of WARN.

> 

-- 
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo

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