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

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

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

Same questions as in v2: 
http://lore.virtuozzo.com/kernel/960265f5-da39-4346-9297-709da41d6932@virtuozzo.com/.
It is fine to print something into host.

> 

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

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