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

[no-reply@sashiko.virtuozzo.com]

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

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

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

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

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

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

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

[ ... ]

> +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, ".

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261006-ve-printk-irq-v2-0-db1dc4f6ed07@virtuozzo.com?part=3

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