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

[no-reply@sashiko.virtuozzo.com]

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?

[ ... ]

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

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

[ ... ]

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

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

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