linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH for 4.19] tracepoint: Fix: tracepoint array element size mismatch
@ 2018-10-13 19:10 Mathieu Desnoyers
  2018-10-15 12:23 ` Jessica Yu
  0 siblings, 1 reply; 3+ messages in thread
From: Mathieu Desnoyers @ 2018-10-13 19:10 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: linux-kernel, Mathieu Desnoyers, Michael Ellerman, Ingo Molnar,
	Ard Biesheuvel, Arnd Bergmann, Benjamin Herrenschmidt,
	Bjorn Helgaas, Catalin Marinas, James Morris, James Morris,
	Jessica Yu, Josh Poimboeuf, Kees Cook, Nicolas Pitre,
	Paul Mackerras, Petr Mladek, Russell King, Serge E. Hallyn,
	Sergey Senozhatsky, Thomas Garnier, Thomas Gleixner, Will Deacon,
	Andrew Morton, Linus Torvalds, Greg Kroah-Hartman

commit 46e0c9be206f ("kernel: tracepoints: add support for relative
references") changes the layout of the __tracepoint_ptrs section on
architectures supporting relative references. However, it does so
without turning struct tracepoint * const into const int elsewhere in
the tracepoint code, which has the following side-effect:

Setting mod->num_tracepoints is done in by module.c:

    mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs",
                                         sizeof(*mod->tracepoints_ptrs),
                                         &mod->num_tracepoints);

Basically, since sizeof(*mod->tracepoints_ptrs) is a pointer size
(rather than sizeof(int)), num_tracepoints is erroneously set to half the
size it should be on 64-bit arch. So a module with an odd number of
tracepoints misses the last tracepoint due to effect of integer
division.

So in the module going notifier:

        for_each_tracepoint_range(mod->tracepoints_ptrs,
                mod->tracepoints_ptrs + mod->num_tracepoints,
                tp_module_going_check_quiescent, NULL);

the expression (mod->tracepoints_ptrs + mod->num_tracepoints) actually
evaluates to something within the bounds of the array, but miss the
last tracepoint if the number of tracepoints is odd on 64-bit arch.

Fix this by introducing a new typedef: tracepoint_ptr_t, which
is either "const int" on architectures that have PREL32 relocations,
or "struct tracepoint * const" on architectures that does not have
this feature.

Also provide a new tracepoint_ptr_defer() static inline to
encapsulate deferencing this type rather than duplicate code and
ugly idefs within the for_each_tracepoint_range() implementation.

This issue appears in 4.19-rc kernels, and should ideally be fixed
before the end of the rc cycle.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Link: http://lkml.kernel.org/r/20180704083651.24360-7-ard.biesheuvel@linaro.org
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Steven Rostedt (VMware) <rostedt@goodmis.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: James Morris <james.morris@microsoft.com>
Cc: James Morris <jmorris@namei.org>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Nicolas Pitre <nico@linaro.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Cc: Thomas Garnier <thgarnie@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/linux/module.h          |  3 ++-
 include/linux/tracepoint-defs.h |  6 ++++++
 include/linux/tracepoint.h      | 36 +++++++++++++++++++++------------
 kernel/tracepoint.c             | 24 ++++++++--------------
 4 files changed, 39 insertions(+), 30 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index f807f15bebbe..e19ae08c7fb8 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -20,6 +20,7 @@
 #include <linux/export.h>
 #include <linux/rbtree_latch.h>
 #include <linux/error-injection.h>
+#include <linux/tracepoint-defs.h>
 
 #include <linux/percpu.h>
 #include <asm/module.h>
@@ -430,7 +431,7 @@ struct module {
 
 #ifdef CONFIG_TRACEPOINTS
 	unsigned int num_tracepoints;
-	struct tracepoint * const *tracepoints_ptrs;
+	tracepoint_ptr_t *tracepoints_ptrs;
 #endif
 #ifdef HAVE_JUMP_LABEL
 	struct jump_entry *jump_entries;
diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
index 22c5a46e9693..49ba9cde7e4b 100644
--- a/include/linux/tracepoint-defs.h
+++ b/include/linux/tracepoint-defs.h
@@ -35,6 +35,12 @@ struct tracepoint {
 	struct tracepoint_func __rcu *funcs;
 };
 
+#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
+typedef const int tracepoint_ptr_t;
+#else
+typedef struct tracepoint * const tracepoint_ptr_t;
+#endif
+
 struct bpf_raw_event_map {
 	struct tracepoint	*tp;
 	void			*bpf_func;
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 041f7e56a289..538ba1a58f5b 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -99,6 +99,29 @@ extern void syscall_unregfunc(void);
 #define TRACE_DEFINE_ENUM(x)
 #define TRACE_DEFINE_SIZEOF(x)
 
+#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
+static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
+{
+	return offset_to_ptr(p);
+}
+
+#define __TRACEPOINT_ENTRY(name)					\
+	asm("	.section \"__tracepoints_ptrs\", \"a\"		\n"	\
+	    "	.balign 4					\n"	\
+	    "	.long 	__tracepoint_" #name " - .		\n"	\
+	    "	.previous					\n")
+#else
+static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
+{
+	return *p;
+}
+
+#define __TRACEPOINT_ENTRY(name)					 \
+	static tracepoint_ptr_t __tracepoint_ptr_##name __used		 \
+	__attribute__((section("__tracepoints_ptrs"))) =		 \
+		&__tracepoint_##name
+#endif
+
 #endif /* _LINUX_TRACEPOINT_H */
 
 /*
@@ -253,19 +276,6 @@ extern void syscall_unregfunc(void);
 		return static_key_false(&__tracepoint_##name.key);	\
 	}
 
-#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
-#define __TRACEPOINT_ENTRY(name)					\
-	asm("	.section \"__tracepoints_ptrs\", \"a\"		\n"	\
-	    "	.balign 4					\n"	\
-	    "	.long 	__tracepoint_" #name " - .		\n"	\
-	    "	.previous					\n")
-#else
-#define __TRACEPOINT_ENTRY(name)					 \
-	static struct tracepoint * const __tracepoint_ptr_##name __used	 \
-	__attribute__((section("__tracepoints_ptrs"))) =		 \
-		&__tracepoint_##name
-#endif
-
 /*
  * We have no guarantee that gcc and the linker won't up-align the tracepoint
  * structures, so we create an array of pointers that will be used for iteration
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index bf2c06ef9afc..a3be42304485 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -28,8 +28,8 @@
 #include <linux/sched/task.h>
 #include <linux/static_key.h>
 
-extern struct tracepoint * const __start___tracepoints_ptrs[];
-extern struct tracepoint * const __stop___tracepoints_ptrs[];
+extern tracepoint_ptr_t __start___tracepoints_ptrs[];
+extern tracepoint_ptr_t __stop___tracepoints_ptrs[];
 
 DEFINE_SRCU(tracepoint_srcu);
 EXPORT_SYMBOL_GPL(tracepoint_srcu);
@@ -371,25 +371,17 @@ int tracepoint_probe_unregister(struct tracepoint *tp, void *probe, void *data)
 }
 EXPORT_SYMBOL_GPL(tracepoint_probe_unregister);
 
-static void for_each_tracepoint_range(struct tracepoint * const *begin,
-		struct tracepoint * const *end,
+static void for_each_tracepoint_range(
+		tracepoint_ptr_t *begin, tracepoint_ptr_t *end,
 		void (*fct)(struct tracepoint *tp, void *priv),
 		void *priv)
 {
+	tracepoint_ptr_t *iter;
+
 	if (!begin)
 		return;
-
-	if (IS_ENABLED(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS)) {
-		const int *iter;
-
-		for (iter = (const int *)begin; iter < (const int *)end; iter++)
-			fct(offset_to_ptr(iter), priv);
-	} else {
-		struct tracepoint * const *iter;
-
-		for (iter = begin; iter < end; iter++)
-			fct(*iter, priv);
-	}
+	for (iter = begin; iter < end; iter++)
+		fct(tracepoint_ptr_deref(iter), priv);
 }
 
 #ifdef CONFIG_MODULES
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH for 4.19] tracepoint: Fix: tracepoint array element size mismatch
  2018-10-13 19:10 [PATCH for 4.19] tracepoint: Fix: tracepoint array element size mismatch Mathieu Desnoyers
@ 2018-10-15 12:23 ` Jessica Yu
  2018-10-15 15:48   ` Steven Rostedt
  0 siblings, 1 reply; 3+ messages in thread
From: Jessica Yu @ 2018-10-15 12:23 UTC (permalink / raw)
  To: Mathieu Desnoyers
  Cc: Steven Rostedt, linux-kernel, Michael Ellerman, Ingo Molnar,
	Ard Biesheuvel, Arnd Bergmann, Benjamin Herrenschmidt,
	Bjorn Helgaas, Catalin Marinas, James Morris, James Morris,
	Josh Poimboeuf, Kees Cook, Nicolas Pitre, Paul Mackerras,
	Petr Mladek, Russell King, Serge E. Hallyn, Sergey Senozhatsky,
	Thomas Garnier, Thomas Gleixner, Will Deacon, Andrew Morton,
	Linus Torvalds, Greg Kroah-Hartman

+++ Mathieu Desnoyers [13/10/18 15:10 -0400]:
>commit 46e0c9be206f ("kernel: tracepoints: add support for relative
>references") changes the layout of the __tracepoint_ptrs section on
>architectures supporting relative references. However, it does so
>without turning struct tracepoint * const into const int elsewhere in
>the tracepoint code, which has the following side-effect:
>
>Setting mod->num_tracepoints is done in by module.c:
>
>    mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs",
>                                         sizeof(*mod->tracepoints_ptrs),
>                                         &mod->num_tracepoints);
>
>Basically, since sizeof(*mod->tracepoints_ptrs) is a pointer size
>(rather than sizeof(int)), num_tracepoints is erroneously set to half the
>size it should be on 64-bit arch. So a module with an odd number of
>tracepoints misses the last tracepoint due to effect of integer
>division.
>
>So in the module going notifier:
>
>        for_each_tracepoint_range(mod->tracepoints_ptrs,
>                mod->tracepoints_ptrs + mod->num_tracepoints,
>                tp_module_going_check_quiescent, NULL);
>
>the expression (mod->tracepoints_ptrs + mod->num_tracepoints) actually
>evaluates to something within the bounds of the array, but miss the
>last tracepoint if the number of tracepoints is odd on 64-bit arch.
>
>Fix this by introducing a new typedef: tracepoint_ptr_t, which
>is either "const int" on architectures that have PREL32 relocations,
>or "struct tracepoint * const" on architectures that does not have
>this feature.
>
>Also provide a new tracepoint_ptr_defer() static inline to
>encapsulate deferencing this type rather than duplicate code and
>ugly idefs within the for_each_tracepoint_range() implementation.
>
>This issue appears in 4.19-rc kernels, and should ideally be fixed
>before the end of the rc cycle.
>
>Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>Link: http://lkml.kernel.org/r/20180704083651.24360-7-ard.biesheuvel@linaro.org
>Cc: Michael Ellerman <mpe@ellerman.id.au>
>Cc: Ingo Molnar <mingo@kernel.org>
>Cc: Steven Rostedt (VMware) <rostedt@goodmis.org>
>Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>Cc: Arnd Bergmann <arnd@arndb.de>
>Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>Cc: Bjorn Helgaas <bhelgaas@google.com>
>Cc: Catalin Marinas <catalin.marinas@arm.com>
>Cc: James Morris <james.morris@microsoft.com>
>Cc: James Morris <jmorris@namei.org>
>Cc: Jessica Yu <jeyu@kernel.org>
>Cc: Josh Poimboeuf <jpoimboe@redhat.com>
>Cc: Kees Cook <keescook@chromium.org>
>Cc: Nicolas Pitre <nico@linaro.org>
>Cc: Paul Mackerras <paulus@samba.org>
>Cc: Petr Mladek <pmladek@suse.com>
>Cc: Russell King <linux@armlinux.org.uk>
>Cc: "Serge E. Hallyn" <serge@hallyn.com>
>Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>Cc: Thomas Garnier <thgarnie@google.com>
>Cc: Thomas Gleixner <tglx@linutronix.de>
>Cc: Will Deacon <will.deacon@arm.com>
>Cc: Andrew Morton <akpm@linux-foundation.org>
>Cc: Linus Torvalds <torvalds@linux-foundation.org>
>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Whoops, acked the first version of the patch instead of this one.
In any case, for module parts:

Acked-by: Jessica Yu <jeyu@kernel.org>

>---
> include/linux/module.h          |  3 ++-
> include/linux/tracepoint-defs.h |  6 ++++++
> include/linux/tracepoint.h      | 36 +++++++++++++++++++++------------
> kernel/tracepoint.c             | 24 ++++++++--------------
> 4 files changed, 39 insertions(+), 30 deletions(-)
>
>diff --git a/include/linux/module.h b/include/linux/module.h
>index f807f15bebbe..e19ae08c7fb8 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -20,6 +20,7 @@
> #include <linux/export.h>
> #include <linux/rbtree_latch.h>
> #include <linux/error-injection.h>
>+#include <linux/tracepoint-defs.h>
>
> #include <linux/percpu.h>
> #include <asm/module.h>
>@@ -430,7 +431,7 @@ struct module {
>
> #ifdef CONFIG_TRACEPOINTS
> 	unsigned int num_tracepoints;
>-	struct tracepoint * const *tracepoints_ptrs;
>+	tracepoint_ptr_t *tracepoints_ptrs;
> #endif
> #ifdef HAVE_JUMP_LABEL
> 	struct jump_entry *jump_entries;
>diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
>index 22c5a46e9693..49ba9cde7e4b 100644
>--- a/include/linux/tracepoint-defs.h
>+++ b/include/linux/tracepoint-defs.h
>@@ -35,6 +35,12 @@ struct tracepoint {
> 	struct tracepoint_func __rcu *funcs;
> };
>
>+#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
>+typedef const int tracepoint_ptr_t;
>+#else
>+typedef struct tracepoint * const tracepoint_ptr_t;
>+#endif
>+
> struct bpf_raw_event_map {
> 	struct tracepoint	*tp;
> 	void			*bpf_func;
>diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
>index 041f7e56a289..538ba1a58f5b 100644
>--- a/include/linux/tracepoint.h
>+++ b/include/linux/tracepoint.h
>@@ -99,6 +99,29 @@ extern void syscall_unregfunc(void);
> #define TRACE_DEFINE_ENUM(x)
> #define TRACE_DEFINE_SIZEOF(x)
>
>+#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
>+static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>+{
>+	return offset_to_ptr(p);
>+}
>+
>+#define __TRACEPOINT_ENTRY(name)					\
>+	asm("	.section \"__tracepoints_ptrs\", \"a\"		\n"	\
>+	    "	.balign 4					\n"	\
>+	    "	.long 	__tracepoint_" #name " - .		\n"	\
>+	    "	.previous					\n")
>+#else
>+static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>+{
>+	return *p;
>+}
>+
>+#define __TRACEPOINT_ENTRY(name)					 \
>+	static tracepoint_ptr_t __tracepoint_ptr_##name __used		 \
>+	__attribute__((section("__tracepoints_ptrs"))) =		 \
>+		&__tracepoint_##name
>+#endif
>+
> #endif /* _LINUX_TRACEPOINT_H */
>
> /*
>@@ -253,19 +276,6 @@ extern void syscall_unregfunc(void);
> 		return static_key_false(&__tracepoint_##name.key);	\
> 	}
>
>-#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
>-#define __TRACEPOINT_ENTRY(name)					\
>-	asm("	.section \"__tracepoints_ptrs\", \"a\"		\n"	\
>-	    "	.balign 4					\n"	\
>-	    "	.long 	__tracepoint_" #name " - .		\n"	\
>-	    "	.previous					\n")
>-#else
>-#define __TRACEPOINT_ENTRY(name)					 \
>-	static struct tracepoint * const __tracepoint_ptr_##name __used	 \
>-	__attribute__((section("__tracepoints_ptrs"))) =		 \
>-		&__tracepoint_##name
>-#endif
>-
> /*
>  * We have no guarantee that gcc and the linker won't up-align the tracepoint
>  * structures, so we create an array of pointers that will be used for iteration
>diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
>index bf2c06ef9afc..a3be42304485 100644
>--- a/kernel/tracepoint.c
>+++ b/kernel/tracepoint.c
>@@ -28,8 +28,8 @@
> #include <linux/sched/task.h>
> #include <linux/static_key.h>
>
>-extern struct tracepoint * const __start___tracepoints_ptrs[];
>-extern struct tracepoint * const __stop___tracepoints_ptrs[];
>+extern tracepoint_ptr_t __start___tracepoints_ptrs[];
>+extern tracepoint_ptr_t __stop___tracepoints_ptrs[];
>
> DEFINE_SRCU(tracepoint_srcu);
> EXPORT_SYMBOL_GPL(tracepoint_srcu);
>@@ -371,25 +371,17 @@ int tracepoint_probe_unregister(struct tracepoint *tp, void *probe, void *data)
> }
> EXPORT_SYMBOL_GPL(tracepoint_probe_unregister);
>
>-static void for_each_tracepoint_range(struct tracepoint * const *begin,
>-		struct tracepoint * const *end,
>+static void for_each_tracepoint_range(
>+		tracepoint_ptr_t *begin, tracepoint_ptr_t *end,
> 		void (*fct)(struct tracepoint *tp, void *priv),
> 		void *priv)
> {
>+	tracepoint_ptr_t *iter;
>+
> 	if (!begin)
> 		return;
>-
>-	if (IS_ENABLED(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS)) {
>-		const int *iter;
>-
>-		for (iter = (const int *)begin; iter < (const int *)end; iter++)
>-			fct(offset_to_ptr(iter), priv);
>-	} else {
>-		struct tracepoint * const *iter;
>-
>-		for (iter = begin; iter < end; iter++)
>-			fct(*iter, priv);
>-	}
>+	for (iter = begin; iter < end; iter++)
>+		fct(tracepoint_ptr_deref(iter), priv);
> }
>
> #ifdef CONFIG_MODULES
>-- 
>2.17.1
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH for 4.19] tracepoint: Fix: tracepoint array element size mismatch
  2018-10-15 12:23 ` Jessica Yu
@ 2018-10-15 15:48   ` Steven Rostedt
  0 siblings, 0 replies; 3+ messages in thread
From: Steven Rostedt @ 2018-10-15 15:48 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Mathieu Desnoyers, linux-kernel, Michael Ellerman, Ingo Molnar,
	Ard Biesheuvel, Arnd Bergmann, Benjamin Herrenschmidt,
	Bjorn Helgaas, Catalin Marinas, James Morris, James Morris,
	Josh Poimboeuf, Kees Cook, Nicolas Pitre, Paul Mackerras,
	Petr Mladek, Russell King, Serge E. Hallyn, Sergey Senozhatsky,
	Thomas Garnier, Thomas Gleixner, Will Deacon, Andrew Morton,
	Linus Torvalds, Greg Kroah-Hartman

On Mon, 15 Oct 2018 14:23:23 +0200
Jessica Yu <jeyu@kernel.org> wrote:

> Whoops, acked the first version of the patch instead of this one.
> In any case, for module parts:
> 
> Acked-by: Jessica Yu <jeyu@kernel.org>

Yep, I meant I was going to pull this patch in ;-)

-- Steve

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-10-15 15:48 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-13 19:10 [PATCH for 4.19] tracepoint: Fix: tracepoint array element size mismatch Mathieu Desnoyers
2018-10-15 12:23 ` Jessica Yu
2018-10-15 15:48   ` Steven Rostedt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).