Re: [Devel] [PATCH hci-8.1 5/6] ve/failcount: Use ve_printk with specified ve

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] printk: !CONFIG_PRINTK __ve_printk_ratelimited() stub lacks the ve parameter
--

commit 446bb8b8f19e9083361fc5d794bba0859020d3a4
Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

ve/failcount: Use ve_printk with specified ve

This patch adds __ve_printk_ratelimited() and __ve_pr_warn_ratelimited()
helpers that take an explicit ve argument. ve_failcount_inc() now uses them
so the message goes to the right VE when called from IRQ context.

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

[ ... ]

>  #else
>  #define printk_ratelimited(fmt, ...)					\
>  	no_printk(fmt, ##__VA_ARGS__)
>  #define ve_printk_ratelimited(dst, fmt, ...)				\
>  	no_printk(fmt, ##__VA_ARGS__)
> +#define __ve_printk_ratelimited(dst, fmt, ...)				\
> +	no_printk(fmt, ##__VA_ARGS__)
>  #endif

[Severity: Low]
Is the parameter list of this !CONFIG_PRINTK stub correct? The
CONFIG_PRINTK version of __ve_printk_ratelimited() takes
(dst, ve, fmt, ...), but this stub takes only (dst, fmt, ...).

[ ... ]

> +#define __ve_pr_warn_ratelimited(dst, ve, fmt, ...)				\
> +	__ve_printk_ratelimited(dst, ve, KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)

This macro is defined outside the #ifdef and always passes ve as the second
argument. With CONFIG_VE=y and CONFIG_PRINTK=n, it expands into the
mismatched stub, so ve is bound to fmt.

> diff --git a/include/linux/ve.h b/include/linux/ve.h
> --- a/include/linux/ve.h
> +++ b/include/linux/ve.h

[ ... ]

> +	__ve_pr_warn_ratelimited(VE_LOG_BOTH, __ve, "CT%s: hits the " #name	\
> +		" limit, see the '" #name "' counter in ve.failcount "		\
> +		"of the container's ve cgroup\n", ve_name(__ve));		\

With CONFIG_PRINTK=n, ve_failcount_inc() expands like this:

ve_failcount_inc()
  __ve_pr_warn_ratelimited(VE_LOG_BOTH, __ve, "CT%s: ...", ve_name(__ve))
    __ve_printk_ratelimited(dst, ve, KERN_WARNING pr_fmt(fmt), ...)
      no_printk(<struct ve_struct *>, <real format>, ve_name(__ve))

no_printk() then receives a struct ve_struct pointer as its const char *
format argument. The real format string and ve_name(__ve) end up in the
varargs.

Can this produce an incompatible-pointer diagnostic, which is an error with
-Werror or newer compilers? The format string also stops being checked.

Should the stub be changed to take (dst, ve, fmt, ...)? As written, this
can break the build when CONFIG_VE=y and CONFIG_PRINTK=n.

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

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