Re: [Devel] [PATCH hci-8.1 3/6] ve/printk: Specify ve in printks

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

On 10/2/26 23:36, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] printk: host-log decision in ve_vprintk() uses get_exec_env() instead of the explicit ve
> - [Low] printk: ve_printk_ratelimited() passes get_exec_env() as explicit ve, bypassing in_task() guard
> - [Low] printk: !CONFIG_PRINTK stub ve_printk_deferred() keeps stale __printf(2, 3) attribute
> --
> 
> commit 59948e2d94e2f343f89f9252383a061f24bb4a33
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve/printk: Specify ve in printks
> 
> This commit adds ve_printk helpers that take an explicit VE, so callers
> where get_exec_env() is unreliable can name the VE to log to. The
> ve_ratelimit() and ve_net_ratelimit() helpers now require a VE argument,
> and the netfilter callers pass net->owner_ve.
> 
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-146839
> 
>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>> --- a/include/linux/printk.h
>> +++ b/include/linux/printk.h
> 
> [ ... ]
> 
>>   static inline __printf(3, 4) __cold
>> +int __ve_printk(int dst, struct ve_struct *ve, const char *s, ...)
>> +{
>> +	return 0;
>> +}
>>   static inline __printf(2, 3) __cold
>> -int ve_printk_deferred(int dst, const char *s, ...)
>> +int ve_printk_deferred(int dst, struct ve_struct *ve, const char *s, ...)
>>   {
>>   	return 0;
>>   }
> 
> [Severity: Low]
> The stub ve_printk_deferred() gained the struct ve_struct *ve parameter,
> so the format string is now argument 3. It still carries
> __printf(2, 3), while the real prototype was updated to __printf(3, 4).
> 
> With the old attribute, GCC treats the ve pointer as the format string.
> Could this produce a -Wformat warning (an error under -Werror) at call
> sites such as __ve_ratelimit() in lib/ratelimit.c when building with
> CONFIG_PRINTK=n?
> 
> Should the stub use __printf(3, 4) to match?

Yes, correct. Will fix in v2.

> 
> [ ... ]
> 
>> @@ -747,7 +761,8 @@ struct pi_entry {
>>   				      DEFAULT_RATELIMIT_BURST);		\
>>   									\
>> -	if (ve_ratelimit(dst, &_rs))					\
>> +	WARN_ON_ONCE((dst & VE_LOG) && !in_task());			\
>> +	if (ve_ratelimit(dst, get_exec_env(), &_rs))			\
>>   		ve_printk(dst, fmt, ##__VA_ARGS__);			\
>>   })
> 
> [Severity: Low]
> Here get_exec_env() is passed as an explicit VE. When
> ve_printk_ratelimited(VE_LOG, ...) runs from softirq, the interval-reset
> path in __ve_ratelimit() calls:
> 
> __ve_ratelimit()
>    ve_printk_deferred(dst, ve, KERN_WARNING "%s: %d callbacks suppressed\n", ...)
> 
> With a non-NULL ve, ve_vprintk_deferred() computes ve_provided as true,
> so this check is skipped:
> 
> 	if (!ve_provided && WARN_ON_ONCE(!in_task()))
> 		goto done;
> 
> Can the "callbacks suppressed" message then be written to the log of
> whichever VE belongs to the interrupted task? The in_task() guard no
> longer protects this path.

True. Will fix in v2 by passing ve0 value here in IRQ case.

> 
> Would it be better to keep passing NULL here, or to use a variant that
> keeps the guard?
> 
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
> 
> [ ... ]
> 
>> -static asmlinkage int ve_vprintk(int dst, const char *fmt, va_list args)
>> +static asmlinkage int ve_vprintk(int dst, struct ve_struct *ve,
>> +				const char *fmt, va_list args)
>>   {
>>   	va_list args2;
>> -	int r = 0;
>> +	int r = 0, ve_provided = (ve != NULL);
>>   
>>   	WARN_ON_ONCE(!dst || (dst & ~VE_LOG_BOTH));
>>   
>> +	if (!ve)
>> +		ve = get_exec_env();
>> +
>>   	va_copy(args2, args);
>>   	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
>>   		r = vprintk(fmt, args);
> 
> [Severity: Medium]
> The host-log decision here still uses ve_is_super(get_exec_env()), while
> the per-VE branch below uses ve_is_super(ve). When an explicit ve is
> passed, can the two destinations disagree?
> 
> Take net_velog_ratelimited(net->owner_ve, ...) called from a netfilter
> softirq path such as xt_TCPMSS or nf_conntrack allocation:
> 
> - If a container-owned VE_LOG-only message is emitted while the softirq
>    interrupted a VE0 task, ve_is_super(get_exec_env()) is true. The
>    message is then also printed to the host log.
> 
> - If a VE0-owned message is emitted while the softirq interrupted a
>    container task, ve_is_super(get_exec_env()) is false. If dst has no
>    VE0_LOG, the host branch is skipped. The per-VE branch is skipped too,
>    since ve_is_super(ve) is true. The message is lost.
> 
> Should this test use ve_is_super(ve) instead of get_exec_env()?

True, missed it. Will fix in v2. Next one as well.

> 
> [ ... ]
> 
>> +static asmlinkage int ve_vprintk_deferred(int dst, struct ve_struct *ve,
>> +					const char *fmt, va_list args)
>>   {
>>   	va_list args2;
>> -	int r = 0;
>> +	int r = 0, ve_provided = (ve != NULL);
>>   
>>   	WARN_ON_ONCE(!dst || (dst & ~VE_LOG_BOTH));
>>   
>> +	if (!ve)
>> +		ve = get_exec_env();
>> +
>>   	va_copy(args2, args);
>>   	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
>>   		r = vprintk_emit(0, LOGLEVEL_SCHED, NULL, fmt, args);
> 
> [Severity: Medium]
> The deferred variant has the same issue. __ve_ratelimit() calls
> ve_printk_deferred() from arbitrary contexts with an explicit ve, but the
> host-log condition still looks at get_exec_env().
> 
> Can the "callbacks suppressed" message go to the host log, or be dropped,
> based on the interrupted task's VE rather than the ve passed in?
> 
> Should this use ve_is_super(ve) as well?
> 

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

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