[Devel] [PATCH hci-8.1 0/6] ve/printk: Fix logs leakage via ve_printk called from IRQ context

[Devel] [PATCH hci-8.1 0/6] ve/printk: Fix logs leakage via ve_printk called from IRQ context

[Vladimir Riabchun]

Printk function that printed logs to containers' dmesgs mostly used
get_exec_env() for getting VE. This is fine for task contexts and
might be wrong for IRQ contexts.

This patchset fixes this in the following way:
 - Report non-task usages of ve_printk function which get
   VE from current task. Do it with WARN so users will be
   frightened and will report it to us so we can fix it.
   This will also make an issue visible to our internal tests.
 - Reject printing to containers when VE is unreliable.
 - Calculate correct time offsets when calling printk. It can
   be delayed and the context might change, we can see time
   travels if time was taken from a different container.

With these fixes failcounts can be now used in any context,
so the last failcount - neigh_tbl - is added.

This was tested by our neoltp tests on host and inside containers.
No tests crashed, so there an no obvious IRQ callers of ve_printk
functions.

https://virtuozzo.atlassian.net/browse/VSTOR-146839

Feature: ve printk
Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
---
Vladimir Riabchun (6):
      ve/printk: Add sanity checks in ve_printk functions
      ve/printk: Report potentially leaking usage of ve_printk functions
      ve/printk: Specify ve in printks
      ve/printk: Fix time virtualization
      ve/failcount: Use ve_printk with specified ve
      ve: add neigh_tbl failcounter

 drivers/base/core.c                 |   6 +-
 include/linux/net.h                 |  16 ++---
 include/linux/printk.h              |  37 ++++++++--
 include/linux/ratelimit_types.h     |   6 +-
 include/linux/ve.h                  |  17 ++---
 kernel/printk/internal.h            |   6 ++
 kernel/printk/printk.c              | 132 ++++++++++++++++++++++++++++--------
 kernel/printk/printk_ringbuffer.h   |   2 +
 kernel/ve/ve.c                      |   3 +
 lib/ratelimit.c                     |   7 +-
 net/core/neighbour.c                |   3 +-
 net/core/utils.c                    |   4 +-
 net/netfilter/nf_conntrack_core.c   |   2 +-
 net/netfilter/nf_conntrack_expect.c |   2 +-
 net/netfilter/xt_TCPMSS.c           |   4 +-
 15 files changed, 181 insertions(+), 66 deletions(-)
---
base-commit: ca54f152160587a74ff17cb52feef16f7b9d4e2b
change-id: 20260923-ve-printk-irq-027c3378660f

Best regards,
-- 
Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

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

[Devel] [PATCH hci-8.1 1/6] ve/printk: Add sanity checks in ve_printk functions

[Vladimir Riabchun]

This patch adds dst sanity check and reorders VE_LOG checks
in if.

https://virtuozzo.atlassian.net/browse/VSTOR-146839

Feature: ve printk
Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
---
 drivers/base/core.c    | 4 +++-
 kernel/printk/printk.c | 8 ++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index ebc6c5570a4d..2d233ffb5661 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4945,12 +4945,14 @@ int ve_dev_printk_emit(int dst, int level, const struct device *dev, const char
 	va_list args, args2;
 	int r;
 
+	WARN_ON_ONCE(!dst || (dst & ~VE_LOG_BOTH));
+
 	va_start(args, fmt);
 	va_copy(args2, args);
 
 	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
 		r = dev_vprintk_emit(level, dev, fmt, args);
-	if (!ve_is_super(get_exec_env()) && (dst & VE_LOG))
+	if ((dst & VE_LOG) && !ve_is_super(get_exec_env()))
 		r = ve_dev_vprintk_emit(level, dev, fmt, args2);
 
 	va_end(args2);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 8940b36e790b..efe2c3279c85 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2535,10 +2535,12 @@ static asmlinkage int ve_vprintk(int dst, const char *fmt, va_list args)
 	va_list args2;
 	int r = 0;
 
+	WARN_ON_ONCE(!dst || (dst & ~VE_LOG_BOTH));
+
 	va_copy(args2, args);
 	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
 		r = vprintk(fmt, args);
-	if (!ve_is_super(get_exec_env()) && (dst & VE_LOG))
+	if ((dst & VE_LOG) && !ve_is_super(get_exec_env()))
 		r = ve_vprintk_emit(0, LOGLEVEL_DEFAULT, NULL, fmt, args2);
 	va_end(args2);
 
@@ -2567,10 +2569,12 @@ static asmlinkage int ve_vprintk_deferred(int dst, const char *fmt, va_list args
 	va_list args2;
 	int r = 0;
 
+	WARN_ON_ONCE(!dst || (dst & ~VE_LOG_BOTH));
+
 	va_copy(args2, args);
 	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
 		r = vprintk_emit(0, LOGLEVEL_SCHED, NULL, fmt, args);
-	if (!ve_is_super(get_exec_env()) && (dst & VE_LOG))
+	if ((dst & VE_LOG) && !ve_is_super(get_exec_env()))
 		r = ve_vprintk_emit(0, LOGLEVEL_SCHED, NULL, fmt, args2);
 	va_end(args2);
 

-- 
2.52.0

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

Re: [Devel] [PATCH hci-8.1 1/6] ve/printk: Add sanity checks in ve_printk functions

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] printk: WARN_ON_ONCE() in ve_vprintk_deferred() uses non-deferred printk path
--

commit 2b22fc2da5243bd2b5c695100f66ab107b28c158
Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

ve/printk: Add sanity checks in ve_printk functions

This patch adds a sanity check on the dst argument in the ve_printk
helpers. It also reorders the VE_LOG test so the dst bit is checked
before ve_is_super().

Link: https://virtuozzo.atlassian.net/browse/VSTOR-146839

> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c

[ ... ]

> @@ -2567,10 +2569,12 @@ static asmlinkage int ve_vprintk_deferred(int dst, const char *fmt, va_list args
>  	va_list args2;
>  	int r = 0;
>  
> +	WARN_ON_ONCE(!dst || (dst & ~VE_LOG_BOTH));
> +
>  	va_copy(args2, args);
>  	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
>  		r = vprintk_emit(0, LOGLEVEL_SCHED, NULL, fmt, args);

[Severity: Low]
This isn't a bug in the normal case, but is the new WARN_ON_ONCE() safe
in ve_vprintk_deferred()?

This function exists for contexts where the regular printk path is
unsafe, which is why it emits at LOGLEVEL_SCHED through vprintk_emit().
WARN_ON_ONCE() reports through the normal printk and console path. It
can also panic if panic_on_warn is set.

If a caller passes dst == 0 or a dst with bits outside VE_LOG_BOTH from
such a context, could the warning recurse into scheduler or console
locks that the deferred variant is meant to avoid?

I did not find a caller that passes an invalid dst. The trigger would
need a buggy caller, and the warning fires only once.

Would a BUILD_BUG_ON() for constant arguments be more suitable here?
Alternatively, the report could go through a deferred-safe mechanism
such as printk_deferred().

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

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

Re: [Devel] [PATCH hci-8.1 1/6] ve/printk: Add sanity checks in ve_printk functions

[Vladimir Riabchun]

On 10/2/26 23:31, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] printk: WARN_ON_ONCE() in ve_vprintk_deferred() uses non-deferred printk path
> --
> 
> commit 2b22fc2da5243bd2b5c695100f66ab107b28c158
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve/printk: Add sanity checks in ve_printk functions
> 
> This patch adds a sanity check on the dst argument in the ve_printk
> helpers. It also reorders the VE_LOG test so the dst bit is checked
> before ve_is_super().
> 
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-146839
> 
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
> 
> [ ... ]
> 
>> @@ -2567,10 +2569,12 @@ static asmlinkage int ve_vprintk_deferred(int dst, const char *fmt, va_list args
>>   	va_list args2;
>>   	int r = 0;
>>   
>> +	WARN_ON_ONCE(!dst || (dst & ~VE_LOG_BOTH));
>> +
>>   	va_copy(args2, args);
>>   	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
>>   		r = vprintk_emit(0, LOGLEVEL_SCHED, NULL, fmt, args);
> 
> [Severity: Low]
> This isn't a bug in the normal case, but is the new WARN_ON_ONCE() safe
> in ve_vprintk_deferred()?
> 
> This function exists for contexts where the regular printk path is
> unsafe, which is why it emits at LOGLEVEL_SCHED through vprintk_emit().
> WARN_ON_ONCE() reports through the normal printk and console path. It
> can also panic if panic_on_warn is set.
> 
> If a caller passes dst == 0 or a dst with bits outside VE_LOG_BOTH from
> such a context, could the warning recurse into scheduler or console
> locks that the deferred variant is meant to avoid?
> 
> I did not find a caller that passes an invalid dst. The trigger would
> need a buggy caller, and the warning fires only once.
> 
> Would a BUILD_BUG_ON() for constant arguments be more suitable here?
> Alternatively, the report could go through a deferred-safe mechanism
> such as printk_deferred().

We can't know passed values when compiling this file.
Also, WARN_ON doesn't print anything itself, instead it executes UD2,
leaving all the job to the exception handler.

And kernel does use WARNs in sched code: check 
force_compatible_cpus_allowed_ptr,
it has both WARN_ON and printk_deferred.

> 

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

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

[Devel] [PATCH hci-8.1 2/6] ve/printk: Report potentially leaking usage of ve_printk functions

[Vladimir Riabchun]

We don't want to rely on get_exec_env() value in non-task context.
When this happens, let's issue a WARN and either reject printing or
return host log state.

Some messages may probably go to host dmesg instead of CT, but at
least we won't leak info to random container.

https://virtuozzo.atlassian.net/browse/VSTOR-146839

Feature: ve printk
Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
---
 drivers/base/core.c    |  4 +++-
 kernel/printk/printk.c | 11 ++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2d233ffb5661..a92c80603ae3 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4952,7 +4952,9 @@ int ve_dev_printk_emit(int dst, int level, const struct device *dev, const char
 
 	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
 		r = dev_vprintk_emit(level, dev, fmt, args);
-	if ((dst & VE_LOG) && !ve_is_super(get_exec_env()))
+
+	/* 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_dev_vprintk_emit(level, dev, fmt, args2);
 
 	va_end(args2);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index efe2c3279c85..b06fc835d7c7 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -560,7 +560,8 @@ struct log_state *ve_log_state(void)
 {
 	struct log_state *log = &init_log_state;
 #ifdef CONFIG_VE
-	if (get_exec_env()->log_state)
+	/* get_exec_env() is reliable only in task context */
+	if (!WARN_ON_ONCE(!in_task()) && get_exec_env()->log_state)
 		log = get_exec_env()->log_state;
 #endif
 	return log;
@@ -2540,7 +2541,9 @@ static asmlinkage int ve_vprintk(int dst, const char *fmt, va_list args)
 	va_copy(args2, args);
 	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
 		r = vprintk(fmt, args);
-	if ((dst & VE_LOG) && !ve_is_super(get_exec_env()))
+
+	/* 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);
 	va_end(args2);
 
@@ -2574,7 +2577,9 @@ static asmlinkage int ve_vprintk_deferred(int dst, const char *fmt, va_list args
 	va_copy(args2, args);
 	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
 		r = vprintk_emit(0, LOGLEVEL_SCHED, NULL, fmt, args);
-	if ((dst & VE_LOG) && !ve_is_super(get_exec_env()))
+
+	/* 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_SCHED, NULL, fmt, args2);
 	va_end(args2);
 

-- 
2.52.0

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

Re: [Devel] [PATCH hci-8.1 2/6] ve/printk: Report potentially leaking usage of ve_printk functions

[no-reply]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] printk: WARN_ON_ONCE() added to ve_vprintk_deferred() and ve_log_state() on printk-sensitive paths
- [Low] ve: ve_dev_printk_emit() returns uninitialized r when VE_LOG is rejected in non-task context
- [Low] printk: in_task() WARN is evaluated before ve_is_super(), so host-context calls warn spuriously
- [Low] printk: host-log decision still uses get_exec_env() in non-task context
--

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

ve/printk: Report potentially leaking usage of ve_printk functions

This patch adds WARN_ON_ONCE(!in_task()) checks to the ve printk helpers
and to ve_log_state(). The goal is to avoid relying on get_exec_env() in
non-task context, and either reject VE printing or fall back to the host
log state.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-146839

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c

[ ... ]

> @@ -4952,7 +4952,9 @@ int ve_dev_printk_emit(int dst, int level, const struct device *dev, const char
>  
>  	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
>  		r = dev_vprintk_emit(level, dev, fmt, args);
> -	if ((dst & VE_LOG) && !ve_is_super(get_exec_env()))
> +
> +	/* 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_dev_vprintk_emit(level, dev, fmt, args2);

[Severity: Low]
Can ve_dev_printk_emit() return an uninitialized value here?

The local variable is declared as "int r;" with no initializer. If
ve_dev_printk_emit(VE_LOG, ...) is called from IRQ, softirq or NMI
context while the interrupted task is in a non-super container, the
first branch is not taken because ve_is_super() is false and VE0_LOG
is not set. The second branch is now skipped because
WARN_ON_ONCE(!in_task()) is true. The function then does "return r"
with r never assigned.

Before this patch every valid dst assigned r in at least one branch.
The sibling functions ve_vprintk() and ve_vprintk_deferred() use
"int r = 0". Should r be initialized the same way here?

[Severity: Low]
Is it intended that WARN_ON_ONCE(!in_task()) is evaluated before
ve_is_super() in this condition?

With the current ordering, ve_dev_printk_emit(VE_LOG, ...) called from
IRQ context while the interrupted task is in the host (VE0) triggers
the warning, even though a host caller cannot leak into a container.
Because the warning is once per site, an early harmless hit could also
suppress a later hit that really would leak. The same ordering appears
in ve_vprintk() and ve_vprintk_deferred() below. Would testing
!ve_is_super(get_exec_env()) first limit the warning to the
potentially leaking case?

[Severity: Low]
Is the host-log decision in the preceding branch still based on
get_exec_env() in non-task context?

	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
		r = dev_vprintk_emit(level, dev, fmt, args);

If ve_dev_printk_emit(VE_LOG, ...) is called from IRQ and the
interrupted task is in the host, the message goes to the host log. If
the interrupted task is in a container, the VE branch is rejected by
the WARN and the message is dropped. The commit message says printing
is either rejected or the host log state is returned, but this branch
still depends on whichever task was interrupted. The same pattern
exists in ve_vprintk() and ve_vprintk_deferred(). Is that routing
intended?

> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -560,7 +560,8 @@ struct log_state *ve_log_state(void)
>  {
>  	struct log_state *log = &init_log_state;
>  #ifdef CONFIG_VE
> -	if (get_exec_env()->log_state)
> +	/* get_exec_env() is reliable only in task context */
> +	if (!WARN_ON_ONCE(!in_task()) && get_exec_env()->log_state)
>  		log = get_exec_env()->log_state;
>  #endif
>  	return log;

[Severity: Medium]
Can this WARN_ON_ONCE() recurse into printk?

ve_log_state() is reached from ve_vprintk_emit() and then
vprintk_emit_log(), so the WARN fires in the middle of the printk emit
path. A WARN prints a message and backtrace through the regular
non-deferred printk, and it can panic under panic_on_warn.

[ ... ]

> @@ -2540,7 +2541,9 @@ static asmlinkage int ve_vprintk(int dst, const char *fmt, va_list args)
>  	va_copy(args2, args);
>  	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
>  		r = vprintk(fmt, args);
> -	if ((dst & VE_LOG) && !ve_is_super(get_exec_env()))
> +
> +	/* 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);
>  	va_end(args2);

[ ... ]

> @@ -2574,7 +2577,9 @@ static asmlinkage int ve_vprintk_deferred(int dst, const char *fmt, va_list args
>  	va_copy(args2, args);
>  	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
>  		r = vprintk_emit(0, LOGLEVEL_SCHED, NULL, fmt, args);
> -	if ((dst & VE_LOG) && !ve_is_super(get_exec_env()))
> +
> +	/* 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_SCHED, NULL, fmt, args2);
>  	va_end(args2);

[Severity: Medium]
Is a synchronous WARN_ON_ONCE() safe inside ve_vprintk_deferred()?

This helper exists for contexts where regular printk is unsafe, such as
scheduler code or code holding rq or timekeeping locks. If a caller
uses ve_printk_deferred(VE_LOG, ...) from hardirq, softirq or
scheduler-related context, the WARN fires inside
ve_vprintk_deferred() and prints through the non-deferred printk with
a backtrace. Can this deadlock or recurse into printk, or panic under
panic_on_warn?

The WARN added in ve_log_state() has the same concern on the
ve_vprintk_emit() path used here.

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

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

Re: [Devel] [PATCH hci-8.1 2/6] ve/printk: Report potentially leaking usage of ve_printk functions

[Vladimir Riabchun]

On 10/2/26 23:32, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [Medium] printk: WARN_ON_ONCE() added to ve_vprintk_deferred() and ve_log_state() on printk-sensitive paths
> - [Low] ve: ve_dev_printk_emit() returns uninitialized r when VE_LOG is rejected in non-task context
> - [Low] printk: in_task() WARN is evaluated before ve_is_super(), so host-context calls warn spuriously
> - [Low] printk: host-log decision still uses get_exec_env() in non-task context
> --
> 
> commit b1f1fbc536b0ab55f70c0064fc9579c59cbe13a9
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve/printk: Report potentially leaking usage of ve_printk functions
> 
> This patch adds WARN_ON_ONCE(!in_task()) checks to the ve printk helpers
> and to ve_log_state(). The goal is to avoid relying on get_exec_env() in
> non-task context, and either reject VE printing or fall back to the host
> log state.
> 
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-146839
> 
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
> 
> [ ... ]
> 
>> @@ -4952,7 +4952,9 @@ int ve_dev_printk_emit(int dst, int level, const struct device *dev, const char
>>   
>>   	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
>>   		r = dev_vprintk_emit(level, dev, fmt, args);
>> -	if ((dst & VE_LOG) && !ve_is_super(get_exec_env()))
>> +
>> +	/* 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_dev_vprintk_emit(level, dev, fmt, args2);
> 
> [Severity: Low]
> Can ve_dev_printk_emit() return an uninitialized value here?
> 
> The local variable is declared as "int r;" with no initializer. If
> ve_dev_printk_emit(VE_LOG, ...) is called from IRQ, softirq or NMI
> context while the interrupted task is in a non-super container, the
> first branch is not taken because ve_is_super() is false and VE0_LOG
> is not set. The second branch is now skipped because
> WARN_ON_ONCE(!in_task()) is true. The function then does "return r"
> with r never assigned.
> 
> Before this patch every valid dst assigned r in at least one branch.
> The sibling functions ve_vprintk() and ve_vprintk_deferred() use
> "int r = 0". Should r be initialized the same way here?

True, will fix in v2.

> 
> [Severity: Low]
> Is it intended that WARN_ON_ONCE(!in_task()) is evaluated before
> ve_is_super() in this condition?
> 
> With the current ordering, ve_dev_printk_emit(VE_LOG, ...) called from
> IRQ context while the interrupted task is in the host (VE0) triggers
> the warning, even though a host caller cannot leak into a container.
> Because the warning is once per site, an early harmless hit could also
> suppress a later hit that really would leak. The same ordering appears
> in ve_vprintk() and ve_vprintk_deferred() below. Would testing
> !ve_is_super(get_exec_env()) first limit the warning to the
> potentially leaking case?

It doesn't matter that this hit was harmless. We report problematic
places, not problematic situations - interrupted task is a random value.

> 
> [Severity: Low]
> Is the host-log decision in the preceding branch still based on
> get_exec_env() in non-task context?
> 
> 	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
> 		r = dev_vprintk_emit(level, dev, fmt, args);
> 
> If ve_dev_printk_emit(VE_LOG, ...) is called from IRQ and the
> interrupted task is in the host, the message goes to the host log. If
> the interrupted task is in a container, the VE branch is rejected by
> the WARN and the message is dropped. The commit message says printing
> is either rejected or the host log state is returned, but this branch
> still depends on whichever task was interrupted. The same pattern
> exists in ve_vprintk() and ve_vprintk_deferred(). Is that routing
> intended?

We might miss some logs in the described case, and these logs might
go to the host log instead of container's one.
Both cases are fine IMO and commit allows both. This is a sort of
best-effort printing.

> 
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -560,7 +560,8 @@ struct log_state *ve_log_state(void)
>>   {
>>   	struct log_state *log = &init_log_state;
>>   #ifdef CONFIG_VE
>> -	if (get_exec_env()->log_state)
>> +	/* get_exec_env() is reliable only in task context */
>> +	if (!WARN_ON_ONCE(!in_task()) && get_exec_env()->log_state)
>>   		log = get_exec_env()->log_state;
>>   #endif
>>   	return log;
> 
> [Severity: Medium]
> Can this WARN_ON_ONCE() recurse into printk?

It is fine.

> 
> ve_log_state() is reached from ve_vprintk_emit() and then
> vprintk_emit_log(), so the WARN fires in the middle of the printk emit
> path. A WARN prints a message and backtrace through the regular
> non-deferred printk, and it can panic under panic_on_warn.
> 
> [ ... ]
> 
>> @@ -2540,7 +2541,9 @@ static asmlinkage int ve_vprintk(int dst, const char *fmt, va_list args)
>>   	va_copy(args2, args);
>>   	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
>>   		r = vprintk(fmt, args);
>> -	if ((dst & VE_LOG) && !ve_is_super(get_exec_env()))
>> +
>> +	/* 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);
>>   	va_end(args2);
> 
> [ ... ]
> 
>> @@ -2574,7 +2577,9 @@ static asmlinkage int ve_vprintk_deferred(int dst, const char *fmt, va_list args
>>   	va_copy(args2, args);
>>   	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
>>   		r = vprintk_emit(0, LOGLEVEL_SCHED, NULL, fmt, args);
>> -	if ((dst & VE_LOG) && !ve_is_super(get_exec_env()))
>> +
>> +	/* 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_SCHED, NULL, fmt, args2);
>>   	va_end(args2);
> 
> [Severity: Medium]
> Is a synchronous WARN_ON_ONCE() safe inside ve_vprintk_deferred()?
> 
> This helper exists for contexts where regular printk is unsafe, such as
> scheduler code or code holding rq or timekeeping locks. If a caller
> uses ve_printk_deferred(VE_LOG, ...) from hardirq, softirq or
> scheduler-related context, the WARN fires inside
> ve_vprintk_deferred() and prints through the non-deferred printk with
> a backtrace. Can this deadlock or recurse into printk, or panic under
> panic_on_warn?
> 
> The WARN added in ve_log_state() has the same concern on the
> ve_vprintk_emit() path used here.

It should be fine, WARNs are used in sched code. See reply to the
previous patch.

> 

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

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

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

[Vladimir Riabchun]

Introduce ve_printk helpers that accept VE structure,
so we can provide correct VE where get_exec_env is unreliable.

This is true for ve_printk_deferred functions, and their users.
Now VE ratelimiting requires specified VE, and this requirement
is further forwarded to ve_net_ratelimited kind.

https://virtuozzo.atlassian.net/browse/VSTOR-146839

Feature: ve printk
Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
---
 include/linux/net.h                 | 16 ++++----
 include/linux/printk.h              | 23 ++++++++++--
 include/linux/ratelimit_types.h     |  6 ++-
 kernel/printk/printk.c              | 74 +++++++++++++++++++++++++++++++------
 lib/ratelimit.c                     |  7 ++--
 net/core/utils.c                    |  4 +-
 net/netfilter/nf_conntrack_core.c   |  2 +-
 net/netfilter/nf_conntrack_expect.c |  2 +-
 net/netfilter/xt_TCPMSS.c           |  4 +-
 9 files changed, 104 insertions(+), 34 deletions(-)

diff --git a/include/linux/net.h b/include/linux/net.h
index 3f3bbe9f149d..4c34b9a0386f 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -265,7 +265,7 @@ struct socket *sockfd_lookup(int fd, int *err);
 struct socket *sock_from_file(struct file *file);
 #define		     sockfd_put(sock) fput(sock->file)
 int net_ratelimit(void);
-int ve_net_ratelimit(int dst);
+int ve_net_ratelimit(int dst, struct ve_struct *ve);
 
 #define net_ratelimited_function(function, ...)			\
 do {								\
@@ -273,10 +273,10 @@ do {								\
 		function(__VA_ARGS__);				\
 } while (0)
 
-#define ve_net_ratelimited_function(function, dst, ...)		\
+#define ve_net_ratelimited_function(function, dst, ve, ...)	\
 do {								\
-	if (ve_net_ratelimit(dst))				\
-		function(dst, __VA_ARGS__);			\
+	if (ve_net_ratelimit(dst, ve))				\
+		function(dst, ve, __VA_ARGS__);			\
 } while (0)
 
 #define net_emerg_ratelimited(fmt, ...)				\
@@ -311,10 +311,10 @@ do {									\
 	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
 #endif
 
-#define net_velog_ratelimited(fmt, ...)                                \
-	ve_net_ratelimited_function(ve_printk, VE_LOG, fmt, ##__VA_ARGS__)
-#define net_veboth_ratelimited(fmt, ...)                               \
-	ve_net_ratelimited_function(ve_printk, VE_LOG_BOTH, fmt, ##__VA_ARGS__)
+#define net_velog_ratelimited(ve, fmt, ...)						\
+	ve_net_ratelimited_function(__ve_printk, VE_LOG, ve, fmt, ##__VA_ARGS__)
+#define net_veboth_ratelimited(ve, fmt, ...)						\
+	ve_net_ratelimited_function(__ve_printk, VE_LOG_BOTH, ve, fmt, ##__VA_ARGS__)
 
 #define net_get_random_once(buf, nbytes)			\
 	get_random_once((buf), (nbytes))
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 3b0af0deb6c3..1478e47dc582 100644
--- 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);
 
@@ -160,6 +161,11 @@ int ve_vprintk_emit(int facility, int level,
 		    const struct dev_printk_info *dev_info,
 		    const char *fmt, va_list args);
 
+asmlinkage __printf(5, 0)
+int __ve_vprintk_emit(struct ve_struct *ve, int facility, int level,
+		    const struct dev_printk_info *dev_info,
+		    const char *fmt, va_list args);
+
 asmlinkage __printf(1, 0)
 int vprintk(const char *fmt, va_list args);
 __printf(1, 0)
@@ -171,8 +177,11 @@ int _printk(const char *fmt, ...);
 asmlinkage __printf(2, 3) __cold
 int ve_printk(int dst, const char *fmt, ...);
 
-asmlinkage __printf(2, 3) __cold
-int ve_printk_deferred(int dst, const char *fmt, ...);
+asmlinkage __printf(3, 4) __cold
+int __ve_printk(int dst, struct ve_struct *ve, const char *fmt, ...);
+
+asmlinkage __printf(3, 4) __cold
+int ve_printk_deferred(int dst, struct ve_struct *ve, const char *fmt, ...);
 
 struct ve_struct;
 int ve_log_init(struct ve_struct *ve);
@@ -248,8 +257,13 @@ int ve_printk(int dst, const char *s, ...)
 {
 	return 0;
 }
+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;
 }
@@ -747,7 +761,8 @@ struct pi_entry {
 				      DEFAULT_RATELIMIT_INTERVAL,	\
 				      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__);			\
 })
 #else
diff --git a/include/linux/ratelimit_types.h b/include/linux/ratelimit_types.h
index 3902066f0fee..aa9eee2a2455 100644
--- a/include/linux/ratelimit_types.h
+++ b/include/linux/ratelimit_types.h
@@ -44,7 +44,9 @@ struct ratelimit_state {
 extern int ___ratelimit(struct ratelimit_state *rs, const char *func);
 #define __ratelimit(state) ___ratelimit(state, __func__)
 
-extern int __ve_ratelimit(int dst, struct ratelimit_state *rs, const char *func);
-#define ve_ratelimit(dst, state) __ve_ratelimit(dst, state, __func__)
+struct ve_struct;
+extern int __ve_ratelimit(int dst, struct ve_struct *ve,
+	struct ratelimit_state *rs, const char *func);
+#define ve_ratelimit(dst, ve, state) __ve_ratelimit(dst, ve, state, __func__)
 
 #endif /* _LINUX_RATELIMIT_TYPES_H */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index b06fc835d7c7..df7cf83fb4af 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -567,6 +567,16 @@ struct log_state *ve_log_state(void)
 	return log;
 }
 
+struct log_state *__ve_log_state(struct ve_struct *ve)
+{
+	struct log_state *log = &init_log_state;
+#ifdef CONFIG_VE
+	if (ve->log_state)
+		log = ve->log_state;
+#endif
+	return log;
+}
+
 void log_poll_wait(struct file *filp, poll_table *p)
 {
 	poll_wait(filp, &ve_log_state()->wait, p);
@@ -2525,26 +2535,45 @@ asmlinkage int ve_vprintk_emit(int facility, int level,
 }
 EXPORT_SYMBOL(ve_vprintk_emit);
 
+asmlinkage int __ve_vprintk_emit(struct ve_struct *ve, int facility, int level,
+			       const struct dev_printk_info *dev_info,
+			       const char *fmt, va_list args)
+{
+	return vprintk_emit_log(__ve_log_state(ve),
+				facility, level, dev_info,
+				fmt, args);
+}
+EXPORT_SYMBOL(__ve_vprintk_emit);
+
 int vprintk_default(const char *fmt, va_list args)
 {
 	return vprintk_emit(0, LOGLEVEL_DEFAULT, NULL, fmt, args);
 }
 EXPORT_SYMBOL_GPL(vprintk_default);
 
-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);
 
 	/* 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);
 
 	return r;
@@ -2560,39 +2589,62 @@ asmlinkage int ve_printk(int dst, const char *fmt, ...)
 	int r;
 
 	va_start(args, fmt);
-	r = ve_vprintk(dst, fmt, args);
+	r = ve_vprintk(dst, NULL, fmt, args);
 	va_end(args);
 
 	return r;
 }
 EXPORT_SYMBOL(ve_printk);
 
-static asmlinkage int ve_vprintk_deferred(int dst, const char *fmt, va_list args)
+asmlinkage int __ve_printk(int dst, struct ve_struct *ve, const char *fmt, ...)
+{
+	va_list args;
+	int r;
+
+	va_start(args, fmt);
+	r = ve_vprintk(dst, ve, fmt, args);
+	va_end(args);
+
+	return r;
+}
+EXPORT_SYMBOL(__ve_printk);
+
+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);
 
 	/* 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_SCHED, 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_SCHED, NULL, fmt, args2);
+	}
+done:
 	va_end(args2);
 
 	return r;
 }
 
-asmlinkage int ve_printk_deferred(int dst, const char *fmt, ...)
+asmlinkage int ve_printk_deferred(int dst, struct ve_struct *ve,
+	const char *fmt, ...)
 {
 	va_list args;
 	int r;
 
 	va_start(args, fmt);
-	r = ve_vprintk_deferred(dst, fmt, args);
+	r = ve_vprintk_deferred(dst, ve, fmt, args);
 	va_end(args);
 
 	return r;
diff --git a/lib/ratelimit.c b/lib/ratelimit.c
index 98409e870eea..98c45bb19e50 100644
--- a/lib/ratelimit.c
+++ b/lib/ratelimit.c
@@ -11,6 +11,7 @@
 #include <linux/ratelimit.h>
 #include <linux/jiffies.h>
 #include <linux/export.h>
+#include <linux/ve_proto.h>
 
 /*
  * __ratelimit - rate limiting
@@ -24,7 +25,7 @@
  * 0 means callbacks will be suppressed.
  * 1 means go ahead and do it.
  */
-int __ve_ratelimit(int dst, struct ratelimit_state *rs, const char *func)
+int __ve_ratelimit(int dst, struct ve_struct *ve, struct ratelimit_state *rs, const char *func)
 {
 	/* Paired with WRITE_ONCE() in .proc_handler().
 	 * Changing two values seperately could be inconsistent
@@ -55,7 +56,7 @@ int __ve_ratelimit(int dst, struct ratelimit_state *rs, const char *func)
 
 		if (m) {
 			if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE)) {
-				ve_printk_deferred(dst, KERN_WARNING
+				ve_printk_deferred(dst, ve, KERN_WARNING
 						"%s: %d callbacks suppressed\n", func, m);
 			}
 		}
@@ -77,6 +78,6 @@ EXPORT_SYMBOL(__ve_ratelimit);
 
 int ___ratelimit(struct ratelimit_state *rs, const char *func)
 {
-	return __ve_ratelimit(VE0_LOG, rs, func);
+	return __ve_ratelimit(VE0_LOG, get_ve0(), rs, func);
 }
 EXPORT_SYMBOL(___ratelimit);
diff --git a/net/core/utils.c b/net/core/utils.c
index cffc3bc44baa..bf4e53205a8e 100644
--- a/net/core/utils.c
+++ b/net/core/utils.c
@@ -41,9 +41,9 @@ int net_ratelimit(void)
 }
 EXPORT_SYMBOL(net_ratelimit);
 
-int ve_net_ratelimit(int dst)
+int ve_net_ratelimit(int dst, struct ve_struct *ve)
 {
-	return ve_ratelimit(dst, &net_ratelimit_state);
+	return ve_ratelimit(dst, ve, &net_ratelimit_state);
 }
 EXPORT_SYMBOL(ve_net_ratelimit);
 
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index f078136edddd..8900a41b48b6 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1701,7 +1701,7 @@ __nf_conntrack_alloc(struct net *net,
 			if (!conntrack_gc_work.early_drop)
 				conntrack_gc_work.early_drop = true;
 			atomic_dec(&cnet->count);
-			net_veboth_ratelimited(KERN_WARNING "VE%s: "
+			net_veboth_ratelimited(net->owner_ve, KERN_WARNING "VE%s: "
 					       "nf_conntrack table full in netns %u, dropping packet\n",
 					       ve_name(net->owner_ve), net->ns.inum);
 			return ERR_PTR(-ENOMEM);
diff --git a/net/netfilter/nf_conntrack_expect.c b/net/netfilter/nf_conntrack_expect.c
index 4b077b2cb432..23e78957bb79 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -508,7 +508,7 @@ static inline int __nf_ct_expect_check(struct nf_conntrack_expect *expect,
 
 	cnet = nf_ct_pernet(net);
 	if (cnet->expect_count >= cnet->expect_max) {
-		net_veboth_ratelimited(KERN_WARNING "VE%s: "
+		net_veboth_ratelimited(net->owner_ve, KERN_WARNING "VE%s: "
 					"nf_conntrack: expectation table full in netns %u\n",
 					ve_name(net->owner_ve), net->ns.inum);
 		ret = -EMFILE;
diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
index 822b3f71570e..0b9ccc8987e2 100644
--- a/net/netfilter/xt_TCPMSS.c
+++ b/net/netfilter/xt_TCPMSS.c
@@ -105,8 +105,8 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 		unsigned int min_mtu = min(dst_mtu(skb_dst(skb)), in_mtu);
 
 		if (min_mtu <= minlen) {
-			net_velog_ratelimited("unknown or invalid path-MTU (%u)\n",
-					    min_mtu);
+			net_velog_ratelimited(net->owner_ve, "unknown or invalid path-MTU (%u)\n",
+					      min_mtu);
 			return -1;
 		}
 		newmss = min_mtu - minlen;

-- 
2.52.0

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

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

[no-reply]

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

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

[Vladimir Riabchun]

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

[Devel] [PATCH hci-8.1 4/6] ve/printk: Fix time virtualization

[Vladimir Riabchun]

The usage of get_exec_env() in ve_timens_add_boottime_ns can lead
to time travels - a printk from IRQ might use container time for
printing to host dmesg and vice versa.

Fix this by adding timens_offsets info to printk_info entry. This way
correct time is provided by the original printer of message, rather than
printing job, that may be delayed and called from a different context.
It seems complicated to keep time namespace in log_state, because
VE creation and namespace creation happen at different times and it
seems that one VE can have different time namespaces if it was stopped
and then started again.
So instead of tracking time namespaces, let's put actual time offsets
into log state.

https://virtuozzo.atlassian.net/browse/VSTOR-146839

Feature: ve printk
Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
---
 kernel/printk/internal.h          |  6 +++++
 kernel/printk/printk.c            | 51 ++++++++++++++++++++++++---------------
 kernel/printk/printk_ringbuffer.h |  2 ++
 3 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index d00ac40d8401..d0096aba5af7 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -4,6 +4,7 @@
  */
 #include <linux/console.h>
 #include <linux/percpu.h>
+#include <linux/time_namespace.h>
 #include <linux/types.h>
 
 #if defined(CONFIG_PRINTK) && defined(CONFIG_SYSCTL)
@@ -354,11 +355,16 @@ struct log_state {
 	 */
 	struct latched_seq clear_seq;
 	struct printk_ringbuffer *prb;
+	/* Updated on every log_state request */
+	struct timens_offsets time_offset;
 };
 
+#define GET_VE_LOG(ve) ((struct log_state *) ve->log_state)
+
 extern struct log_state init_log_state;
 
 struct log_state *ve_log_state(void);
+struct log_state *__ve_log_state(struct ve_struct *ve);
 
 bool printk_get_next_message(struct log_state *log,
 			     struct printk_message *pmsg, u64 seq,
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index df7cf83fb4af..575bd3590198 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -556,13 +556,28 @@ DEFINE_STRUCT_MEMBER_ALIAS(clear_seq, init_log_state, clear_seq);
 DEFINE_STRUCT_MEMBER_ALIAS(prb, init_log_state, prb);
 #undef DEFINE_STRUCT_MEMBER_ALIAS
 
+static void update_log_time(struct ve_struct *ve)
+{
+	struct time_namespace *timens = ve_get_time_ns(ve);
+
+	if (timens)
+		GET_VE_LOG(ve)->time_offset = timens->offsets;
+	else
+		GET_VE_LOG(ve)->time_offset = init_time_ns.offsets;
+
+	if (timens)
+		put_time_ns(timens);
+}
+
 struct log_state *ve_log_state(void)
 {
 	struct log_state *log = &init_log_state;
 #ifdef CONFIG_VE
 	/* get_exec_env() is reliable only in task context */
-	if (!WARN_ON_ONCE(!in_task()) && get_exec_env()->log_state)
+	if (!WARN_ON_ONCE(!in_task()) && get_exec_env()->log_state) {
+		update_log_time(get_exec_env());
 		log = get_exec_env()->log_state;
+	}
 #endif
 	return log;
 }
@@ -571,8 +586,10 @@ struct log_state *__ve_log_state(struct ve_struct *ve)
 {
 	struct log_state *log = &init_log_state;
 #ifdef CONFIG_VE
-	if (ve->log_state)
+	if (ve->log_state) {
+		update_log_time(ve);
 		log = ve->log_state;
+	}
 #endif
 	return log;
 }
@@ -698,21 +715,10 @@ static void append_char(char **pp, char *e, char c)
 		*(*pp)++ = c;
 }
 
-static inline u64 ve_timens_add_boottime_ns(u64 nsec)
+static inline u64 ve_timens_add_boottime_ns(u64 nsec, const struct timens_offsets *off)
 {
-	struct time_namespace *ve_time_ns;
-	struct timens_offsets *ns_offsets;
-
-	ve_time_ns = ve_get_time_ns(get_exec_env());
-	if (unlikely(!ve_time_ns)) {
-		/* container not yet started */
-		return nsec;
-	}
-
-	ns_offsets = &ve_time_ns->offsets;
-	nsec += ns_offsets->boottime.tv_sec * NSEC_PER_SEC;
-	nsec += ns_offsets->boottime.tv_nsec;
-	put_time_ns(ve_time_ns);
+	nsec += off->boottime.tv_sec * NSEC_PER_SEC;
+	nsec += off->boottime.tv_nsec;
 
 	return nsec;
 }
@@ -732,7 +738,7 @@ static ssize_t info_print_ext_header(char *buf, size_t size,
 #endif
 
 	/* shift the timestamp on the Container uptime value */
-	ts_usec = ve_timens_add_boottime_ns(ts_usec);
+	ts_usec = ve_timens_add_boottime_ns(ts_usec, &info->time_offset);
 	do_div(ts_usec, 1000);
 
 	return scnprintf(buf, size, "%u,%llu,%llu,%c%s;",
@@ -1108,6 +1114,7 @@ void log_buf_vmcoreinfo_setup(void)
 	VMCOREINFO_OFFSET(printk_info, text_len);
 	VMCOREINFO_OFFSET(printk_info, caller_id);
 	VMCOREINFO_OFFSET(printk_info, dev_info);
+	VMCOREINFO_OFFSET(printk_info, time_offset);
 
 	VMCOREINFO_STRUCT_SIZE(dev_printk_info);
 	VMCOREINFO_OFFSET(dev_printk_info, subsystem);
@@ -1219,6 +1226,7 @@ static unsigned int __init add_to_rb(struct printk_ringbuffer *rb,
 	dest_r.info->ts_nsec = r->info->ts_nsec;
 	dest_r.info->caller_id = r->info->caller_id;
 	memcpy(&dest_r.info->dev_info, &r->info->dev_info, sizeof(dest_r.info->dev_info));
+	dest_r.info->time_offset = r->info->time_offset;
 
 	prb_final_commit(&e);
 
@@ -1427,12 +1435,13 @@ static size_t print_syslog(unsigned int level, char *buf)
 	return sprintf(buf, "<%u>", level);
 }
 
-static size_t print_time(u64 ts, char *buf)
+static size_t print_time(const struct printk_info *info, char *buf)
 {
 	unsigned long rem_nsec;
+	u64 ts;
 
 	/* shift the timestamp on the Container uptime value */
-	ts = ve_timens_add_boottime_ns(ts);
+	ts = ve_timens_add_boottime_ns(info->ts_nsec, &info->time_offset);
 
 	rem_nsec = do_div(ts, 1000000000);
 
@@ -1462,7 +1471,7 @@ static size_t info_print_prefix(const struct printk_info  *info, bool syslog,
 		len = print_syslog((info->facility << 3) | info->level, buf);
 
 	if (time)
-		len += print_time(info->ts_nsec, buf + len);
+		len += print_time(info, buf + len);
 
 	len += print_caller(info->caller_id, buf + len);
 
@@ -2402,6 +2411,8 @@ static int vprintk_store_log(struct log_state *log, int facility, int level,
 	if (dev_info)
 		memcpy(&r.info->dev_info, dev_info, sizeof(r.info->dev_info));
 
+	r.info->time_offset = log->time_offset;
+
 	/* A message without a trailing newline can be continued. */
 	if (!(flags & LOG_NEWLINE))
 		prb_commit(&e);
diff --git a/kernel/printk/printk_ringbuffer.h b/kernel/printk/printk_ringbuffer.h
index 4ef81349d9fb..19052ba005fc 100644
--- a/kernel/printk/printk_ringbuffer.h
+++ b/kernel/printk/printk_ringbuffer.h
@@ -7,6 +7,7 @@
 #include <linux/bits.h>
 #include <linux/dev_printk.h>
 #include <linux/stddef.h>
+#include <linux/time_namespace.h>
 #include <linux/types.h>
 
 /*
@@ -25,6 +26,7 @@ struct printk_info {
 	u32	caller_id;	/* thread id or processor id */
 
 	struct dev_printk_info	dev_info;
+	struct timens_offsets	time_offset;	/* Author timens offsets */
 };
 
 /*

-- 
2.52.0

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

Re: [Devel] [PATCH hci-8.1 4/6] ve/printk: Fix time virtualization

[no-reply]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] printk: ve_log_printk() path stamps records with zero or stale time_offset
- [Medium] printk: unsynchronized log_state->time_offset used to pass author offsets
- [Medium] printk: time_namespace get/put in update_log_time() on atomic printk paths
- [Low] printk: 32-byte timens_offsets embedded in every printk_info grows ringbuffers and vmcoreinfo
--

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

ve/printk: Fix time virtualization

This patch stores timens offsets in each printk_info record, so the
timestamp is shifted using the offsets of the context that emitted the
message. The offsets are copied into log_state on each log_state lookup
and then into the record in vprintk_store_log().

Link: https://virtuozzo.atlassian.net/browse/VSTOR-146839

> diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h

[ ... ]

> @@ -354,11 +355,16 @@ struct log_state {
>  	 */
>  	struct latched_seq clear_seq;
>  	struct printk_ringbuffer *prb;
> +	/* Updated on every log_state request */
> +	struct timens_offsets time_offset;
>  };

[Severity: Medium]
Can passing the author offsets through shared log_state storage produce
torn or stale values?

update_log_time() does a plain multi-word struct copy into
GET_VE_LOG(ve)->time_offset on every ve_log_state() or __ve_log_state()
call. That includes read-only callers such as devkmsg_poll(),
devkmsg_read() and do_syslog(), and they can run concurrently on other
CPUs or in IRQ context.

vprintk_store_log() later reads the field with another plain copy, after
vsnprintf and prb_reserve. Nothing orders the two accesses, since there
is no lock, no seqcount and no READ_ONCE()/WRITE_ONCE().

The offsets are normally the same for a given VE. They can differ across a
VE stop and start, which the commit message says can produce a new time
namespace. In that case a mixed or stale value could be stored in the
ring buffer, and KCSAN would likely flag the race.

Would it be possible to compute the offsets locally in the writer and
pass them down to vprintk_emit_log() by value or by pointer?

> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -556,13 +556,28 @@ DEFINE_STRUCT_MEMBER_ALIAS(clear_seq, init_log_state, clear_seq);
>  DEFINE_STRUCT_MEMBER_ALIAS(prb, init_log_state, prb);
>  #undef DEFINE_STRUCT_MEMBER_ALIAS
>  
> +static void update_log_time(struct ve_struct *ve)
> +{
> +	struct time_namespace *timens = ve_get_time_ns(ve);
> +
> +	if (timens)
> +		GET_VE_LOG(ve)->time_offset = timens->offsets;
> +	else
> +		GET_VE_LOG(ve)->time_offset = init_time_ns.offsets;
> +
> +	if (timens)
> +		put_time_ns(timens);
> +}

[Severity: Medium]
Can the get/put pair here run from atomic context?

__ve_vprintk_emit() calls __ve_log_state(ve), which has no in_task()
guard, so it can be reached from IRQ or other atomic context:

__ve_vprintk_emit()
  __ve_log_state()
    update_log_time()
      ve_get_time_ns()
      put_time_ns()

If VE teardown clears ve_nsproxy and drops its reference between the get
and the put, the put_time_ns() here would be the last one. free_time_ns()
would then run from atomic or scheduler-locked printk context. I could not
confirm that this race is reachable, so this part is speculative.

Is the reference needed at all? An rcu_read_lock() section that copies the
offsets looks like it would be enough. The old code only took the reference
when formatting output, whereas now every log_state lookup pays for the
refcount inc/dec and the struct copy, including the read-only syslog and
devkmsg paths.

>  struct log_state *ve_log_state(void)
>  {
>  	struct log_state *log = &init_log_state;
>  #ifdef CONFIG_VE
>  	/* get_exec_env() is reliable only in task context */
> -	if (!WARN_ON_ONCE(!in_task()) && get_exec_env()->log_state)
> +	if (!WARN_ON_ONCE(!in_task()) && get_exec_env()->log_state) {
> +		update_log_time(get_exec_env());
>  		log = get_exec_env()->log_state;
> +	}
>  #endif
>  	return log;
>  }

[ ... ]

> @@ -1108,6 +1114,7 @@ void log_buf_vmcoreinfo_setup(void)
>  	VMCOREINFO_OFFSET(printk_info, text_len);
>  	VMCOREINFO_OFFSET(printk_info, caller_id);
>  	VMCOREINFO_OFFSET(printk_info, dev_info);
> +	VMCOREINFO_OFFSET(printk_info, time_offset);

[Severity: Low]
Is the size of the new field intended? struct printk_info now embeds a
32-byte struct timens_offsets (two timespec64) in every descriptor, but
only the boottime offset is used.

That grows the static host __log_buf infos array and every per-VE
ringbuffer infos array. SIZE(printk_info) also changes and a new
OFFSET(printk_info.time_offset) is exported, so dump tools such as crash
and makedumpfile would need updating.

Would a per-log value, or a 16-byte boottime delta, be enough? The commit
message does not mention the size or vmcoreinfo change.

[ ... ]

> @@ -2402,6 +2411,8 @@ static int vprintk_store_log(struct log_state *log, int facility, int level,
>  	if (dev_info)
>  		memcpy(&r.info->dev_info, dev_info, sizeof(r.info->dev_info));
>  
> +	r.info->time_offset = log->time_offset;
> +

[Severity: Medium]
Can this store a zero or stale time_offset for records coming from
ve_log_printk()?

ve_log_printk() calls ve_log_vprintk(), which for a non-super VE calls
vprintk_emit_log() directly:

r = vprintk_emit_log(ve->log_state, 0, LOGLEVEL_DEFAULT, NULL,
		     fmt, args);

That path never goes through ve_log_state() or __ve_log_state(), which
are the only callers of update_log_time(). vprintk_store_log() then
copies whatever is in log->time_offset.

The field is zero from the kzalloc in ve_log_init() unless some other
lookup has updated it. The record would then be stored with an unshifted
host uptime, which is the leak this patch is meant to prevent.

Should the offsets be refreshed on this path as well?

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

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

Re: [Devel] [PATCH hci-8.1 4/6] ve/printk: Fix time virtualization

[Vladimir Riabchun]

On 10/2/26 23:37, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [Medium] printk: ve_log_printk() path stamps records with zero or stale time_offset
> - [Medium] printk: unsynchronized log_state->time_offset used to pass author offsets
> - [Medium] printk: time_namespace get/put in update_log_time() on atomic printk paths
> - [Low] printk: 32-byte timens_offsets embedded in every printk_info grows ringbuffers and vmcoreinfo
> --

All of these are true. Two biggest issues seem to be printk_info
growth and synchronization troubles with log_state->time_offset.
The patch will be reworked in v2.

> 
> commit eea8f8e8aad7cad7bcb35ee878f7d51794c27e83
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve/printk: Fix time virtualization
> 
> ...
> 

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

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

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

[Vladimir Riabchun]

ve_failcount_inc could be called from IRQ context, and we want the message
to go to the right place. Use new  __ve_printk helpers for this.

https://virtuozzo.atlassian.net/browse/VSTOR-146839

Feature: ve printk
Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
---
 include/linux/printk.h | 18 ++++++++++++++----
 include/linux/ve.h     | 16 ++++++++--------
 2 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 1478e47dc582..ce82623fb138 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -755,21 +755,28 @@ struct pi_entry {
 		printk(fmt, ##__VA_ARGS__);				\
 })
 
-#define ve_printk_ratelimited(dst, fmt, ...)				\
+#define __ve_printk_ratelimited(dst, ve, fmt, ...)			\
 ({									\
 	static DEFINE_RATELIMIT_STATE(_rs,				\
 				      DEFAULT_RATELIMIT_INTERVAL,	\
 				      DEFAULT_RATELIMIT_BURST);		\
 									\
-	WARN_ON_ONCE((dst & VE_LOG) && !in_task());			\
-	if (ve_ratelimit(dst, get_exec_env(), &_rs))			\
-		ve_printk(dst, fmt, ##__VA_ARGS__);			\
+	if (ve_ratelimit(dst, ve, &_rs))				\
+		__ve_printk(dst, ve, fmt, ##__VA_ARGS__);		\
+})
+
+#define ve_printk_ratelimited(dst, fmt, ...)					\
+({										\
+	WARN_ON_ONCE((dst & VE_LOG) && !in_task());				\
+	__ve_printk_ratelimited(dst, get_exec_env(), fmt, ##__VA_ARGS__);	\
 })
 #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
 
 #define pr_emerg_ratelimited(fmt, ...)					\
@@ -787,6 +794,7 @@ struct pi_entry {
 #define pr_info_ratelimited(fmt, ...)					\
 	printk_ratelimited(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
 /* no pr_cont_ratelimited, don't do that... */
+
 #define ve_pr_emerg_ratelimited(dst, fmt, ...)					\
 	ve_printk_ratelimited(dst, KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
 #define ve_pr_alert_ratelimited(dst, fmt, ...)					\
@@ -797,6 +805,8 @@ struct pi_entry {
 	ve_printk_ratelimited(dst, KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
 #define ve_pr_warn_ratelimited(dst, fmt, ...)					\
 	ve_printk_ratelimited(dst, KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
+#define __ve_pr_warn_ratelimited(dst, ve, fmt, ...)				\
+	__ve_printk_ratelimited(dst, ve, KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
 #define ve_pr_notice_ratelimited(dst, fmt, ...)					\
 	ve_printk_ratelimited(dst, KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
 #define ve_pr_info_ratelimited(dst, fmt, ...)					\
diff --git a/include/linux/ve.h b/include/linux/ve.h
index a400d21593b2..946df4eebe89 100644
--- a/include/linux/ve.h
+++ b/include/linux/ve.h
@@ -156,14 +156,14 @@ extern int nr_ve;
 #define BPF_PROG_MAX_NR_DEFAULT	256	/* number of loaded BPF progs per-VE */
 #define MNT_MAX_NR_DEFAULT	4096	/* number of mounts per-VE */
 
-#define ve_failcount_inc(ve, name)					\
-do {									\
-	struct ve_struct *__ve = (ve);					\
-									\
-	atomic_inc(&__ve->name##_failcount);				\
-	ve_pr_warn_ratelimited(VE_LOG_BOTH, "CT%s: hits the " #name	\
-		" limit, see the '" #name "' counter in ve.failcount "	\
-		"of the container's ve cgroup\n", ve_name(__ve));	\
+#define ve_failcount_inc(ve, name)						\
+do {										\
+	struct ve_struct *__ve = (ve);						\
+										\
+	atomic_inc(&__ve->name##_failcount);					\
+	__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));		\
 } while (0)
 
 #ifdef CONFIG_VE

-- 
2.52.0

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

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

[no-reply]

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

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

[Vladimir Riabchun]

On 10/2/26 23:38, no-reply@sashiko.virtuozzo.com wrote:
> 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, ...).

No, it is not. Will fix in v2.

> 
> [ ... ]
> 
>> +#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.
> 

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

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

[Devel] [PATCH hci-8.1 6/6] ve: add neigh_tbl failcounter

[Vladimir Riabchun]

Count hits of the per-VE neigh_tbl limit in ve.failcount.

This one was delayed due to a high risk of IRQ-context hits,
when destination VE can't be obtained from get_exec_env.

https://virtuozzo.atlassian.net/browse/VSTOR-135520
https://virtuozzo.atlassian.net/browse/VSTOR-146839

Feature: per-ve failcounters
Feature: ve printk
Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
---
 include/linux/ve.h   | 1 +
 kernel/ve/ve.c       | 3 +++
 net/core/neighbour.c | 3 +--
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/ve.h b/include/linux/ve.h
index 946df4eebe89..44e04dc75823 100644
--- a/include/linux/ve.h
+++ b/include/linux/ve.h
@@ -97,6 +97,7 @@ struct ve_struct {
 
 	atomic_t		arp_neigh_nr;
 	atomic_t		nd_neigh_nr;
+	atomic_t		neigh_tbl_failcount;
 	unsigned long		meminfo_val;
 
 	/*
diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
index 51cc635be2e2..35fad9658149 100644
--- a/kernel/ve/ve.c
+++ b/kernel/ve/ve.c
@@ -118,6 +118,7 @@ struct ve_struct ve0 = {
 
 	.arp_neigh_nr		= ATOMIC_INIT(0),
 	.nd_neigh_nr		= ATOMIC_INIT(0),
+	.neigh_tbl_failcount	= ATOMIC_INIT(0),
 	.mnt_avail_nr		= ATOMIC_INIT(INT_MAX),
 	.mnt_max_nr		= INT_MAX,
 	.mnt_failcount		= ATOMIC_INIT(0),
@@ -834,6 +835,7 @@ static struct cgroup_subsys_state *ve_create(struct cgroup_subsys_state *parent_
 
 	atomic_set(&ve->arp_neigh_nr, 0);
 	atomic_set(&ve->nd_neigh_nr, 0);
+	atomic_set(&ve->neigh_tbl_failcount, 0);
 	ve->mnt_max_nr = MNT_MAX_NR_DEFAULT;
 	atomic_set(&ve->mnt_failcount, 0);
 	atomic_set(&ve->mnt_avail_nr, MNT_MAX_NR_DEFAULT);
@@ -1098,6 +1100,7 @@ static const struct ve_failcount_entry {
 	VE_FC_ENTRY(mnt),
 	VE_FC_ENTRY(netif),
 	VE_FC_ENTRY(bpf_prog),
+	VE_FC_ENTRY(neigh_tbl),
 #ifdef CONFIG_AIO
 	VE_FC_ENTRY(aio),
 #endif
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index f90deb17fb25..c7d5e6df50c2 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -520,8 +520,7 @@ static struct neighbour *neigh_alloc(struct neigh_table *tbl,
 	    (glob_entries >= READ_ONCE(tbl->gc_thresh2) &&
 	     time_after(now, READ_ONCE(tbl->last_flush) + 5 * HZ))) {
 		if (!neigh_forced_gc(tbl, ve) && entries >= gc_thresh3) {
-			net_info_ratelimited("%s: neighbor table overflow!\n",
-					     tbl->id);
+			ve_failcount_inc(ve, neigh_tbl);
 			NEIGH_CACHE_STAT_INC(tbl, table_fulls);
 			goto out_entries;
 		}

-- 
2.52.0

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

Re: [Devel] [PATCH hci-8.1 6/6] ve: add neigh_tbl failcounter

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: neigh_alloc() drops table-specific overflow log; ARP and ND share one failcount
--

commit 83435d15f32b1a7fcb7f3aac09fb30774f63d147
Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

ve: add neigh_tbl failcounter

This commit adds a per-VE neigh_tbl_failcount and exposes it in
ve.failcount. In neigh_alloc(), the "neighbor table overflow!" message
is replaced by a call to ve_failcount_inc() when the table limit is hit.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-135520
Link: https://virtuozzo.atlassian.net/browse/VSTOR-146839

> diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
> --- a/kernel/ve/ve.c
> +++ b/kernel/ve/ve.c

[ ... ]

> @@ -834,6 +835,7 @@ static struct cgroup_subsys_state *ve_create(struct cgroup_subsys_state *parent_
>  
>  	atomic_set(&ve->arp_neigh_nr, 0);
>  	atomic_set(&ve->nd_neigh_nr, 0);
> +	atomic_set(&ve->neigh_tbl_failcount, 0);

[Severity: Medium]
ARP and ND have separate per-VE counters and limits (arp_neigh_nr and
nd_neigh_nr, selected by get_perve_tbl_entries_counter()), but both now
feed the single neigh_tbl_failcount.

Can the ve.failcount output tell an operator whether the ARP table or the
ND table hit its limit?

[ ... ]

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c

[ ... ]

> @@ -520,8 +520,7 @@ static struct neighbour *neigh_alloc(struct neigh_table *tbl,
>  	    (glob_entries >= READ_ONCE(tbl->gc_thresh2) &&
>  	     time_after(now, READ_ONCE(tbl->last_flush) + 5 * HZ))) {
>  		if (!neigh_forced_gc(tbl, ve) && entries >= gc_thresh3) {
> -			net_info_ratelimited("%s: neighbor table overflow!\n",
> -					     tbl->id);
> +			ve_failcount_inc(ve, neigh_tbl);
>  			NEIGH_CACHE_STAT_INC(tbl, table_fulls);
>  			goto out_entries;
>  		}

[Severity: Medium]
The removed net_info_ratelimited() message named the table through
tbl->id. The replacement message from ve_failcount_inc() is fixed text
and has no table or family information:

"CT<name>: hits the neigh_tbl limit, see the 'neigh_tbl' counter in
ve.failcount of the container's ve cgroup"

Is it intended to drop the arp_tbl versus nd_tbl distinction and the
host-level overflow message? The commit message does not mention that the
original message is being removed.

Also, this path runs for ve0 as well. Would the host then log "CT0: hits
the neigh_tbl limit ... of the container's ve cgroup", which seems
misleading wording for the host?

Finally, the ratelimit state in ve_failcount_inc() comes from
__ve_pr_warn_ratelimited(), which is a single static state at the macro
call site. Is it a concern that all VEs share it, so one container that
overflows its table often could suppress the message for other containers?

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

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

Re: [Devel] [PATCH hci-8.1 6/6] ve: add neigh_tbl failcounter

[Vladimir Riabchun]

On 10/2/26 23:40, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] net: neigh_alloc() drops table-specific overflow log; ARP and ND share one failcount
> --
> 
> commit 83435d15f32b1a7fcb7f3aac09fb30774f63d147
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve: add neigh_tbl failcounter
> 
> This commit adds a per-VE neigh_tbl_failcount and exposes it in
> ve.failcount. In neigh_alloc(), the "neighbor table overflow!" message
> is replaced by a call to ve_failcount_inc() when the table limit is hit.
> 
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-135520
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-146839
> 
>> diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
>> --- a/kernel/ve/ve.c
>> +++ b/kernel/ve/ve.c
> 
> [ ... ]
> 
>> @@ -834,6 +835,7 @@ static struct cgroup_subsys_state *ve_create(struct cgroup_subsys_state *parent_
>>   
>>   	atomic_set(&ve->arp_neigh_nr, 0);
>>   	atomic_set(&ve->nd_neigh_nr, 0);
>> +	atomic_set(&ve->neigh_tbl_failcount, 0);
> 
> [Severity: Medium]
> ARP and ND have separate per-VE counters and limits (arp_neigh_nr and
> nd_neigh_nr, selected by get_perve_tbl_entries_counter()), but both now
> feed the single neigh_tbl_failcount.

This is true, but IMO it won't add much value.
I'll return net_info_ratelimited message with table id and ve name
for debugging purposes. It will go to the host only.

> 
> Can the ve.failcount output tell an operator whether the ARP table or the
> ND table hit its limit?
> 
> [ ... ]
> 
>> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
>> --- a/net/core/neighbour.c
>> +++ b/net/core/neighbour.c
> 
> [ ... ]
> 
>> @@ -520,8 +520,7 @@ static struct neighbour *neigh_alloc(struct neigh_table *tbl,
>>   	    (glob_entries >= READ_ONCE(tbl->gc_thresh2) &&
>>   	     time_after(now, READ_ONCE(tbl->last_flush) + 5 * HZ))) {
>>   		if (!neigh_forced_gc(tbl, ve) && entries >= gc_thresh3) {
>> -			net_info_ratelimited("%s: neighbor table overflow!\n",
>> -					     tbl->id);
>> +			ve_failcount_inc(ve, neigh_tbl);
>>   			NEIGH_CACHE_STAT_INC(tbl, table_fulls);
>>   			goto out_entries;
>>   		}
> 
> [Severity: Medium]
> The removed net_info_ratelimited() message named the table through
> tbl->id. The replacement message from ve_failcount_inc() is fixed text
> and has no table or family information:
> 
> "CT<name>: hits the neigh_tbl limit, see the 'neigh_tbl' counter in
> ve.failcount of the container's ve cgroup"
> 
> Is it intended to drop the arp_tbl versus nd_tbl distinction and the
> host-level overflow message? The commit message does not mention that the
> original message is being removed.
> 
> Also, this path runs for ve0 as well. Would the host then log "CT0: hits
> the neigh_tbl limit ... of the container's ve cgroup", which seems
> misleading wording for the host?
> 
> Finally, the ratelimit state in ve_failcount_inc() comes from
> __ve_pr_warn_ratelimited(), which is a single static state at the macro
> call site. Is it a concern that all VEs share it, so one container that
> overflows its table often could suppress the message for other containers?
> 

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

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