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

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

On 10/7/26 00:03, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] printk: explicit-VE path skips the in_task() guard, so VE log wakeups can run in softirq
> - [Low] printk: stale IRQ comment, trailing whitespace and duplicate struct ve_struct decl
> --
> 
> commit a71d74a31e0ae4dbd29bc8edddfdbf1bedab2ffe
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve/printk: Specify ve in printks
> 
> This patch adds ve_printk helpers that take an explicit VE pointer. The
> ratelimit helpers and the net_ve*_ratelimited macros now require a VE
> argument. The conntrack and TCPMSS 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
> 
> [ ... ]
> 
>> @@ -76,6 +76,7 @@ extern int console_printk[];
>>   #define VE0_LOG		1
>>   #define VE_LOG		2
>>   #define VE_LOG_BOTH	(VE0_LOG | VE_LOG)
>> +struct ve_struct;
>>   
>>   extern void console_verbose(void);
> 
> [Severity: Low]
> This isn't a bug, but this adds a second forward declaration of struct
> ve_struct. printk.h already has one further down, just before
> ve_log_init().

True, will remove in v3.

> 
>> 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(ve) || (dst & VE0_LOG))
>> +	if (ve_is_super(ve) || (dst & VE0_LOG))
>>   		r = vprintk(fmt, args);
>>   
>>   	/* 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);
>> +	if ((dst & VE_LOG)) {
>> +		if (!ve_provided && WARN_ON_ONCE(!in_task()))
>> +			goto done;
>> +		if (!ve_is_super(ve))
>> +			r = __ve_vprintk_emit(ve, 0, LOGLEVEL_DEFAULT, NULL, fmt, args2);
>> +	}
>> +done:
>>   	va_end(args2);
> 
> [Severity: Medium]
> Before this patch, the VE log branch was skipped whenever in_task() was
> false. With an explicit VE the in_task() guard is now bypassed.

Yes, this is intentional.

> 
> Can this path now run from softirq or BH-disabled context?

It could do it before the patch series as well.

> 
> __nf_conntrack_alloc() runs on the packet path. __nf_ct_expect_check()
> runs under spin_lock_bh(). Both call net_veboth_ratelimited(net->owner_ve,
> ...), which expands to the non-deferred __ve_printk():
> 
> __nf_conntrack_alloc()
>    net_veboth_ratelimited(net->owner_ve, ...)
>      __ve_printk()
>        ve_vprintk()            /* ve_provided is true */
>          __ve_vprintk_emit(ve, 0, LOGLEVEL_DEFAULT, ...)
>            vprintk_emit_log(__ve_log_state(ve), ...)
>              wake_up_interruptible(&log->wait)
> 
> LOGLEVEL_DEFAULT does not clear ft.legacy_direct, so a non-init log goes
> through the wake_up_interruptible() path in vprintk_emit_log().
> 
> The waitqueue lock is a spinlock_t. That is fine in softirq on non-RT
> kernels. Is it also intended to be reachable from these contexts on
> PREEMPT_RT, and from any future explicit-VE caller running in hardirq or
> under a raw_spinlock_t?

I don't want to debug working printk. It works, it used to work.
The only thing I change in the end is rejecting prints to
random containers.

> 
> The commit message only justifies the explicit VE for the deferred
> helpers ("This is true for ve_printk_deferred functions, and their
> users"). Yet net_velog_ratelimited() and net_veboth_ratelimited() use the
> non-deferred __ve_printk().

Interrupt != scheduler.

> 
> Also, ve_printk_ratelimited() in printk.h still falls back to ve0 when
> !in_task(). So the two ratelimited paths now follow different rules.

net_veboth_ratelimited uses explicit ve, so no rules are needed.
With generic ve_ratelimit we will redirect message to host and drop
a warning. Consistent with the behavior "print to host or drop".

> 
> [Severity: Low]
> This isn't a bug, but the comment "Report IRQ usage here - printing to VE
> requires explicit ve reference" was kept in both ve_vprintk() and
> ve_vprintk_deferred(). The in_task() check now only applies when no VE
> was passed, so the comment no longer matches the code.

True, will drop the comment in v3 since
"!ve_provided && WARN_ON_ONCE(!in_task())" is self-descriptive.

> 
> [ ... ]
> 
>> +static asmlinkage int ve_vprintk_deferred(int dst, struct ve_struct *ve,
>> +					const char *fmt, va_list args)
> 
> [Severity: Low]
> This isn't a bug, but this line adds trailing whitespace after
> "struct ve_struct *ve, ".

I was looking for this whitespace for days...

> 
> [ ... ]
> 

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

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