Re: [Devel] [PATCH hci-8.1 v2 1/6] ve/printk: Add sanity checks in ve_printk functions

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

On 10/6/26 23:58, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] printk: ve_vprintk_deferred() raises non-deferred WARN_ON_ONCE() on invalid dst
> --
> 
> commit 57d6a4cedde45a78dd9af42f6ddde31d3e97b8fb
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve/printk: Add sanity checks in ve_printk functions
> 
> This commit adds a WARN_ON_ONCE() check on the dst argument in
> ve_dev_printk_emit(), ve_vprintk() and ve_vprintk_deferred(). It also
> reorders the VE_LOG test in the second if statement of each function.
> 
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-146839
> 
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
> 
> [ ... ]
> 
>> @@ -2567,10 +2569,12 @@ static asmlinkage int ve_vprintk_deferred(int dst, const char *fmt, va_list args
>>   	va_list args2;
>>   	int r = 0;
>>   
>> +	WARN_ON_ONCE(!dst || (dst & ~VE_LOG_BOTH));
>> +
> 
> [Severity: Low]
> Can this WARN_ON_ONCE() in ve_vprintk_deferred() cause problems in
> printk-unsafe contexts?
> 
> ve_vprintk_deferred() emits through vprintk_emit() at LOGLEVEL_SCHED
> so that it can be used where plain printk is not safe. The warning
> goes through the regular __warn() path, which prints via the normal,
> non-deferred printk.
> 
> If a caller passes dst == 0 or a value with bits outside VE_LOG_BOTH
> from such a context, the warning could recurse into or deadlock on
> console or logbuf locks:
> 
> ve_ratelimit() (lib/ratelimit.c)
>    ve_printk_deferred(dst, ve, KERN_WARNING ...)
>      ve_vprintk_deferred(dst, ...)
>        WARN_ON_ONCE(!dst || (dst & ~VE_LOG_BOTH));
> 
> I did not verify that any current caller passes an invalid dst, so this
> is only a possible problem. The warning fires at most once.
> 
> Would it make sense to drop the check from the deferred helper or use a
> deferred-safe way to report a bad dst?

No. Replied to v1, same issue:
1. dst == 0 is actually a bug
2. WARN_ON is not printing anything himself.

No fixes needed.

> 
>>   	va_copy(args2, args);
>>   	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
>>   		r = vprintk_emit(0, LOGLEVEL_SCHED, NULL, fmt, args);
>> -	if (!ve_is_super(get_exec_env()) && (dst & VE_LOG))
>> +	if ((dst & VE_LOG) && !ve_is_super(get_exec_env()))
>>   		r = ve_vprintk_emit(0, LOGLEVEL_SCHED, NULL, fmt, args2);
>>   	va_end(args2);
> 

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

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