[PATCH v1 0/3] kallsyms: Rust requirements

[PATCH v1 0/3] kallsyms: Rust requirements

[Miguel Ojeda]

These are the kallsyms patches we carry on the Rust patch series as
prerequisites. We were requested to submit them independently, so
here they are!

  - The first one is an improvement that may be applied even without
    the big symbol support.

  - The second adds support for "big" symbols without actually
    increasing the limit.

  - The third performs the actual increase.

Thanks!

Boqun Feng (1):
  kallsyms: avoid hardcoding the buffer size

Miguel Ojeda (2):
  kallsyms: support "big" kernel symbols
  kallsyms: increase maximum kernel symbol length to 512

 include/linux/kallsyms.h            |  2 +-
 kernel/kallsyms.c                   | 26 +++++++++++++---
 kernel/livepatch/core.c             |  4 +--
 scripts/kallsyms.c                  | 47 ++++++++++++++++++++++++-----
 tools/include/linux/kallsyms.h      |  2 +-
 tools/lib/perf/include/perf/event.h |  2 +-
 tools/lib/symbol/kallsyms.h         |  2 +-
 7 files changed, 68 insertions(+), 17 deletions(-)


base-commit: 672c0c5173427e6b3e2a9bbb7be51ceeec78093a
-- 
2.35.3

[PATCH v1 1/3] kallsyms: avoid hardcoding the buffer size

[Miguel Ojeda]

From: Boqun Feng <boqun.feng@gmail.com>

This makes it easier to update the size later on.

Furthermore, a static assert is added to ensure both are updated
when that happens. The relationship used is one that keeps the new
size (512+1) close to the original buffer size (500).

Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 scripts/kallsyms.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index 8caabddf817c..880c4404731b 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -27,8 +27,18 @@
 
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr[0]))
 
+#define _stringify_1(x)	#x
+#define _stringify(x)	_stringify_1(x)
+
 #define KSYM_NAME_LEN		128
 
+/* A substantially bigger size than the current maximum. */
+#define KSYM_NAME_LEN_BUFFER	512
+_Static_assert(
+	KSYM_NAME_LEN_BUFFER == KSYM_NAME_LEN * 4,
+	"Please keep KSYM_NAME_LEN_BUFFER in sync with KSYM_NAME_LEN"
+);
+
 struct sym_entry {
 	unsigned long long addr;
 	unsigned int len;
@@ -197,15 +207,15 @@ static void check_symbol_range(const char *sym, unsigned long long addr,
 
 static struct sym_entry *read_symbol(FILE *in)
 {
-	char name[500], type;
+	char name[KSYM_NAME_LEN_BUFFER+1], type;
 	unsigned long long addr;
 	unsigned int len;
 	struct sym_entry *sym;
 	int rc;
 
-	rc = fscanf(in, "%llx %c %499s\n", &addr, &type, name);
+	rc = fscanf(in, "%llx %c %" _stringify(KSYM_NAME_LEN_BUFFER) "s\n", &addr, &type, name);
 	if (rc != 3) {
-		if (rc != EOF && fgets(name, 500, in) == NULL)
+		if (rc != EOF && fgets(name, KSYM_NAME_LEN_BUFFER + 1, in) == NULL)
 			fprintf(stderr, "Read error or end of file.\n");
 		return NULL;
 	}
-- 
2.35.3

[PATCH v1 2/3] kallsyms: support "big" kernel symbols

[Miguel Ojeda]

Rust symbols can become quite long due to namespacing introduced
by modules, types, traits, generics, etc.

Increasing to 255 is not enough in some cases, and therefore
we need to introduce longer lengths to the symbol table.

In order to avoid increasing all lengths to 2 bytes (since most
of them are small, including many Rust ones), we use ULEB128 to
keep smaller symbols in 1 byte, with the rest in 2 bytes.

Co-developed-by: Alex Gaynor <alex.gaynor@gmail.com>
Signed-off-by: Alex Gaynor <alex.gaynor@gmail.com>
Co-developed-by: Wedson Almeida Filho <wedsonaf@google.com>
Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com>
Co-developed-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Gary Guo <gary@garyguo.net>
Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Co-developed-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 kernel/kallsyms.c  | 26 ++++++++++++++++++++++----
 scripts/kallsyms.c | 29 ++++++++++++++++++++++++++---
 2 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 79f2eb617a62..e8d2262ef2d2 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -69,12 +69,20 @@ static unsigned int kallsyms_expand_symbol(unsigned int off,
 	data = &kallsyms_names[off];
 	len = *data;
 	data++;
+	off++;
+
+	/* If MSB is 1, it is a "big" symbol, so needs an additional byte. */
+	if ((len & 0x80) != 0) {
+		len = (len & 0x7F) | (*data << 7);
+		data++;
+		off++;
+	}
 
 	/*
 	 * Update the offset to return the offset for the next symbol on
 	 * the compressed stream.
 	 */
-	off += len + 1;
+	off += len;
 
 	/*
 	 * For every byte on the compressed symbol data, copy the table
@@ -127,7 +135,7 @@ static char kallsyms_get_symbol_type(unsigned int off)
 static unsigned int get_symbol_offset(unsigned long pos)
 {
 	const u8 *name;
-	int i;
+	int i, len;
 
 	/*
 	 * Use the closest marker we have. We have markers every 256 positions,
@@ -141,8 +149,18 @@ static unsigned int get_symbol_offset(unsigned long pos)
 	 * so we just need to add the len to the current pointer for every
 	 * symbol we wish to skip.
 	 */
-	for (i = 0; i < (pos & 0xFF); i++)
-		name = name + (*name) + 1;
+	for (i = 0; i < (pos & 0xFF); i++) {
+		len = *name;
+
+		/*
+		 * If MSB is 1, it is a "big" symbol, so we need to look into
+		 * the next byte (and skip it, too).
+		 */
+		if ((len & 0x80) != 0)
+			len = ((len & 0x7F) | (name[1] << 7)) + 1;
+
+		name = name + len + 1;
+	}
 
 	return name - kallsyms_names;
 }
diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index 880c4404731b..c4e85eec2b4b 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -480,12 +480,35 @@ static void write_src(void)
 		if ((i & 0xFF) == 0)
 			markers[i >> 8] = off;
 
-		printf("\t.byte 0x%02x", table[i]->len);
+		/* There cannot be any symbol of length zero. */
+		if (table[i]->len == 0) {
+			fprintf(stderr, "kallsyms failure: "
+				"unexpected zero symbol length\n");
+			exit(EXIT_FAILURE);
+		}
+
+		/* Only lengths that fit in up-to-two-byte ULEB128 are supported. */
+		if (table[i]->len > 0x3FFF) {
+			fprintf(stderr, "kallsyms failure: "
+				"unexpected huge symbol length\n");
+			exit(EXIT_FAILURE);
+		}
+
+		/* Encode length with ULEB128. */
+		if (table[i]->len <= 0x7F) {
+			/* Most symbols use a single byte for the length. */
+			printf("\t.byte 0x%02x", table[i]->len);
+			off += table[i]->len + 1;
+		} else {
+			/* "Big" symbols use two bytes. */
+			printf("\t.byte 0x%02x, 0x%02x",
+				(table[i]->len & 0x7F) | 0x80,
+				(table[i]->len >> 7) & 0x7F);
+			off += table[i]->len + 2;
+		}
 		for (k = 0; k < table[i]->len; k++)
 			printf(", 0x%02x", table[i]->sym[k]);
 		printf("\n");
-
-		off += table[i]->len + 1;
 	}
 	printf("\n");
 
-- 
2.35.3

[PATCH v1 3/3] kallsyms: increase maximum kernel symbol length to 512

[Miguel Ojeda]

Rust symbols can become quite long due to namespacing introduced
by modules, types, traits, generics, etc. For instance,
the following code:

    pub mod my_module {
        pub struct MyType;
        pub struct MyGenericType<T>(T);

        pub trait MyTrait {
            fn my_method() -> u32;
        }

        impl MyTrait for MyGenericType<MyType> {
            fn my_method() -> u32 {
                42
            }
        }
    }

generates a symbol of length 96 when using the upcoming v0 mangling scheme:

    _RNvXNtCshGpAVYOtgW1_7example9my_moduleINtB2_13MyGenericTypeNtB2_6MyTypeENtB2_7MyTrait9my_method

At the moment, Rust symbols may reach up to 300 in length.
Setting 512 as the maximum seems like a reasonable choice to
keep some headroom.

Co-developed-by: Alex Gaynor <alex.gaynor@gmail.com>
Signed-off-by: Alex Gaynor <alex.gaynor@gmail.com>
Co-developed-by: Wedson Almeida Filho <wedsonaf@google.com>
Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com>
Co-developed-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Gary Guo <gary@garyguo.net>
Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 include/linux/kallsyms.h            | 2 +-
 kernel/livepatch/core.c             | 4 ++--
 scripts/kallsyms.c                  | 4 ++--
 tools/include/linux/kallsyms.h      | 2 +-
 tools/lib/perf/include/perf/event.h | 2 +-
 tools/lib/symbol/kallsyms.h         | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index ce1bd2fbf23e..e5ad6e31697d 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -15,7 +15,7 @@
 
 #include <asm/sections.h>
 
-#define KSYM_NAME_LEN 128
+#define KSYM_NAME_LEN 512
 #define KSYM_SYMBOL_LEN (sizeof("%s+%#lx/%#lx [%s %s]") + \
 			(KSYM_NAME_LEN - 1) + \
 			2*(BITS_PER_LONG*3/10) + (MODULE_NAME_LEN - 1) + \
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index bc475e62279d..ec06ce59d728 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -213,7 +213,7 @@ static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
 	 * we use the smallest/strictest upper bound possible (56, based on
 	 * the current definition of MODULE_NAME_LEN) to prevent overflows.
 	 */
-	BUILD_BUG_ON(MODULE_NAME_LEN < 56 || KSYM_NAME_LEN != 128);
+	BUILD_BUG_ON(MODULE_NAME_LEN < 56 || KSYM_NAME_LEN != 512);
 
 	relas = (Elf_Rela *) relasec->sh_addr;
 	/* For each rela in this klp relocation section */
@@ -227,7 +227,7 @@ static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
 
 		/* Format: .klp.sym.sym_objname.sym_name,sympos */
 		cnt = sscanf(strtab + sym->st_name,
-			     ".klp.sym.%55[^.].%127[^,],%lu",
+			     ".klp.sym.%55[^.].%511[^,],%lu",
 			     sym_objname, sym_name, &sympos);
 		if (cnt != 3) {
 			pr_err("symbol %s has an incorrectly formatted name\n",
diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index c4e85eec2b4b..f9d07f9eb709 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -30,10 +30,10 @@
 #define _stringify_1(x)	#x
 #define _stringify(x)	_stringify_1(x)
 
-#define KSYM_NAME_LEN		128
+#define KSYM_NAME_LEN		512
 
 /* A substantially bigger size than the current maximum. */
-#define KSYM_NAME_LEN_BUFFER	512
+#define KSYM_NAME_LEN_BUFFER	2048
 _Static_assert(
 	KSYM_NAME_LEN_BUFFER == KSYM_NAME_LEN * 4,
 	"Please keep KSYM_NAME_LEN_BUFFER in sync with KSYM_NAME_LEN"
diff --git a/tools/include/linux/kallsyms.h b/tools/include/linux/kallsyms.h
index efb6c3f5f2a9..5a37ccbec54f 100644
--- a/tools/include/linux/kallsyms.h
+++ b/tools/include/linux/kallsyms.h
@@ -6,7 +6,7 @@
 #include <stdio.h>
 #include <unistd.h>
 
-#define KSYM_NAME_LEN 128
+#define KSYM_NAME_LEN 512
 
 struct module;
 
diff --git a/tools/lib/perf/include/perf/event.h b/tools/lib/perf/include/perf/event.h
index e7758707cadd..116a80c31675 100644
--- a/tools/lib/perf/include/perf/event.h
+++ b/tools/lib/perf/include/perf/event.h
@@ -95,7 +95,7 @@ struct perf_record_throttle {
 };
 
 #ifndef KSYM_NAME_LEN
-#define KSYM_NAME_LEN 256
+#define KSYM_NAME_LEN 512
 #endif
 
 struct perf_record_ksymbol {
diff --git a/tools/lib/symbol/kallsyms.h b/tools/lib/symbol/kallsyms.h
index 72ab9870454b..542f9b059c3b 100644
--- a/tools/lib/symbol/kallsyms.h
+++ b/tools/lib/symbol/kallsyms.h
@@ -7,7 +7,7 @@
 #include <linux/types.h>
 
 #ifndef KSYM_NAME_LEN
-#define KSYM_NAME_LEN 256
+#define KSYM_NAME_LEN 512
 #endif
 
 static inline u8 kallsyms2elf_binding(char type)
-- 
2.35.3

Re: [PATCH v1 1/3] kallsyms: avoid hardcoding the buffer size

[Kees Cook]

On Thu, May 05, 2022 at 09:16:43PM +0200, Miguel Ojeda wrote:
> From: Boqun Feng <boqun.feng@gmail.com>
> 
> This makes it easier to update the size later on.
> 
> Furthermore, a static assert is added to ensure both are updated
> when that happens. The relationship used is one that keeps the new
> size (512+1) close to the original buffer size (500).
> 
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> ---
>  scripts/kallsyms.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
> index 8caabddf817c..880c4404731b 100644
> --- a/scripts/kallsyms.c
> +++ b/scripts/kallsyms.c
> @@ -27,8 +27,18 @@
>  
>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr[0]))
>  
> +#define _stringify_1(x)	#x
> +#define _stringify(x)	_stringify_1(x)
> +
>  #define KSYM_NAME_LEN		128
>  
> +/* A substantially bigger size than the current maximum. */
> +#define KSYM_NAME_LEN_BUFFER	512
> +_Static_assert(
> +	KSYM_NAME_LEN_BUFFER == KSYM_NAME_LEN * 4,
> +	"Please keep KSYM_NAME_LEN_BUFFER in sync with KSYM_NAME_LEN"
> +);
> +
>  struct sym_entry {
>  	unsigned long long addr;
>  	unsigned int len;
> @@ -197,15 +207,15 @@ static void check_symbol_range(const char *sym, unsigned long long addr,
>  
>  static struct sym_entry *read_symbol(FILE *in)
>  {
> -	char name[500], type;
> +	char name[KSYM_NAME_LEN_BUFFER+1], type;
>  	unsigned long long addr;
>  	unsigned int len;
>  	struct sym_entry *sym;
>  	int rc;
>  
> -	rc = fscanf(in, "%llx %c %499s\n", &addr, &type, name);
> +	rc = fscanf(in, "%llx %c %" _stringify(KSYM_NAME_LEN_BUFFER) "s\n", &addr, &type, name);
>  	if (rc != 3) {
> -		if (rc != EOF && fgets(name, 500, in) == NULL)
> +		if (rc != EOF && fgets(name, KSYM_NAME_LEN_BUFFER + 1, in) == NULL)

No need to repeat the sizing:

	fgets(name, sizeof(name), in)

>  			fprintf(stderr, "Read error or end of file.\n");
>  		return NULL;
>  	}
> -- 
> 2.35.3
> 

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v1 2/3] kallsyms: support "big" kernel symbols

[Kees Cook]

On Thu, May 05, 2022 at 09:16:44PM +0200, Miguel Ojeda wrote:
> Rust symbols can become quite long due to namespacing introduced
> by modules, types, traits, generics, etc.
> 
> Increasing to 255 is not enough in some cases, and therefore
> we need to introduce longer lengths to the symbol table.
> 
> In order to avoid increasing all lengths to 2 bytes (since most
> of them are small, including many Rust ones), we use ULEB128 to
> keep smaller symbols in 1 byte, with the rest in 2 bytes.
> 
> Co-developed-by: Alex Gaynor <alex.gaynor@gmail.com>
> Signed-off-by: Alex Gaynor <alex.gaynor@gmail.com>
> Co-developed-by: Wedson Almeida Filho <wedsonaf@google.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com>
> Co-developed-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Co-developed-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Looks good to me.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v1 3/3] kallsyms: increase maximum kernel symbol length to 512

[Kees Cook]

On Thu, May 05, 2022 at 09:16:45PM +0200, Miguel Ojeda wrote:
> Rust symbols can become quite long due to namespacing introduced
> by modules, types, traits, generics, etc. For instance,
> the following code:
> 
>     pub mod my_module {
>         pub struct MyType;
>         pub struct MyGenericType<T>(T);
> 
>         pub trait MyTrait {
>             fn my_method() -> u32;
>         }
> 
>         impl MyTrait for MyGenericType<MyType> {
>             fn my_method() -> u32 {
>                 42
>             }
>         }
>     }
> 
> generates a symbol of length 96 when using the upcoming v0 mangling scheme:
> 
>     _RNvXNtCshGpAVYOtgW1_7example9my_moduleINtB2_13MyGenericTypeNtB2_6MyTypeENtB2_7MyTrait9my_method
> 
> At the moment, Rust symbols may reach up to 300 in length.
> Setting 512 as the maximum seems like a reasonable choice to
> keep some headroom.
> 
> Co-developed-by: Alex Gaynor <alex.gaynor@gmail.com>
> Signed-off-by: Alex Gaynor <alex.gaynor@gmail.com>
> Co-developed-by: Wedson Almeida Filho <wedsonaf@google.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com>
> Co-developed-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

I look forward to aiming my demangler at /proc/kallsyms. ;)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v1 1/3] kallsyms: avoid hardcoding the buffer size

[Miguel Ojeda]

On Fri, May 6, 2022 at 1:46 AM Kees Cook <keescook@chromium.org> wrote:
>
> No need to repeat the sizing:
>
>         fgets(name, sizeof(name), in)

Definitely.

> Reviewed-by: Kees Cook <keescook@chromium.org>

Thanks for the three reviews!

Cheers,
Miguel

Re: [PATCH v1 3/3] kallsyms: increase maximum kernel symbol length to 512

[Petr Mladek]

On Thu 2022-05-05 21:16:45, Miguel Ojeda wrote:
> Rust symbols can become quite long due to namespacing introduced
> by modules, types, traits, generics, etc. For instance,
> the following code:
> 
>     pub mod my_module {
>         pub struct MyType;
>         pub struct MyGenericType<T>(T);
> 
>         pub trait MyTrait {
>             fn my_method() -> u32;
>         }
> 
>         impl MyTrait for MyGenericType<MyType> {
>             fn my_method() -> u32 {
>                 42
>             }
>         }
>     }
> 
> generates a symbol of length 96 when using the upcoming v0 mangling scheme:
> 
>     _RNvXNtCshGpAVYOtgW1_7example9my_moduleINtB2_13MyGenericTypeNtB2_6MyTypeENtB2_7MyTrait9my_method
> 
> At the moment, Rust symbols may reach up to 300 in length.
> Setting 512 as the maximum seems like a reasonable choice to
> keep some headroom.
> 
> Co-developed-by: Alex Gaynor <alex.gaynor@gmail.com>
> Signed-off-by: Alex Gaynor <alex.gaynor@gmail.com>
> Co-developed-by: Wedson Almeida Filho <wedsonaf@google.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com>
> Co-developed-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

I was primary interested into the livepatching code.
But the entire patch looks good:

Reviewed-by: Petr Mladek <pmladek@suse.com>

I just hope that it will not cause stack overflows
somewhere.

Best Regards,
Petr

Re: [PATCH v1 3/3] kallsyms: increase maximum kernel symbol length to 512

[Geert Stappers]

On Thu, May 05, 2022 at 09:16:45PM +0200, Miguel Ojeda wrote:
> 
> generates a symbol of length 96 when using the upcoming v0 mangling scheme:
> 
>     _RNvXNtCshGpAVYOtgW1_7example9my_moduleINtB2_13MyGenericTypeNtB2_6MyTypeENtB2_7MyTrait9my_method
> 
> At the moment, Rust symbols may reach up to 300 in length.
> Setting 512 as the maximum seems like a reasonable choice to
> keep some headroom.
> 
> diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
> index ce1bd2fbf23e..e5ad6e31697d 100644
> --- a/include/linux/kallsyms.h
> +++ b/include/linux/kallsyms.h
> @@ -15,7 +15,7 @@
>  
>  #include <asm/sections.h>
>  
> -#define KSYM_NAME_LEN 128
> +#define KSYM_NAME_LEN 512

multiplication factor is four


> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -213,7 +213,7 @@ static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
>  	 * we use the smallest/strictest upper bound possible (56, based on
>  	 * the current definition of MODULE_NAME_LEN) to prevent overflows.
>  	 */
> -	BUILD_BUG_ON(MODULE_NAME_LEN < 56 || KSYM_NAME_LEN != 128);
> +	BUILD_BUG_ON(MODULE_NAME_LEN < 56 || KSYM_NAME_LEN != 512);

factor four


> @@ -227,7 +227,7 @@ static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
>  
>  		/* Format: .klp.sym.sym_objname.sym_name,sympos */
>  		cnt = sscanf(strtab + sym->st_name,
> -			     ".klp.sym.%55[^.].%127[^,],%lu",
> +			     ".klp.sym.%55[^.].%511[^,],%lu",

4 * ( 127 + 1 )  =  511 + 1


> --- a/scripts/kallsyms.c
> +++ b/scripts/kallsyms.c
> @@ -30,10 +30,10 @@
>  #define _stringify_1(x)	#x
>  #define _stringify(x)	_stringify_1(x)
>  
> -#define KSYM_NAME_LEN		128
> +#define KSYM_NAME_LEN		512

factor four


> --- a/tools/include/linux/kallsyms.h
> +++ b/tools/include/linux/kallsyms.h
> @@ -6,7 +6,7 @@
>  #include <stdio.h>
>  #include <unistd.h>
>  
> -#define KSYM_NAME_LEN 128
> +#define KSYM_NAME_LEN 512

factor four


> --- a/tools/lib/perf/include/perf/event.h
> +++ b/tools/lib/perf/include/perf/event.h
> @@ -95,7 +95,7 @@ struct perf_record_throttle {
>  };
>  
>  #ifndef KSYM_NAME_LEN
> -#define KSYM_NAME_LEN 256
> +#define KSYM_NAME_LEN 512

Here is the multiplication factor  two.


> --- a/tools/lib/symbol/kallsyms.h
> +++ b/tools/lib/symbol/kallsyms.h
> @@ -7,7 +7,7 @@
>  #include <linux/types.h>
>  
>  #ifndef KSYM_NAME_LEN
> -#define KSYM_NAME_LEN 256
> +#define KSYM_NAME_LEN 512

Another  "factor two"



It feels good to unify all the KSYM_NAME_LEN to 512.

Thing that feels less good is doubling 256 versus quadrupling 128.

I felt the need to report that.


Feel free to ignore this remark.



Groeten
Geert Stappers
-- 
Silence is hard to parse

Re: [PATCH v1 1/3] kallsyms: avoid hardcoding the buffer size

[James Bottomley]

On Thu, 2022-05-05 at 21:16 +0200, Miguel Ojeda wrote:
> From: Boqun Feng <boqun.feng@gmail.com>
> 
> This makes it easier to update the size later on.
> 
> Furthermore, a static assert is added to ensure both are updated
> when that happens. The relationship used is one that keeps the new
> size (512+1) close to the original buffer size (500).
> 
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> ---
>  scripts/kallsyms.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
> index 8caabddf817c..880c4404731b 100644
> --- a/scripts/kallsyms.c
> +++ b/scripts/kallsyms.c
> @@ -27,8 +27,18 @@
>  
>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr[0]))
>  
> +#define _stringify_1(x)	#x
> +#define _stringify(x)	_stringify_1(x)
> +
>  #define KSYM_NAME_LEN		128
>  
> +/* A substantially bigger size than the current maximum. */
> +#define KSYM_NAME_LEN_BUFFER	512
> +_Static_assert(
> +	KSYM_NAME_LEN_BUFFER == KSYM_NAME_LEN * 4,
> +	"Please keep KSYM_NAME_LEN_BUFFER in sync with KSYM_NAME_LEN"
> +);
> +
>  struct sym_entry {
>  	unsigned long long addr;
>  	unsigned int len;
> @@ -197,15 +207,15 @@ static void check_symbol_range(const char *sym,
> unsigned long long addr,
>  
>  static struct sym_entry *read_symbol(FILE *in)
>  {
> -	char name[500], type;
> +	char name[KSYM_NAME_LEN_BUFFER+1], type;

When you raise KSYM_NAME_LEN to 512, this on stack allocation becomes
2049 bytes.  How did you manage not to trigger the frame size warning,
which is 1024 on 32 bit and 2048 on 64 bit by default?

James

Re: [PATCH v1 1/3] kallsyms: avoid hardcoding the buffer size

[Miguel Ojeda]

Hi James,

On Fri, May 6, 2022 at 1:19 PM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> When you raise KSYM_NAME_LEN to 512, this on stack allocation becomes
> 2049 bytes.  How did you manage not to trigger the frame size warning,
> which is 1024 on 32 bit and 2048 on 64 bit by default?

Thanks for taking a look!

If you mean `CONFIG_FRAME_WARN`, that applies to kernel objects, not scripts.

If I enable it for `HOSTCC` with a 2048 threshold, we do get the warning:

    scripts/kallsyms.c: In function 'read_symbol':
    scripts/kallsyms.c:257:1: warning: the frame size of 2080 bytes is
larger than 2048 bytes [-Wframe-larger-than=]
     257 | }
          | ^

as well as in a few other places (e.g. for my config [0]), but given
it is userspace it should be fine unless they happen to end up
recursing quite a few times.

Cheers,
Miguel

[0]
    scripts/mod/modpost.c: In function 'main':
    scripts/mod/modpost.c:2636:1: warning: the frame size of 4208
bytes is larger than 2048 bytes [-Wframe-larger-than=]
     2636 | }
          | ^
    scripts/mod/sumversion.c: In function 'get_src_version':
    scripts/mod/sumversion.c:419:1: warning: the frame size of 4272
bytes is larger than 2048 bytes [-Wframe-larger-than=]
      419 | }
          | ^
    usr/gen_init_cpio.c: In function 'cpio_mkgeneric_line':
    usr/gen_init_cpio.c:223:1: warning: the frame size of 4384 bytes
is larger than 2048 bytes [-Wframe-larger-than=]
      223 | }
          | ^
    usr/gen_init_cpio.c: In function 'cpio_mknod_line':
    usr/gen_init_cpio.c:293:1: warning: the frame size of 4400 bytes
is larger than 2048 bytes [-Wframe-larger-than=]
      293 | }
          | ^
    usr/gen_init_cpio.c: In function 'cpio_mkfile_line':
    usr/gen_init_cpio.c:456:1: warning: the frame size of 12560 bytes
is larger than 2048 bytes [-Wframe-larger-than=]
      456 | }
          | ^
    usr/gen_init_cpio.c: In function 'cpio_mkslink_line':
    usr/gen_init_cpio.c:150:1: warning: the frame size of 8496 bytes
is larger than 2048 bytes [-Wframe-larger-than=]
      150 | }
          | ^
    usr/gen_init_cpio.c: In function 'main':
    usr/gen_init_cpio.c:640:1: warning: the frame size of 8528 bytes
is larger than 2048 bytes [-Wframe-larger-than=]
      640 | }
          | ^
    drivers/tty/vt/conmakehash.c: In function 'main':
    drivers/tty/vt/conmakehash.c:290:1: warning: the frame size of
65584 bytes is larger than 2048 bytes [-Wframe-larger-than=]
      290 | }
          | ^

Re: [PATCH v1 1/3] kallsyms: avoid hardcoding the buffer size

[James Bottomley]

On Fri, 2022-05-06 at 14:58 +0200, Miguel Ojeda wrote:
> Hi James,
> 
> On Fri, May 6, 2022 at 1:19 PM James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > When you raise KSYM_NAME_LEN to 512, this on stack allocation
> > becomes 2049 bytes.  How did you manage not to trigger the frame
> > size warning, which is 1024 on 32 bit and 2048 on 64 bit by
> > default?
> 
> Thanks for taking a look!
> 
> If you mean `CONFIG_FRAME_WARN`, that applies to kernel objects, not
> scripts.

Oh, right, I missed that it was a script.  Forget the comment then; we
only have a restricted stack inside the kernel not in userspace.

James