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

[Vladimir Riabchun]

On 10/2/26 23:32, no-reply at sashiko.virtuozzo.com wrote:
> 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 at 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?

True, will fix in v2.

> 
> [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?

It doesn't matter that this hit was harmless. We report problematic
places, not problematic situations - interrupted task is a random value.

> 
> [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?

We might miss some logs in the described case, and these logs might
go to the host log instead of container's one.
Both cases are fine IMO and commit allows both. This is a sort of
best-effort printing.

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

It is fine.

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

It should be fine, WARNs are used in sched code. See reply to the
previous patch.

> 

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