LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v3] earlycon: Use a pointer table to fix __earlycon_table stride
@ 2018-03-20 22:02 Daniel Kurtz
  2018-03-20 22:12 ` Guenter Roeck
  0 siblings, 1 reply; 2+ messages in thread
From: Daniel Kurtz @ 2018-03-20 22:02 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Guenter Roeck
  Cc: adurbin, linux-kernel, Daniel Kurtz, Rob Herring, Frank Rowand,
	Jiri Slaby, Arnd Bergmann,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE,
	open list:SERIAL DRIVERS,
	open list:GENERIC INCLUDE/ASM HEADER FILES

Commit 99492c39f39f ("earlycon: Fix __earlycon_table stride") tried to fix
__earlycon_table stride by forcing the earlycon_id struct alignment to 32
and asking the linker to 32-byte align the __earlycon_table symbol.  This
fix was based on commit 07fca0e57fca92 ("tracing: Properly align linker
defined symbols") which tried a similar fix for the tracing subsystem.

However, this fix doesn't quite work because there is no guarantee that
gcc will place structures packed into an array format.  In fact, gcc 4.9
chooses to 64-byte align these structs by inserting additional padding
between the entries because it has no clue that they are supposed to be in
an array.  If we are unlucky, the linker will assign symbol
"__earlycon_table" to a 32-byte aligned address which does not correpsond
to the 64-byte alignbed contents of section "__earlycon_table".

To address this same problem, the fix to the tracing system was
subsequently re-implemented using a more robust table of pointers approach
by commits:
 3d56e331b653 ("tracing: Replace syscall_meta_data struct array with pointer array")
 654986462939 ("tracepoints: Fix section alignment using pointer array")
 e4a9ea5ee7c8 ("tracing: Replace trace_event struct array with pointer array")

Let's use this same "array of pointers to structs" approach for
EARLYCON_TABLE.

Fixes: 99492c39f39f ("earlycon: Fix __earlycon_table stride")
Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Suggested-by: Aaron Durbin <adurbin@chromium.org>
---
Changes since v2:
 * Use __initconst instead of __initdata to avoid h8300 and alpha kbuild errors

 drivers/of/fdt.c                  |  7 +++++--
 drivers/tty/serial/earlycon.c     |  6 ++++--
 include/asm-generic/vmlinux.lds.h |  2 +-
 include/linux/serial_core.h       | 21 ++++++++++++++-------
 4 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 84aa9d676375..6da20b9688f7 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -942,7 +942,7 @@ int __init early_init_dt_scan_chosen_stdout(void)
 	int offset;
 	const char *p, *q, *options = NULL;
 	int l;
-	const struct earlycon_id *match;
+	const struct earlycon_id **p_match;
 	const void *fdt = initial_boot_params;
 
 	offset = fdt_path_offset(fdt, "/chosen");
@@ -969,7 +969,10 @@ int __init early_init_dt_scan_chosen_stdout(void)
 		return 0;
 	}
 
-	for (match = __earlycon_table; match < __earlycon_table_end; match++) {
+	for (p_match = __earlycon_table; p_match < __earlycon_table_end;
+	     p_match++) {
+		const struct earlycon_id *match = *p_match;
+
 		if (!match->compatible[0])
 			continue;
 
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index a24278380fec..22683393a0f2 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -169,7 +169,7 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
  */
 int __init setup_earlycon(char *buf)
 {
-	const struct earlycon_id *match;
+	const struct earlycon_id **p_match;
 
 	if (!buf || !buf[0])
 		return -EINVAL;
@@ -177,7 +177,9 @@ int __init setup_earlycon(char *buf)
 	if (early_con.flags & CON_ENABLED)
 		return -EALREADY;
 
-	for (match = __earlycon_table; match < __earlycon_table_end; match++) {
+	for (p_match = __earlycon_table; p_match < __earlycon_table_end;
+	     p_match++) {
+		const struct earlycon_id *match = *p_match;
 		size_t len = strlen(match->name);
 
 		if (strncmp(buf, match->name, len))
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 1ab0e520d6fc..e17de55c2542 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -179,7 +179,7 @@
 #endif
 
 #ifdef CONFIG_SERIAL_EARLYCON
-#define EARLYCON_TABLE() STRUCT_ALIGN();			\
+#define EARLYCON_TABLE() . = ALIGN(8);				\
 			 VMLINUX_SYMBOL(__earlycon_table) = .;	\
 			 KEEP(*(__earlycon_table))		\
 			 VMLINUX_SYMBOL(__earlycon_table_end) = .;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index b32df49a3bd5..c4219b9cbb70 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -351,10 +351,10 @@ struct earlycon_id {
 	char	name[16];
 	char	compatible[128];
 	int	(*setup)(struct earlycon_device *, const char *options);
-} __aligned(32);
+};
 
-extern const struct earlycon_id __earlycon_table[];
-extern const struct earlycon_id __earlycon_table_end[];
+extern const struct earlycon_id *__earlycon_table[];
+extern const struct earlycon_id *__earlycon_table_end[];
 
 #if defined(CONFIG_SERIAL_EARLYCON) && !defined(MODULE)
 #define EARLYCON_USED_OR_UNUSED	__used
@@ -362,12 +362,19 @@ extern const struct earlycon_id __earlycon_table_end[];
 #define EARLYCON_USED_OR_UNUSED	__maybe_unused
 #endif
 
-#define OF_EARLYCON_DECLARE(_name, compat, fn)				\
-	static const struct earlycon_id __UNIQUE_ID(__earlycon_##_name)	\
-	     EARLYCON_USED_OR_UNUSED __section(__earlycon_table)	\
+#define _OF_EARLYCON_DECLARE(_name, compat, fn, unique_id)		\
+	static const struct earlycon_id unique_id			\
+	     EARLYCON_USED_OR_UNUSED __initconst			\
 		= { .name = __stringify(_name),				\
 		    .compatible = compat,				\
-		    .setup = fn  }
+		    .setup = fn  };					\
+	static const struct earlycon_id EARLYCON_USED_OR_UNUSED		\
+		__section(__earlycon_table)				\
+		* const __PASTE(__p, unique_id) = &unique_id
+
+#define OF_EARLYCON_DECLARE(_name, compat, fn)				\
+	_OF_EARLYCON_DECLARE(_name, compat, fn,				\
+			     __UNIQUE_ID(__earlycon_##_name))
 
 #define EARLYCON_DECLARE(_name, fn)	OF_EARLYCON_DECLARE(_name, "", fn)
 
-- 
2.16.2.804.g6dcf76e118-goog


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

* Re: [PATCH v3] earlycon: Use a pointer table to fix __earlycon_table stride
  2018-03-20 22:02 [PATCH v3] earlycon: Use a pointer table to fix __earlycon_table stride Daniel Kurtz
@ 2018-03-20 22:12 ` Guenter Roeck
  0 siblings, 0 replies; 2+ messages in thread
From: Guenter Roeck @ 2018-03-20 22:12 UTC (permalink / raw)
  To: Daniel Kurtz
  Cc: Greg Kroah-Hartman, Guenter Roeck, Aaron Durbin, linux-kernel,
	Rob Herring, Frank Rowand, Jiri Slaby, Arnd Bergmann,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE,
	open list:SERIAL DRIVERS,
	open list:GENERIC INCLUDE/ASM HEADER FILES

On Tue, Mar 20, 2018 at 3:02 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Commit 99492c39f39f ("earlycon: Fix __earlycon_table stride") tried to fix
> __earlycon_table stride by forcing the earlycon_id struct alignment to 32
> and asking the linker to 32-byte align the __earlycon_table symbol.  This
> fix was based on commit 07fca0e57fca92 ("tracing: Properly align linker
> defined symbols") which tried a similar fix for the tracing subsystem.
>
> However, this fix doesn't quite work because there is no guarantee that
> gcc will place structures packed into an array format.  In fact, gcc 4.9
> chooses to 64-byte align these structs by inserting additional padding
> between the entries because it has no clue that they are supposed to be in
> an array.  If we are unlucky, the linker will assign symbol
> "__earlycon_table" to a 32-byte aligned address which does not correpsond
> to the 64-byte alignbed contents of section "__earlycon_table".
>
> To address this same problem, the fix to the tracing system was
> subsequently re-implemented using a more robust table of pointers approach
> by commits:
>  3d56e331b653 ("tracing: Replace syscall_meta_data struct array with pointer array")
>  654986462939 ("tracepoints: Fix section alignment using pointer array")
>  e4a9ea5ee7c8 ("tracing: Replace trace_event struct array with pointer array")
>
> Let's use this same "array of pointers to structs" approach for
> EARLYCON_TABLE.
>
> Fixes: 99492c39f39f ("earlycon: Fix __earlycon_table stride")
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> Suggested-by: Aaron Durbin <adurbin@chromium.org>

Build tested on sh4, alpha, and h8300 to ensure that the build problem
reported by 0day is gone.
For that,

Tested-by: Guenter Roeck <groeck@chromium.org>

I am not an expert in this area, but the change makes sense to me. As such,

Reviewed-by: Guenter Roeck <groeck@chromium.org>

but take it with a grain of salt.

Guenter

> ---
> Changes since v2:
>  * Use __initconst instead of __initdata to avoid h8300 and alpha kbuild errors
>
>  drivers/of/fdt.c                  |  7 +++++--
>  drivers/tty/serial/earlycon.c     |  6 ++++--
>  include/asm-generic/vmlinux.lds.h |  2 +-
>  include/linux/serial_core.h       | 21 ++++++++++++++-------
>  4 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 84aa9d676375..6da20b9688f7 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -942,7 +942,7 @@ int __init early_init_dt_scan_chosen_stdout(void)
>         int offset;
>         const char *p, *q, *options = NULL;
>         int l;
> -       const struct earlycon_id *match;
> +       const struct earlycon_id **p_match;
>         const void *fdt = initial_boot_params;
>
>         offset = fdt_path_offset(fdt, "/chosen");
> @@ -969,7 +969,10 @@ int __init early_init_dt_scan_chosen_stdout(void)
>                 return 0;
>         }
>
> -       for (match = __earlycon_table; match < __earlycon_table_end; match++) {
> +       for (p_match = __earlycon_table; p_match < __earlycon_table_end;
> +            p_match++) {
> +               const struct earlycon_id *match = *p_match;
> +
>                 if (!match->compatible[0])
>                         continue;
>
> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> index a24278380fec..22683393a0f2 100644
> --- a/drivers/tty/serial/earlycon.c
> +++ b/drivers/tty/serial/earlycon.c
> @@ -169,7 +169,7 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
>   */
>  int __init setup_earlycon(char *buf)
>  {
> -       const struct earlycon_id *match;
> +       const struct earlycon_id **p_match;
>
>         if (!buf || !buf[0])
>                 return -EINVAL;
> @@ -177,7 +177,9 @@ int __init setup_earlycon(char *buf)
>         if (early_con.flags & CON_ENABLED)
>                 return -EALREADY;
>
> -       for (match = __earlycon_table; match < __earlycon_table_end; match++) {
> +       for (p_match = __earlycon_table; p_match < __earlycon_table_end;
> +            p_match++) {
> +               const struct earlycon_id *match = *p_match;
>                 size_t len = strlen(match->name);
>
>                 if (strncmp(buf, match->name, len))
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 1ab0e520d6fc..e17de55c2542 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -179,7 +179,7 @@
>  #endif
>
>  #ifdef CONFIG_SERIAL_EARLYCON
> -#define EARLYCON_TABLE() STRUCT_ALIGN();                       \
> +#define EARLYCON_TABLE() . = ALIGN(8);                         \
>                          VMLINUX_SYMBOL(__earlycon_table) = .;  \
>                          KEEP(*(__earlycon_table))              \
>                          VMLINUX_SYMBOL(__earlycon_table_end) = .;
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index b32df49a3bd5..c4219b9cbb70 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -351,10 +351,10 @@ struct earlycon_id {
>         char    name[16];
>         char    compatible[128];
>         int     (*setup)(struct earlycon_device *, const char *options);
> -} __aligned(32);
> +};
>
> -extern const struct earlycon_id __earlycon_table[];
> -extern const struct earlycon_id __earlycon_table_end[];
> +extern const struct earlycon_id *__earlycon_table[];
> +extern const struct earlycon_id *__earlycon_table_end[];
>
>  #if defined(CONFIG_SERIAL_EARLYCON) && !defined(MODULE)
>  #define EARLYCON_USED_OR_UNUSED        __used
> @@ -362,12 +362,19 @@ extern const struct earlycon_id __earlycon_table_end[];
>  #define EARLYCON_USED_OR_UNUSED        __maybe_unused
>  #endif
>
> -#define OF_EARLYCON_DECLARE(_name, compat, fn)                         \
> -       static const struct earlycon_id __UNIQUE_ID(__earlycon_##_name) \
> -            EARLYCON_USED_OR_UNUSED __section(__earlycon_table)        \
> +#define _OF_EARLYCON_DECLARE(_name, compat, fn, unique_id)             \
> +       static const struct earlycon_id unique_id                       \
> +            EARLYCON_USED_OR_UNUSED __initconst                        \
>                 = { .name = __stringify(_name),                         \
>                     .compatible = compat,                               \
> -                   .setup = fn  }
> +                   .setup = fn  };                                     \
> +       static const struct earlycon_id EARLYCON_USED_OR_UNUSED         \
> +               __section(__earlycon_table)                             \
> +               * const __PASTE(__p, unique_id) = &unique_id
> +
> +#define OF_EARLYCON_DECLARE(_name, compat, fn)                         \
> +       _OF_EARLYCON_DECLARE(_name, compat, fn,                         \
> +                            __UNIQUE_ID(__earlycon_##_name))
>
>  #define EARLYCON_DECLARE(_name, fn)    OF_EARLYCON_DECLARE(_name, "", fn)
>
> --
> 2.16.2.804.g6dcf76e118-goog
>

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

end of thread, other threads:[~2018-03-20 22:12 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-20 22:02 [PATCH v3] earlycon: Use a pointer table to fix __earlycon_table stride Daniel Kurtz
2018-03-20 22:12 ` Guenter Roeck

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