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

[Devel] [PATCH hci-8.1 v2 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>
---
Changes in v2:
- Patch 2: fixed uninitialized r value in ve_dev_printk_emit.
- Patch 3: fixed incorrect __printf annotation for ve_printk_deferred.
- Patch 3: now on IRQ case ve_printk_ratelimited falls back to ve0.
- Patch 3: removed forgotten get_exec_env() usages from ve_printk
  functions.
- Patch 4: completely reworked and simplified.
- Patch 5: fix __ve_printk_ratelimited stub.
- Patch 6: brought back table type logging to host.
- Link to v1: http://lore.virtuozzo.com/20261002-ve-printk-irq-v1-0-cb55896d455c@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                 |   8 ++-
 include/linux/net.h                 |  16 +++---
 include/linux/printk.h              |  42 ++++++++++++---
 include/linux/ratelimit_types.h     |   6 ++-
 include/linux/ve.h                  |  17 +++---
 kernel/printk/internal.h            |   4 ++
 kernel/printk/printk.c              | 101 ++++++++++++++++++++++++++++--------
 kernel/ve/ve.c                      |   3 ++
 lib/ratelimit.c                     |   7 +--
 net/core/neighbour.c                |   5 +-
 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 +-
 14 files changed, 162 insertions(+), 59 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 v2 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

[Devel] [PATCH hci-8.1 v2 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    |  6 ++++--
 kernel/printk/printk.c | 11 ++++++++---
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2d233ffb5661..ba50bdbfb648 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4943,7 +4943,7 @@ static int ve_dev_vprintk_emit(int level, const struct device *dev, const char *
 int ve_dev_printk_emit(int dst, int level, const struct device *dev, const char *fmt, ...)
 {
 	va_list args, args2;
-	int r;
+	int r = 0;
 
 	WARN_ON_ONCE(!dst || (dst & ~VE_LOG_BOTH));
 
@@ -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

[Devel] [PATCH hci-8.1 v2 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              | 28 ++++++++++---
 include/linux/ratelimit_types.h     |  6 ++-
 kernel/printk/printk.c              | 78 ++++++++++++++++++++++++++++++-------
 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, 110 insertions(+), 37 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..a9470c81f8db 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(2, 3) __cold
-int ve_printk_deferred(int dst, const char *s, ...)
+static inline __printf(3, 4) __cold
+int __ve_printk(int dst, struct ve_struct *ve, const char *s, ...)
+{
+	return 0;
+}
+static inline __printf(3, 4) __cold
+int ve_printk_deferred(int dst, struct ve_struct *ve, const char *s, ...)
 {
 	return 0;
 }
@@ -746,8 +760,12 @@ struct pi_entry {
 	static DEFINE_RATELIMIT_STATE(_rs,				\
 				      DEFAULT_RATELIMIT_INTERVAL,	\
 				      DEFAULT_RATELIMIT_BURST);		\
+	struct ve_struct *__ve = get_exec_env();			\
+									\
+	if (WARN_ON_ONCE((dst & VE_LOG) && !in_task()))			\
+		__ve = get_ve0();					\
 									\
-	if (ve_ratelimit(dst, &_rs))					\
+	if (ve_ratelimit(dst, __ve, &_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..d5680fda32ca 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))
+	if (ve_is_super(ve) || (dst & VE0_LOG))
 		r = vprintk(fmt, args);
 
 	/* Report IRQ usage here - printing to VE requires explicit ve reference */
-	if ((dst & VE_LOG) && !WARN_ON_ONCE(!in_task()) && !ve_is_super(get_exec_env()))
-		r = ve_vprintk_emit(0, LOGLEVEL_DEFAULT, NULL, fmt, args2);
+	if ((dst & VE_LOG)) {
+		if (!ve_provided && WARN_ON_ONCE(!in_task()))
+			goto done;
+		if (!ve_is_super(ve))
+			r = __ve_vprintk_emit(ve, 0, LOGLEVEL_DEFAULT, NULL, fmt, args2);
+	}
+done:
 	va_end(args2);
 
 	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))
+	if (ve_is_super(ve) || (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

[Devel] [PATCH hci-8.1 v2 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.

Do time adjustments on message submission in vprintk_store_log,
there we know the final destination and VE, so we can get actual
time namespace and offsets there.

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

Feature: ve printk
Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
---
 kernel/printk/internal.h |  4 ++++
 kernel/printk/printk.c   | 12 +++++-------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index d00ac40d8401..d678718482ed 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -354,11 +354,15 @@ struct log_state {
 	 */
 	struct latched_seq clear_seq;
 	struct printk_ringbuffer *prb;
+	struct ve_struct *owner_ve;
 };
 
+#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 d5680fda32ca..cf4baacd22d8 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -534,6 +534,7 @@ struct log_state init_log_state = {
 		.val[1]	= 0,
 	},
 	.prb = &printk_rb_static,
+	.owner_ve = &ve0,
 };
 
 /* kdump relies on some log_* symbols, let's make it happy */
@@ -698,12 +699,12 @@ 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, struct ve_struct *ve)
 {
 	struct time_namespace *ve_time_ns;
 	struct timens_offsets *ns_offsets;
 
-	ve_time_ns = ve_get_time_ns(get_exec_env());
+	ve_time_ns = ve_get_time_ns(ve);
 	if (unlikely(!ve_time_ns)) {
 		/* container not yet started */
 		return nsec;
@@ -731,8 +732,6 @@ static ssize_t info_print_ext_header(char *buf, size_t size,
 	caller[0] = '\0';
 #endif
 
-	/* shift the timestamp on the Container uptime value */
-	ts_usec = ve_timens_add_boottime_ns(ts_usec);
 	do_div(ts_usec, 1000);
 
 	return scnprintf(buf, size, "%u,%llu,%llu,%c%s;",
@@ -1431,9 +1430,6 @@ static size_t print_time(u64 ts, char *buf)
 {
 	unsigned long rem_nsec;
 
-	/* shift the timestamp on the Container uptime value */
-	ts = ve_timens_add_boottime_ns(ts);
-
 	rem_nsec = do_div(ts, 1000000000);
 
 	return sprintf(buf, "[%5lu.%06lu]",
@@ -2329,6 +2325,7 @@ static int vprintk_store_log(struct log_state *log, int facility, int level,
 	 * timestamp with respect to the caller.
 	 */
 	ts_nsec = local_clock();
+	ts_nsec = ve_timens_add_boottime_ns(ts_nsec, log->owner_ve);
 
 	caller_id = printk_caller_id();
 
@@ -5294,6 +5291,7 @@ int ve_log_init(struct ve_struct *ve)
 	seqcount_latch_init(&log->clear_seq.latch);
 
 	ve->log_state = log;
+	log->owner_ve = ve;
 	return 0;
 }
 

-- 
2.52.0

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

[Devel] [PATCH hci-8.1 v2 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 a9470c81f8db..620a6f767150 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -755,24 +755,31 @@ 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);		\
+									\
+	if (ve_ratelimit(dst, ve, &_rs))				\
+		__ve_printk(dst, ve, fmt, ##__VA_ARGS__);		\
+})
+
+#define ve_printk_ratelimited(dst, fmt, ...)				\
+({									\
 	struct ve_struct *__ve = get_exec_env();			\
 									\
 	if (WARN_ON_ONCE((dst & VE_LOG) && !in_task()))			\
 		__ve = get_ve0();					\
-									\
-	if (ve_ratelimit(dst, __ve, &_rs))				\
-		ve_printk(dst, fmt, ##__VA_ARGS__);			\
+	__ve_printk_ratelimited(dst, __ve, 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, ve, fmt, ...)			\
+	no_printk(fmt, ##__VA_ARGS__)
 #endif
 
 #define pr_emerg_ratelimited(fmt, ...)					\
@@ -790,6 +797,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, ...)					\
@@ -800,6 +808,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

[Devel] [PATCH hci-8.1 v2 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 | 5 +++--
 3 files changed, 7 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..6c2fc778a8ab 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -520,8 +520,9 @@ 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);
+			net_info_ratelimited("%s: neighbor table overflow for ve %s!\n",
+					     tbl->id, ve_name(ve));
+			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 v2 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: ve_vprintk_deferred() raises non-deferred WARN_ON_ONCE() on invalid dst
--

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

ve/printk: Add sanity checks in ve_printk functions

This commit adds a WARN_ON_ONCE() check on the dst argument in
ve_dev_printk_emit(), ve_vprintk() and ve_vprintk_deferred(). It also
reorders the VE_LOG test in the second if statement of each function.

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));
> +

[Severity: Low]
Can this WARN_ON_ONCE() in ve_vprintk_deferred() cause problems in
printk-unsafe contexts?

ve_vprintk_deferred() emits through vprintk_emit() at LOGLEVEL_SCHED
so that it can be used where plain printk is not safe. The warning
goes through the regular __warn() path, which prints via the normal,
non-deferred printk.

If a caller passes dst == 0 or a value with bits outside VE_LOG_BOTH
from such a context, the warning could recurse into or deadlock on
console or logbuf locks:

ve_ratelimit() (lib/ratelimit.c)
  ve_printk_deferred(dst, ve, KERN_WARNING ...)
    ve_vprintk_deferred(dst, ...)
      WARN_ON_ONCE(!dst || (dst & ~VE_LOG_BOTH));

I did not verify that any current caller passes an invalid dst, so this
is only a possible problem. The warning fires at most once.

Would it make sense to drop the check from the deferred helper or use a
deferred-safe way to report a bad dst?

>  	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);

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

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

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

[no-reply]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] printk: WARN_ON_ONCE() inside ve_vprintk_deferred() defeats deferred printk
- [Low] printk: non-task VE_LOG messages are dropped while host routing uses get_exec_env()
- [Low] printk: WARN_ON_ONCE(!in_task()) runs before the ve_is_super() check
--

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

ve/printk: Report potentially leaking usage of ve_printk functions

This commit adds WARN_ON_ONCE(!in_task()) checks to ve_log_state(),
ve_vprintk(), ve_vprintk_deferred() and ve_dev_printk_emit(). When a
VE_LOG message is requested from non-task context, the message is
rejected (or the host log state is returned) instead of relying on
get_exec_env().

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]
Is it intended that the WARN_ON_ONCE(!in_task()) is evaluated before the
ve_is_super() check?

In ve_dev_printk_emit(), any non-task caller that passes dst with VE_LOG
will hit the WARN even when the interrupted task is in ve0, where nothing
could leak to a container. Could the ve_is_super() test be placed first so
the WARN only fires when a container could actually be affected?

The same ordering appears in ve_vprintk() and ve_vprintk_deferred().

Also, the host branch just above still uses get_exec_env():

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

In non-task context with dst == VE_LOG, if a host task was interrupted the
message goes to the host log. If a container task was interrupted, the VE
branch is rejected by the WARN guard, the message is dropped, and r stays
0. Doesn't this make the outcome depend on which task was interrupted,
which is the get_exec_env() unreliability the commit message wants to
avoid? ve_log_state() by contrast falls back to the host log. The same
applies to ve_vprintk() and ve_vprintk_deferred().

> 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 run while printk-side processing already holds locks?

ve_log_state() is called from ve_vprintk_emit(), so a WARN here can fire in
the middle of a printk call. The WARN prints a banner and backtrace through
the normal printk path.

ve_vprintk_emit()
  ve_log_state()
    WARN_ON_ONCE(!in_task())
      __warn() -> printk

Could this recurse or deadlock if the caller is in scheduler, timer or
other non-task context holding such locks? The WARN fires only once, and I
did not find an in-tree caller that triggers this. In the final series
state, the unconditional WARN in ve_log_state() on this path is still
present.

[ ... ]

> @@ -2540,7 +2541,9 @@ static asmlinkage int ve_vprintk(int dst, const char *fmt, va_list 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);

[Severity: Medium]
Does this make any IRQ or NMI caller of ve_printk() that passes VE_LOG
trigger a WARN? See the comments on ve_log_state() above and on
ve_vprintk_deferred() below regarding emitting a WARN from such contexts.

> @@ -2574,7 +2577,9 @@ static asmlinkage int ve_vprintk_deferred(int dst, const char *fmt, va_list 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);

[Severity: Medium]
Does the synchronous WARN_ON_ONCE() inside ve_vprintk_deferred() defeat the
purpose of the deferred variant?

This helper is meant to be safe from scheduler and IRQ contexts. If a
caller outside task context (hardirq, softirq, NMI or scheduler code) calls
it with dst containing VE_LOG and no explicit ve, the WARN goes through
__warn() and the normal printk path. That path prints a banner and
backtrace and may flush consoles or wake klogd.

Could this self-deadlock or recurse when the caller holds scheduler, timer
or logbuf locks? The WARN fires only once. The only in-tree caller I found,
__ve_ratelimit() in lib/ratelimit.c, runs under rs->lock and passes ve, so
I have no concrete deadlocking caller. In the final series state,
ve_vprintk_deferred() still has the check
"if (!ve_provided && WARN_ON_ONCE(!in_task()))", so the concern remains
when no ve is provided.

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

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

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

[no-reply]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] printk: explicit-VE path skips the in_task() guard, so VE log wakeups can run in softirq
- [Low] printk: stale IRQ comment, trailing whitespace and duplicate struct ve_struct decl
--

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

ve/printk: Specify ve in printks

This patch adds ve_printk helpers that take an explicit VE pointer. The
ratelimit helpers and the net_ve*_ratelimited macros now require a VE
argument. The conntrack and TCPMSS callers pass net->owner_ve.

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

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

[ ... ]

> @@ -76,6 +76,7 @@ extern int console_printk[];
>  #define VE0_LOG		1
>  #define VE_LOG		2
>  #define VE_LOG_BOTH	(VE0_LOG | VE_LOG)
> +struct ve_struct;
>  
>  extern void console_verbose(void);

[Severity: Low]
This isn't a bug, but this adds a second forward declaration of struct
ve_struct. printk.h already has one further down, just before
ve_log_init().

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

[ ... ]

> -static asmlinkage int ve_vprintk(int dst, const char *fmt, va_list args)
> +static asmlinkage int ve_vprintk(int dst, struct ve_struct *ve,
> +				const char *fmt, va_list args)
>  {
>  	va_list args2;
> -	int r = 0;
> +	int r = 0, ve_provided = (ve != NULL);
>  
>  	WARN_ON_ONCE(!dst || (dst & ~VE_LOG_BOTH));
>  
> +	if (!ve)
> +		ve = get_exec_env();
> +
>  	va_copy(args2, args);
> -	if (ve_is_super(ve) || (dst & VE0_LOG))
> +	if (ve_is_super(ve) || (dst & VE0_LOG))
>  		r = vprintk(fmt, args);
>  
>  	/* Report IRQ usage here - printing to VE requires explicit ve reference */
> -	if ((dst & VE_LOG) && !WARN_ON_ONCE(!in_task()) && !ve_is_super(get_exec_env()))
> -		r = ve_vprintk_emit(0, LOGLEVEL_DEFAULT, NULL, fmt, args2);
> +	if ((dst & VE_LOG)) {
> +		if (!ve_provided && WARN_ON_ONCE(!in_task()))
> +			goto done;
> +		if (!ve_is_super(ve))
> +			r = __ve_vprintk_emit(ve, 0, LOGLEVEL_DEFAULT, NULL, fmt, args2);
> +	}
> +done:
>  	va_end(args2);

[Severity: Medium]
Before this patch, the VE log branch was skipped whenever in_task() was
false. With an explicit VE the in_task() guard is now bypassed.

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

__nf_conntrack_alloc() runs on the packet path. __nf_ct_expect_check()
runs under spin_lock_bh(). Both call net_veboth_ratelimited(net->owner_ve,
...), which expands to the non-deferred __ve_printk():

__nf_conntrack_alloc()
  net_veboth_ratelimited(net->owner_ve, ...)
    __ve_printk()
      ve_vprintk()            /* ve_provided is true */
        __ve_vprintk_emit(ve, 0, LOGLEVEL_DEFAULT, ...)
          vprintk_emit_log(__ve_log_state(ve), ...)
            wake_up_interruptible(&log->wait)

LOGLEVEL_DEFAULT does not clear ft.legacy_direct, so a non-init log goes
through the wake_up_interruptible() path in vprintk_emit_log().

The waitqueue lock is a spinlock_t. That is fine in softirq on non-RT
kernels. Is it also intended to be reachable from these contexts on
PREEMPT_RT, and from any future explicit-VE caller running in hardirq or
under a raw_spinlock_t?

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

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

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

[ ... ]

> +static asmlinkage int ve_vprintk_deferred(int dst, struct ve_struct *ve, 
> +					const char *fmt, va_list args)

[Severity: Low]
This isn't a bug, but this line adds trailing whitespace after
"struct ve_struct *ve, ".

[ ... ]

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

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

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

[no-reply]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] printk: time_ns refcount get/put in any-context vprintk_store_log() path
- [Low] printk: boottime shift baked in at store time leaves early records unshifted
- [Low] printk: init_log_state references ve0 without a CONFIG_VE guard
--

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

ve/printk: Fix time virtualization

This patch moves the container boottime shift from print time (where
get_exec_env() was used) to message store time in vprintk_store_log().
The shift now uses the owning VE of the log_state, so IRQ printks no
longer pick up the wrong container's offsets.

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

[ ... ]

> +#define GET_VE_LOG(ve) ((struct log_state *) ve->log_state)
> +

[Severity: Low]
This isn't a bug, but GET_VE_LOG() is not used anywhere in kernel/printk,
and it does not parenthesize its argument, so GET_VE_LOG(a + b) would
expand to a + b->log_state. Is the macro needed in this patch?

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

[ ... ]

> @@ -534,6 +534,7 @@ struct log_state init_log_state = {
>  		.val[1]	= 0,
>  	},
>  	.prb = &printk_rb_static,
> +	.owner_ve = &ve0,
>  };

[Severity: Low]
Will this build when CONFIG_PRINTK=y and CONFIG_VE=n? The declaration
extern struct ve_struct ve0; in include/linux/ve_proto.h is inside
#ifdef CONFIG_VE, and the !CONFIG_VE branch only provides the get_ve0()
stub.

Other VE-specific code in this file is guarded by #ifdef CONFIG_VE, so
should the .owner_ve initializer be guarded in the same way?

[ ... ]

> -static inline u64 ve_timens_add_boottime_ns(u64 nsec)
> +static inline u64 ve_timens_add_boottime_ns(u64 nsec, struct ve_struct *ve)
>  {
>  	struct time_namespace *ve_time_ns;
>  	struct timens_offsets *ns_offsets;
>  
> -	ve_time_ns = ve_get_time_ns(get_exec_env());
> +	ve_time_ns = ve_get_time_ns(ve);
>  	if (unlikely(!ve_time_ns)) {
>  		/* container not yet started */
>  		return nsec;

[ ... ]

> @@ -2329,6 +2325,7 @@ static int vprintk_store_log(struct log_state *log, int facility, int level,
>  	 * timestamp with respect to the caller.
>  	 */
>  	ts_nsec = local_clock();
> +	ts_nsec = ve_timens_add_boottime_ns(ts_nsec, log->owner_ve);

[Severity: Medium]
Can this take and drop a time_ns reference from a context where the final
put is unsafe?

vprintk_store_log() runs in the any-context store path, after
printk_enter_irqsave(). It now calls:

vprintk_store_log()
  ve_timens_add_boottime_ns()
    ve_get_time_ns()     /* get_time_ns() under rcu_read_lock() */
    put_time_ns()

ve_get_time_ns() holds the reference past the RCU section. Consider the
teardown side in ve_drop_context():

rcu_assign_pointer(ve->ve_nsproxy, NULL);
synchronize_rcu();
put_nsproxy(ve_nsproxy);

If a printk took its reference before the pointer was cleared, can it end
up holding the last time_ns reference? In that case put_time_ns() would
call free_time_ns() from inside the printk store path, which may be in
hardirq or other atomic context.

Separately, this adds an RCU lock plus an atomic inc/dec on every printk
record, including host (ve0) records, where it touches init_time_ns.

Would it be possible to read the offsets under RCU without taking a
reference, cache the boottime offset in log_state, or skip ve0?

[Severity: Low]
Is it intended that records stored while ve->ve_nsproxy is NULL keep the
raw host uptime permanently?

ve_log_init() runs at css_alloc, but ve_nsproxy is only set later by
ve_grab_context(). Until then ve_get_time_ns() returns NULL and
ve_timens_add_boottime_ns() returns the value unshifted:

	if (unlikely(!ve_time_ns)) {
		/* container not yet started */
		return nsec;
	}

Before this patch the shift was applied at read time, so every record
was shifted consistently once the container had started. Now the
container's dmesg can show a mixed timeline, and the raw ts_nsec in the
VE ring buffer differs from what was read before. The commit message does
not mention this change in behavior.

[ ... ]

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

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

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

[no-reply]

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

Sashiko has reviewed this patch and found no issues. It looks great!

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


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

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

[no-reply]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] net: neigh_alloc() logs one overflow twice and rewords the old message
- [Low] net: neigh_tbl failcount is charged to the VE for global-limit overflows
- [Low] ve: failcount warning shares one static ratelimit state across all containers
--

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

ve: add neigh_tbl failcounter

This commit adds a per-VE neigh_tbl_failcount counter, exposes it in
ve.failcount, and increments it from neigh_alloc() when the neighbour
table overflows. It also changes the existing overflow message to include
the VE name.

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

> 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,9 @@ 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);
> +			net_info_ratelimited("%s: neighbor table overflow for ve %s!\n",
> +					     tbl->id, ve_name(ve));
> +			ve_failcount_inc(ve, neigh_tbl);

[Severity: Low]
Is the reworded net_info_ratelimited() message still needed here?

ve_failcount_inc() already expands to __ve_pr_warn_ratelimited(VE_LOG_BOTH,
...), which prints "CT%s: hits the neigh_tbl limit, see the 'neigh_tbl'
counter in ve.failcount ..." to both the host log and the VE log.

A single overflow in neigh_alloc() therefore produces two log lines from
two separate ratelimit states.

The commit message also does not mention that the existing "neighbor table
overflow!" text was changed to add the VE name.

[Severity: Low]
Is the failcount charged correctly when no per-VE counter exists?

get_perve_tbl_entries_counter() returns a per-VE counter only for AF_INET
and AF_INET6:

    switch (tbl->family) {
    case AF_INET:
            return &ve->arp_neigh_nr;
    case AF_INET6:
            return &ve->nd_neigh_nr;
    }
    return NULL;

For any other family, cnt is NULL in neigh_alloc(), so entries is set to
glob_entries and the entries >= gc_thresh3 test compares the global
tbl->gc_entries against the global threshold.

ve_failcount_inc(ve, neigh_tbl) is still called unconditionally in that
branch, so the VE owning the device is charged for a table-wide overflow.

The commit message says it counts "hits of the per-VE neigh_tbl limit".
Should the increment be limited to the case where cnt is non-NULL?

[Severity: Low]
Can one container suppress the warning for other containers here?

A container can fill its own neighbour table, for example by flooding
ARP/ND entries on its own netns, and reach this overflow branch in
neigh_alloc(). ve_failcount_inc() then calls __ve_printk_ratelimited(),
which declares one static ratelimit state per call site:

    static DEFINE_RATELIMIT_STATE(_rs,
                                  DEFAULT_RATELIMIT_INTERVAL,
                                  DEFAULT_RATELIMIT_BURST);
    if (ve_ratelimit(dst, ve, &_rs))

Because the state is shared by all VEs, a busy container could use up the
burst budget and hide the warning for other containers. The atomic
failcount itself stays correct.

>  			NEIGH_CACHE_STAT_INC(tbl, table_fulls);
>  			goto out_entries;
>  		}

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

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

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

[Vladimir Riabchun]

On 10/6/26 23:58, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] printk: ve_vprintk_deferred() raises non-deferred WARN_ON_ONCE() on invalid dst
> --
> 
> commit 57d6a4cedde45a78dd9af42f6ddde31d3e97b8fb
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve/printk: Add sanity checks in ve_printk functions
> 
> This commit adds a WARN_ON_ONCE() check on the dst argument in
> ve_dev_printk_emit(), ve_vprintk() and ve_vprintk_deferred(). It also
> reorders the VE_LOG test in the second if statement of each function.
> 
> 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));
>> +
> 
> [Severity: Low]
> Can this WARN_ON_ONCE() in ve_vprintk_deferred() cause problems in
> printk-unsafe contexts?
> 
> ve_vprintk_deferred() emits through vprintk_emit() at LOGLEVEL_SCHED
> so that it can be used where plain printk is not safe. The warning
> goes through the regular __warn() path, which prints via the normal,
> non-deferred printk.
> 
> If a caller passes dst == 0 or a value with bits outside VE_LOG_BOTH
> from such a context, the warning could recurse into or deadlock on
> console or logbuf locks:
> 
> ve_ratelimit() (lib/ratelimit.c)
>    ve_printk_deferred(dst, ve, KERN_WARNING ...)
>      ve_vprintk_deferred(dst, ...)
>        WARN_ON_ONCE(!dst || (dst & ~VE_LOG_BOTH));
> 
> I did not verify that any current caller passes an invalid dst, so this
> is only a possible problem. The warning fires at most once.
> 
> Would it make sense to drop the check from the deferred helper or use a
> deferred-safe way to report a bad dst?

No. Replied to v1, same issue:
1. dst == 0 is actually a bug
2. WARN_ON is not printing anything himself.

No fixes needed.

> 
>>   	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);
> 

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

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

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

[Vladimir Riabchun]

On 10/6/26 23:59, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] printk: WARN_ON_ONCE() inside ve_vprintk_deferred() defeats deferred printk
> - [Low] printk: non-task VE_LOG messages are dropped while host routing uses get_exec_env()
> - [Low] printk: WARN_ON_ONCE(!in_task()) runs before the ve_is_super() check
> --
> 
> commit b0fc9f5937694addbb72bb3c2cea42ba105b7306
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve/printk: Report potentially leaking usage of ve_printk functions
> 
> This commit adds WARN_ON_ONCE(!in_task()) checks to ve_log_state(),
> ve_vprintk(), ve_vprintk_deferred() and ve_dev_printk_emit(). When a
> VE_LOG message is requested from non-task context, the message is
> rejected (or the host log state is returned) instead of relying on
> get_exec_env().
> 
> 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]
> Is it intended that the WARN_ON_ONCE(!in_task()) is evaluated before the
> ve_is_super() check?
> 
> In ve_dev_printk_emit(), any non-task caller that passes dst with VE_LOG
> will hit the WARN even when the interrupted task is in ve0, where nothing
> could leak to a container. Could the ve_is_super() test be placed first so
> the WARN only fires when a container could actually be affected?

This is correct - interrupted task is a random value, we report
incorrect logic, not specific situation.

> 
> The same ordering appears in ve_vprintk() and ve_vprintk_deferred().
> 
> Also, the host branch just above still uses get_exec_env():
> 
> 	if (ve_is_super(get_exec_env()) || (dst & VE0_LOG))
> 		r = dev_vprintk_emit(level, dev, fmt, args);
> 
> In non-task context with dst == VE_LOG, if a host task was interrupted the
> message goes to the host log. If a container task was interrupted, the VE
> branch is rejected by the WARN guard, the message is dropped, and r stays
> 0. Doesn't this make the outcome depend on which task was interrupted,
> which is the get_exec_env() unreliability the commit message wants to
> avoid? ve_log_state() by contrast falls back to the host log. The same
> applies to ve_vprintk() and ve_vprintk_deferred().

It is fine if message goes to host.
We will drop a warning so we'll be able to get printk location
based on the call trace.

> 
>> 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 run while printk-side processing already holds locks?
> 
> ve_log_state() is called from ve_vprintk_emit(), so a WARN here can fire in
> the middle of a printk call. The WARN prints a banner and backtrace through
> the normal printk path.
> 
> ve_vprintk_emit()
>    ve_log_state()
>      WARN_ON_ONCE(!in_task())
>        __warn() -> printk
> 
> Could this recurse or deadlock if the caller is in scheduler, timer or
> other non-task context holding such locks? The WARN fires only once, and I
> did not find an in-tree caller that triggers this. In the final series
> state, the unconditional WARN in ve_log_state() on this path is still
> present.

Replied in v1.

> 
> [ ... ]
> 
>> @@ -2540,7 +2541,9 @@ static asmlinkage int ve_vprintk(int dst, const char *fmt, va_list 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);
> 
> [Severity: Medium]
> Does this make any IRQ or NMI caller of ve_printk() that passes VE_LOG
> trigger a WARN? See the comments on ve_log_state() above and on
> ve_vprintk_deferred() below regarding emitting a WARN from such contexts.
> 
>> @@ -2574,7 +2577,9 @@ static asmlinkage int ve_vprintk_deferred(int dst, const char *fmt, va_list 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);
> 
> [Severity: Medium]
> Does the synchronous WARN_ON_ONCE() inside ve_vprintk_deferred() defeat the
> purpose of the deferred variant?
> 
> This helper is meant to be safe from scheduler and IRQ contexts. If a
> caller outside task context (hardirq, softirq, NMI or scheduler code) calls
> it with dst containing VE_LOG and no explicit ve, the WARN goes through
> __warn() and the normal printk path. That path prints a banner and
> backtrace and may flush consoles or wake klogd.
> 
> Could this self-deadlock or recurse when the caller holds scheduler, timer
> or logbuf locks? The WARN fires only once. The only in-tree caller I found,
> __ve_ratelimit() in lib/ratelimit.c, runs under rs->lock and passes ve, so
> I have no concrete deadlocking caller. In the final series state,
> ve_vprintk_deferred() still has the check
> "if (!ve_provided && WARN_ON_ONCE(!in_task()))", so the concern remains
> when no ve is provided.

All of the above is incorrect due to the nature of WARN.

> 

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

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

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

[Vladimir Riabchun]

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

True, will remove in v3.

> 
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
> 
> [ ... ]
> 
>> -static asmlinkage int ve_vprintk(int dst, const char *fmt, va_list args)
>> +static asmlinkage int ve_vprintk(int dst, struct ve_struct *ve,
>> +				const char *fmt, va_list args)
>>   {
>>   	va_list args2;
>> -	int r = 0;
>> +	int r = 0, ve_provided = (ve != NULL);
>>   
>>   	WARN_ON_ONCE(!dst || (dst & ~VE_LOG_BOTH));
>>   
>> +	if (!ve)
>> +		ve = get_exec_env();
>> +
>>   	va_copy(args2, args);
>> -	if (ve_is_super(ve) || (dst & VE0_LOG))
>> +	if (ve_is_super(ve) || (dst & VE0_LOG))
>>   		r = vprintk(fmt, args);
>>   
>>   	/* Report IRQ usage here - printing to VE requires explicit ve reference */
>> -	if ((dst & VE_LOG) && !WARN_ON_ONCE(!in_task()) && !ve_is_super(get_exec_env()))
>> -		r = ve_vprintk_emit(0, LOGLEVEL_DEFAULT, NULL, fmt, args2);
>> +	if ((dst & VE_LOG)) {
>> +		if (!ve_provided && WARN_ON_ONCE(!in_task()))
>> +			goto done;
>> +		if (!ve_is_super(ve))
>> +			r = __ve_vprintk_emit(ve, 0, LOGLEVEL_DEFAULT, NULL, fmt, args2);
>> +	}
>> +done:
>>   	va_end(args2);
> 
> [Severity: Medium]
> Before this patch, the VE log branch was skipped whenever in_task() was
> false. With an explicit VE the in_task() guard is now bypassed.

Yes, this is intentional.

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

It could do it before the patch series as well.

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

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

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

Interrupt != scheduler.

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

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

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

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

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

I was looking for this whitespace for days...

> 
> [ ... ]
> 

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

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

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

[Vladimir Riabchun]

On 10/7/26 00:04, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] printk: time_ns refcount get/put in any-context vprintk_store_log() path
> - [Low] printk: boottime shift baked in at store time leaves early records unshifted
> - [Low] printk: init_log_state references ve0 without a CONFIG_VE guard
> --
> 
> commit 5c30ca9d767fcbace4f34b24a8dcf1364e21ebb6
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve/printk: Fix time virtualization
> 
> This patch moves the container boottime shift from print time (where
> get_exec_env() was used) to message store time in vprintk_store_log().
> The shift now uses the owning VE of the log_state, so IRQ printks no
> longer pick up the wrong container's offsets.
> 
> 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
> 
> [ ... ]
> 
>> +#define GET_VE_LOG(ve) ((struct log_state *) ve->log_state)
>> +
> 
> [Severity: Low]
> This isn't a bug, but GET_VE_LOG() is not used anywhere in kernel/printk,
> and it does not parenthesize its argument, so GET_VE_LOG(a + b) would
> expand to a + b->log_state. Is the macro needed in this patch?

True, missed it. Will remove in v3.

> 
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
> 
> [ ... ]
> 
>> @@ -534,6 +534,7 @@ struct log_state init_log_state = {
>>   		.val[1]	= 0,
>>   	},
>>   	.prb = &printk_rb_static,
>> +	.owner_ve = &ve0,
>>   };
> 
> [Severity: Low]
> Will this build when CONFIG_PRINTK=y and CONFIG_VE=n? The declaration
> extern struct ve_struct ve0; in include/linux/ve_proto.h is inside
> #ifdef CONFIG_VE, and the !CONFIG_VE branch only provides the get_ve0()
> stub.
> 
> Other VE-specific code in this file is guarded by #ifdef CONFIG_VE, so
> should the .owner_ve initializer be guarded in the same way?

Eh, will fix in v3.

> 
> [ ... ]
> 
>> -static inline u64 ve_timens_add_boottime_ns(u64 nsec)
>> +static inline u64 ve_timens_add_boottime_ns(u64 nsec, struct ve_struct *ve)
>>   {
>>   	struct time_namespace *ve_time_ns;
>>   	struct timens_offsets *ns_offsets;
>>   
>> -	ve_time_ns = ve_get_time_ns(get_exec_env());
>> +	ve_time_ns = ve_get_time_ns(ve);
>>   	if (unlikely(!ve_time_ns)) {
>>   		/* container not yet started */
>>   		return nsec;
> 
> [ ... ]
> 
>> @@ -2329,6 +2325,7 @@ static int vprintk_store_log(struct log_state *log, int facility, int level,
>>   	 * timestamp with respect to the caller.
>>   	 */
>>   	ts_nsec = local_clock();
>> +	ts_nsec = ve_timens_add_boottime_ns(ts_nsec, log->owner_ve);
> 
> [Severity: Medium]
> Can this take and drop a time_ns reference from a context where the final
> put is unsafe?
> 
> vprintk_store_log() runs in the any-context store path, after
> printk_enter_irqsave(). It now calls:
> 
> vprintk_store_log()
>    ve_timens_add_boottime_ns()
>      ve_get_time_ns()     /* get_time_ns() under rcu_read_lock() */
>      put_time_ns()
> 
> ve_get_time_ns() holds the reference past the RCU section. Consider the
> teardown side in ve_drop_context():
> 
> rcu_assign_pointer(ve->ve_nsproxy, NULL);
> synchronize_rcu();
> put_nsproxy(ve_nsproxy);
> 
> If a printk took its reference before the pointer was cleared, can it end
> up holding the last time_ns reference? In that case put_time_ns() would
> call free_time_ns() from inside the printk store path, which may be in
> hardirq or other atomic context.
> 
> Separately, this adds an RCU lock plus an atomic inc/dec on every printk
> record, including host (ve0) records, where it touches init_time_ns.
> 
> Would it be possible to read the offsets under RCU without taking a
> reference, cache the boottime offset in log_state, or skip ve0?

According to RCU doc: "anything that disables bottom halves, preemption,
or interrupts also enters an RCU read-side critical section."

ve_timens_add_boottime_ns is called from vprintk_store_log right after
printk_enter_irqsave, so synchronize_rcu will wait anyway.

Also: ve_exit_ns means that VE is stopped and no tasks are there.
But reaching printk with given VE means that either an interrupted task
is still in VE or we got it from somewhere like net structure.
In any case, this is not a deferred usage, so we can't use stale values
there.

About performance - I don't think it's a big problem. Dmesg logs are
expected to be rare enough, frequent printing is bounded by serial
console and may cause stalls.

> 
> [Severity: Low]
> Is it intended that records stored while ve->ve_nsproxy is NULL keep the
> raw host uptime permanently?
> 
> ve_log_init() runs at css_alloc, but ve_nsproxy is only set later by
> ve_grab_context(). Until then ve_get_time_ns() returns NULL and
> ve_timens_add_boottime_ns() returns the value unshifted:
> 
> 	if (unlikely(!ve_time_ns)) {
> 		/* container not yet started */
> 		return nsec;
> 	}
> 
> Before this patch the shift was applied at read time, so every record
> was shifted consistently once the container had started. Now the
> container's dmesg can show a mixed timeline, and the raw ts_nsec in the
> VE ring buffer differs from what was read before. The commit message does
> not mention this change in behavior.

Shifting time when printing was not reliable as well, it could happen
anytime.

The idea of this whole patchset is to avoid printing messages to
random containers. We normally can't get a situation when VE is stopped
and there are processes inside it(except selftests), so it is not
possible to print into stopped VE.

> 
> [ ... ]
> 

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

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

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

[Vladimir Riabchun]

On 10/7/26 00:06, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Low] net: neigh_alloc() logs one overflow twice and rewords the old message
> - [Low] net: neigh_tbl failcount is charged to the VE for global-limit overflows
> - [Low] ve: failcount warning shares one static ratelimit state across all containers
> --
> 
> commit 50b4fd033875ffa61097427a5f8a1d5372007065
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve: add neigh_tbl failcounter
> 
> This commit adds a per-VE neigh_tbl_failcount counter, exposes it in
> ve.failcount, and increments it from neigh_alloc() when the neighbour
> table overflows. It also changes the existing overflow message to include
> the VE name.
> 
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-135520
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-146839
> 
>> 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,9 @@ 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);
>> +			net_info_ratelimited("%s: neighbor table overflow for ve %s!\n",
>> +					     tbl->id, ve_name(ve));
>> +			ve_failcount_inc(ve, neigh_tbl);
> 
> [Severity: Low]
> Is the reworded net_info_ratelimited() message still needed here?
> 
> ve_failcount_inc() already expands to __ve_pr_warn_ratelimited(VE_LOG_BOTH,
> ...), which prints "CT%s: hits the neigh_tbl limit, see the 'neigh_tbl'
> counter in ve.failcount ..." to both the host log and the VE log.
> 
> A single overflow in neigh_alloc() therefore produces two log lines from
> two separate ratelimit states.
> 
> The commit message also does not mention that the existing "neighbor table
> overflow!" text was changed to add the VE name.

Kept old message to print table id. You asked in v1.

> 
> [Severity: Low]
> Is the failcount charged correctly when no per-VE counter exists?
> 
> get_perve_tbl_entries_counter() returns a per-VE counter only for AF_INET
> and AF_INET6:
> 
>      switch (tbl->family) {
>      case AF_INET:
>              return &ve->arp_neigh_nr;
>      case AF_INET6:
>              return &ve->nd_neigh_nr;
>      }
>      return NULL;
> 
> For any other family, cnt is NULL in neigh_alloc(), so entries is set to
> glob_entries and the entries >= gc_thresh3 test compares the global
> tbl->gc_entries against the global threshold.
> 
> ve_failcount_inc(ve, neigh_tbl) is still called unconditionally in that
> branch, so the VE owning the device is charged for a table-wide overflow.
> 
> The commit message says it counts "hits of the per-VE neigh_tbl limit".
> Should the increment be limited to the case where cnt is non-NULL?

As said in v1 reply, limit is global and doesn't depend on the type.
The code there is already ugly, no point in making it more complex.

> 
> [Severity: Low]
> Can one container suppress the warning for other containers here?
> 
> A container can fill its own neighbour table, for example by flooding
> ARP/ND entries on its own netns, and reach this overflow branch in
> neigh_alloc(). ve_failcount_inc() then calls __ve_printk_ratelimited(),
> which declares one static ratelimit state per call site:
> 
>      static DEFINE_RATELIMIT_STATE(_rs,
>                                    DEFAULT_RATELIMIT_INTERVAL,
>                                    DEFAULT_RATELIMIT_BURST);
>      if (ve_ratelimit(dst, ve, &_rs))
> 
> Because the state is shared by all VEs, a busy container could use up the
> burst budget and hide the warning for other containers. The atomic
> failcount itself stays correct.
> 
>>   			NEIGH_CACHE_STAT_INC(tbl, table_fulls);
>>   			goto out_entries;
>>   		}

Yes, it can. We don't have per-container ratelimits, they won't work.

> 

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

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