[PATCH v4 0/2] riscv: Add remaining module relocations and tests

linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed

* [PATCH v4 0/2] riscv: Add remaining module relocations and tests
@ 2023-10-18  5:34 Charlie Jenkins
  2023-10-18  5:34 ` [PATCH v4 1/2] riscv: Add remaining module relocations Charlie Jenkins
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Charlie Jenkins @ 2023-10-18  5:34 UTC (permalink / raw)
  To: linux-riscv, linux-mm, linux-kernel
  Cc: Eric Biederman, Kees Cook, Paul Walmsley, Palmer Dabbelt,
	Albert Ou, Charlie Jenkins

A handful of module relocations were missing, this patch includes the
remaining ones. I also wrote some test cases to ensure that module
loading works properly. Some relocations cannot be supported in the
kernel, these include the ones that rely on thread local storage and
dynamic linking.

ULEB128 handling is a bit special because SET and SUB relocations must
happen together, and SET must happen before SUB. A psABI proposal [1]
mandates that the first SET_ULEB128 that appears before a SUB_ULEB128
is the associated SET_ULEB128.

This can be tested by enabling KUNIT, RUNTIME_KERNEL_TESTING_MENU, and
RISCV_MODULE_LINKING_KUNIT.

[1] https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/403

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
Changes in v4:
- Complete removal of R_RISCV_RVC_LUI
- Fix bug in R_RISCV_SUB6 linking
- Only build ULEB128 tests if supported by toolchain
- Link to v3: https://lore.kernel.org/r/20231016-module_relocations-v3-0-a667fd6071e9@rivosinc.com

Changes in v3:
- Add prototypes to test_module_linking_main as recommended by intel
  zero day bot
- Improve efficiency of ULEB128 pair matching
- Link to v2: https://lore.kernel.org/r/20231006-module_relocations-v2-0-47566453fedc@rivosinc.com

Changes in v2:
- Added ULEB128 relocations
- Link to v1: https://lore.kernel.org/r/20230913-module_relocations-v1-0-bb3d8467e793@rivosinc.com

---
Charlie Jenkins (2):
      riscv: Add remaining module relocations
      riscv: Add tests for riscv module loading

 arch/riscv/Kconfig.debug                           |   1 +
 arch/riscv/include/uapi/asm/elf.h                  |   5 +-
 arch/riscv/kernel/Makefile                         |   1 +
 arch/riscv/kernel/module.c                         | 207 ++++++++++++++++++---
 arch/riscv/kernel/tests/Kconfig.debug              |  35 ++++
 arch/riscv/kernel/tests/Makefile                   |   1 +
 arch/riscv/kernel/tests/module_test/Makefile       |  15 ++
 .../tests/module_test/test_module_linking_main.c   |  85 +++++++++
 arch/riscv/kernel/tests/module_test/test_set16.S   |  23 +++
 arch/riscv/kernel/tests/module_test/test_set32.S   |  20 ++
 arch/riscv/kernel/tests/module_test/test_set6.S    |  23 +++
 arch/riscv/kernel/tests/module_test/test_set8.S    |  23 +++
 arch/riscv/kernel/tests/module_test/test_sub16.S   |  22 +++
 arch/riscv/kernel/tests/module_test/test_sub32.S   |  22 +++
 arch/riscv/kernel/tests/module_test/test_sub6.S    |  22 +++
 arch/riscv/kernel/tests/module_test/test_sub64.S   |  27 +++
 arch/riscv/kernel/tests/module_test/test_sub8.S    |  22 +++
 arch/riscv/kernel/tests/module_test/test_uleb128.S |  20 ++
 18 files changed, 548 insertions(+), 26 deletions(-)
---
base-commit: 4d320c2d9a2b22f53523a1b012cda17a50220965
change-id: 20230908-module_relocations-f63ced651bd7
-- 
- Charlie



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

* [PATCH v4 1/2] riscv: Add remaining module relocations
  2023-10-18  5:34 [PATCH v4 0/2] riscv: Add remaining module relocations and tests Charlie Jenkins
@ 2023-10-18  5:34 ` Charlie Jenkins
  2023-10-18 12:17   ` Emil Renner Berthing
                     ` (2 more replies)
  2023-10-18  5:34 ` [PATCH v4 2/2] riscv: Add tests for riscv module loading Charlie Jenkins
  2023-10-18 11:35 ` [PATCH v4 0/2] riscv: Add remaining module relocations and tests Conor Dooley
  2 siblings, 3 replies; 16+ messages in thread
From: Charlie Jenkins @ 2023-10-18  5:34 UTC (permalink / raw)
  To: linux-riscv, linux-mm, linux-kernel
  Cc: Eric Biederman, Kees Cook, Paul Walmsley, Palmer Dabbelt,
	Albert Ou, Charlie Jenkins

Add all final module relocations and add error logs explaining the ones
that are not supported.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
 arch/riscv/include/uapi/asm/elf.h |   5 +-
 arch/riscv/kernel/module.c        | 207 +++++++++++++++++++++++++++++++++-----
 2 files changed, 186 insertions(+), 26 deletions(-)

diff --git a/arch/riscv/include/uapi/asm/elf.h b/arch/riscv/include/uapi/asm/elf.h
index d696d6610231..11a71b8533d5 100644
--- a/arch/riscv/include/uapi/asm/elf.h
+++ b/arch/riscv/include/uapi/asm/elf.h
@@ -49,6 +49,7 @@ typedef union __riscv_fp_state elf_fpregset_t;
 #define R_RISCV_TLS_DTPREL64	9
 #define R_RISCV_TLS_TPREL32	10
 #define R_RISCV_TLS_TPREL64	11
+#define R_RISCV_IRELATIVE	58
 
 /* Relocation types not used by the dynamic linker */
 #define R_RISCV_BRANCH		16
@@ -81,7 +82,6 @@ typedef union __riscv_fp_state elf_fpregset_t;
 #define R_RISCV_ALIGN		43
 #define R_RISCV_RVC_BRANCH	44
 #define R_RISCV_RVC_JUMP	45
-#define R_RISCV_LUI		46
 #define R_RISCV_GPREL_I		47
 #define R_RISCV_GPREL_S		48
 #define R_RISCV_TPREL_I		49
@@ -93,6 +93,9 @@ typedef union __riscv_fp_state elf_fpregset_t;
 #define R_RISCV_SET16		55
 #define R_RISCV_SET32		56
 #define R_RISCV_32_PCREL	57
+#define R_RISCV_PLT32		59
+#define R_RISCV_SET_ULEB128	60
+#define R_RISCV_SUB_ULEB128	61
 
 
 #endif /* _UAPI_ASM_RISCV_ELF_H */
diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
index 7c651d55fcbd..e860726352ac 100644
--- a/arch/riscv/kernel/module.c
+++ b/arch/riscv/kernel/module.c
@@ -7,6 +7,7 @@
 #include <linux/elf.h>
 #include <linux/err.h>
 #include <linux/errno.h>
+#include <linux/kernel.h>
 #include <linux/moduleloader.h>
 #include <linux/vmalloc.h>
 #include <linux/sizes.h>
@@ -268,6 +269,12 @@ static int apply_r_riscv_align_rela(struct module *me, u32 *location,
 	return -EINVAL;
 }
 
+static int apply_r_riscv_add8_rela(struct module *me, u32 *location, Elf_Addr v)
+{
+	*(u8 *)location += (u8)v;
+	return 0;
+}
+
 static int apply_r_riscv_add16_rela(struct module *me, u32 *location,
 				    Elf_Addr v)
 {
@@ -289,6 +296,12 @@ static int apply_r_riscv_add64_rela(struct module *me, u32 *location,
 	return 0;
 }
 
+static int apply_r_riscv_sub8_rela(struct module *me, u32 *location, Elf_Addr v)
+{
+	*(u8 *)location -= (u8)v;
+	return 0;
+}
+
 static int apply_r_riscv_sub16_rela(struct module *me, u32 *location,
 				    Elf_Addr v)
 {
@@ -310,31 +323,149 @@ static int apply_r_riscv_sub64_rela(struct module *me, u32 *location,
 	return 0;
 }
 
-static int (*reloc_handlers_rela[]) (struct module *me, u32 *location,
-				Elf_Addr v) = {
-	[R_RISCV_32]			= apply_r_riscv_32_rela,
-	[R_RISCV_64]			= apply_r_riscv_64_rela,
-	[R_RISCV_BRANCH]		= apply_r_riscv_branch_rela,
-	[R_RISCV_JAL]			= apply_r_riscv_jal_rela,
-	[R_RISCV_RVC_BRANCH]		= apply_r_riscv_rvc_branch_rela,
-	[R_RISCV_RVC_JUMP]		= apply_r_riscv_rvc_jump_rela,
-	[R_RISCV_PCREL_HI20]		= apply_r_riscv_pcrel_hi20_rela,
-	[R_RISCV_PCREL_LO12_I]		= apply_r_riscv_pcrel_lo12_i_rela,
-	[R_RISCV_PCREL_LO12_S]		= apply_r_riscv_pcrel_lo12_s_rela,
-	[R_RISCV_HI20]			= apply_r_riscv_hi20_rela,
-	[R_RISCV_LO12_I]		= apply_r_riscv_lo12_i_rela,
-	[R_RISCV_LO12_S]		= apply_r_riscv_lo12_s_rela,
-	[R_RISCV_GOT_HI20]		= apply_r_riscv_got_hi20_rela,
-	[R_RISCV_CALL_PLT]		= apply_r_riscv_call_plt_rela,
-	[R_RISCV_CALL]			= apply_r_riscv_call_rela,
-	[R_RISCV_RELAX]			= apply_r_riscv_relax_rela,
-	[R_RISCV_ALIGN]			= apply_r_riscv_align_rela,
-	[R_RISCV_ADD16]			= apply_r_riscv_add16_rela,
-	[R_RISCV_ADD32]			= apply_r_riscv_add32_rela,
-	[R_RISCV_ADD64]			= apply_r_riscv_add64_rela,
-	[R_RISCV_SUB16]			= apply_r_riscv_sub16_rela,
-	[R_RISCV_SUB32]			= apply_r_riscv_sub32_rela,
-	[R_RISCV_SUB64]			= apply_r_riscv_sub64_rela,
+static int dynamic_linking_not_supported(struct module *me, u32 *location,
+					 Elf_Addr v)
+{
+	pr_err("%s: Dynamic linking not supported in kernel modules PC = %p\n",
+	       me->name, location);
+	return -EINVAL;
+}
+
+static int tls_not_supported(struct module *me, u32 *location, Elf_Addr v)
+{
+	pr_err("%s: Thread local storage not supported in kernel modules PC = %p\n",
+	       me->name, location);
+	return -EINVAL;
+}
+
+static int apply_r_riscv_sub6_rela(struct module *me, u32 *location, Elf_Addr v)
+{
+	*(u8 *)location = (*location - ((u8)v & 0x3F)) & 0x3F;
+	return 0;
+}
+
+static int apply_r_riscv_set6_rela(struct module *me, u32 *location, Elf_Addr v)
+{
+	*(u8 *)location = (*(u8 *)location & 0xc0) | ((u8)v & 0x3F);
+	return 0;
+}
+
+static int apply_r_riscv_set8_rela(struct module *me, u32 *location, Elf_Addr v)
+{
+	*(u8 *)location = (u8)v;
+	return 0;
+}
+
+static int apply_r_riscv_set16_rela(struct module *me, u32 *location,
+				    Elf_Addr v)
+{
+	*(u16 *)location = (u16)v;
+	return 0;
+}
+
+static int apply_r_riscv_set32_rela(struct module *me, u32 *location,
+				    Elf_Addr v)
+{
+	*(u32 *)location = (u32)v;
+	return 0;
+}
+
+static int apply_r_riscv_32_pcrel_rela(struct module *me, u32 *location,
+				       Elf_Addr v)
+{
+	*(u32 *)location = (u32)v;
+	return 0;
+}
+
+static int apply_r_riscv_plt32_rela(struct module *me, u32 *location,
+				    Elf_Addr v)
+{
+	*(u32 *)location = (u32)v;
+	return 0;
+}
+
+static int apply_r_riscv_set_uleb128(struct module *me, u32 *location, Elf_Addr v)
+{
+	/*
+	 * Relocation is only performed if R_RISCV_SET_ULEB128 is followed by
+	 * R_RISCV_SUB_ULEB128 so do computation there
+	 */
+	return 0;
+}
+
+static int apply_r_riscv_sub_uleb128(struct module *me, u32 *location, Elf_Addr v)
+{
+	if (v >= 128) {
+		pr_err("%s: uleb128 must be in [0, 127] (not %ld) at PC = %p\n",
+		       me->name, (unsigned long)v, location);
+		return -EINVAL;
+	}
+
+	*location = v;
+	return 0;
+}
+
+/*
+ * Relocations defined in the riscv-elf-psabi-doc.
+ * This handles static linking only.
+ */
+static int (*reloc_handlers_rela[])(struct module *me, u32 *location,
+				    Elf_Addr v) = {
+	[R_RISCV_32] =			apply_r_riscv_32_rela,
+	[R_RISCV_64] =			apply_r_riscv_64_rela,
+	[R_RISCV_RELATIVE] =		dynamic_linking_not_supported,
+	[R_RISCV_COPY] =		dynamic_linking_not_supported,
+	[R_RISCV_JUMP_SLOT] =		dynamic_linking_not_supported,
+	[R_RISCV_TLS_DTPMOD32] =	dynamic_linking_not_supported,
+	[R_RISCV_TLS_DTPMOD64] =	dynamic_linking_not_supported,
+	[R_RISCV_TLS_DTPREL32] =	dynamic_linking_not_supported,
+	[R_RISCV_TLS_DTPREL64] =	dynamic_linking_not_supported,
+	[R_RISCV_TLS_TPREL32] =		dynamic_linking_not_supported,
+	[R_RISCV_TLS_TPREL64] =		dynamic_linking_not_supported,
+	/* 12-15 undefined */
+	[R_RISCV_BRANCH] =		apply_r_riscv_branch_rela,
+	[R_RISCV_JAL] =			apply_r_riscv_jal_rela,
+	[R_RISCV_CALL] =		apply_r_riscv_call_rela,
+	[R_RISCV_CALL_PLT] =		apply_r_riscv_call_plt_rela,
+	[R_RISCV_GOT_HI20] =		apply_r_riscv_got_hi20_rela,
+	[R_RISCV_TLS_GOT_HI20] =	tls_not_supported,
+	[R_RISCV_TLS_GD_HI20] =		tls_not_supported,
+	[R_RISCV_PCREL_HI20] =		apply_r_riscv_pcrel_hi20_rela,
+	[R_RISCV_PCREL_LO12_I] =	apply_r_riscv_pcrel_lo12_i_rela,
+	[R_RISCV_PCREL_LO12_S] =	apply_r_riscv_pcrel_lo12_s_rela,
+	[R_RISCV_HI20] =		apply_r_riscv_hi20_rela,
+	[R_RISCV_LO12_I] =		apply_r_riscv_lo12_i_rela,
+	[R_RISCV_LO12_S] =		apply_r_riscv_lo12_s_rela,
+	[R_RISCV_TPREL_HI20] =		tls_not_supported,
+	[R_RISCV_TPREL_LO12_I] =	tls_not_supported,
+	[R_RISCV_TPREL_LO12_S] =	tls_not_supported,
+	[R_RISCV_TPREL_ADD] =		tls_not_supported,
+	[R_RISCV_ADD8] =		apply_r_riscv_add8_rela,
+	[R_RISCV_ADD16] =		apply_r_riscv_add16_rela,
+	[R_RISCV_ADD32] =		apply_r_riscv_add32_rela,
+	[R_RISCV_ADD64] =		apply_r_riscv_add64_rela,
+	[R_RISCV_SUB8] =		apply_r_riscv_sub8_rela,
+	[R_RISCV_SUB16] =		apply_r_riscv_sub16_rela,
+	[R_RISCV_SUB32] =		apply_r_riscv_sub32_rela,
+	[R_RISCV_SUB64] =		apply_r_riscv_sub64_rela,
+	/* 41-42 reserved for future standard use */
+	[R_RISCV_ALIGN] =		apply_r_riscv_align_rela,
+	[R_RISCV_RVC_BRANCH] =		apply_r_riscv_rvc_branch_rela,
+	[R_RISCV_RVC_JUMP] =		apply_r_riscv_rvc_jump_rela,
+	/* 46-50 reserved for future standard use */
+	[R_RISCV_RELAX] =		apply_r_riscv_relax_rela,
+	[R_RISCV_SUB6] =		apply_r_riscv_sub6_rela,
+	[R_RISCV_SET6] =		apply_r_riscv_set6_rela,
+	[R_RISCV_SET8] =		apply_r_riscv_set8_rela,
+	[R_RISCV_SET16] =		apply_r_riscv_set16_rela,
+	[R_RISCV_SET32] =		apply_r_riscv_set32_rela,
+	[R_RISCV_32_PCREL] =		apply_r_riscv_32_pcrel_rela,
+	[R_RISCV_IRELATIVE] =		dynamic_linking_not_supported,
+	[R_RISCV_PLT32] =		apply_r_riscv_plt32_rela,
+	[R_RISCV_SET_ULEB128] =		apply_r_riscv_set_uleb128,
+	[R_RISCV_SUB_ULEB128] =		apply_r_riscv_sub_uleb128,
+	/* 62-191 reserved for future standard use */
+	/* 192-255 nonstandard ABI extensions  */
 };
 
 int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
@@ -348,6 +479,10 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
 	unsigned int i, type;
 	Elf_Addr v;
 	int res;
+	bool uleb128_set_exists = false;
+	u32 *uleb128_set_loc;
+	unsigned long uleb128_set_sym_val;
+
 
 	pr_debug("Applying relocate section %u to %u\n", relsec,
 	       sechdrs[relsec].sh_info);
@@ -425,6 +560,28 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
 				  me->name);
 				return -EINVAL;
 			}
+		} else if (type == R_RISCV_SET_ULEB128) {
+			if (uleb128_set_exists) {
+				pr_err("%s: riscv psABI requires the next ULEB128 relocation to come after a R_RISCV_SET_ULEB128 is an R_RISCV_SUB_ULEB128, not another R_RISCV_SET_ULEB128.\n",
+				       me->name);
+				return -EINVAL;
+			}
+			uleb128_set_exists = true;
+			uleb128_set_loc = location;
+			uleb128_set_sym_val =
+				((Elf_Sym *)sechdrs[symindex].sh_addr +
+					ELF_RISCV_R_SYM(rel[i].r_info))
+					->st_value +
+				rel[i].r_addend;
+		} else if (type == R_RISCV_SUB_ULEB128) {
+			if (uleb128_set_exists && uleb128_set_loc == location) {
+				/* Calculate set and subtraction */
+				v = uleb128_set_sym_val - v;
+			} else {
+				pr_err("%s: R_RISCV_SUB_ULEB128 must always be paired with the first R_RISCV_SET_ULEB128 that comes before it. PC = %p\n",
+				       me->name, location);
+				return -EINVAL;
+			}
 		}
 
 		res = handler(me, location, v);

-- 
2.34.1



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

* Re: [PATCH v4 1/2] riscv: Add remaining module relocations
  2023-10-18  5:34 ` [PATCH v4 1/2] riscv: Add remaining module relocations Charlie Jenkins
@ 2023-10-18 12:17   ` Emil Renner Berthing
  2023-10-18 18:31     ` Charlie Jenkins
  2023-10-18 18:28   ` Samuel Holland
  2023-10-18 18:47   ` Andreas Schwab
  2 siblings, 1 reply; 16+ messages in thread
From: Emil Renner Berthing @ 2023-10-18 12:17 UTC (permalink / raw)
  To: Charlie Jenkins, linux-riscv, linux-mm, linux-kernel
  Cc: Eric Biederman, Kees Cook, Paul Walmsley, Palmer Dabbelt, Albert Ou

Charlie Jenkins wrote:
> Add all final module relocations and add error logs explaining the ones
> that are not supported.
>
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
>  arch/riscv/include/uapi/asm/elf.h |   5 +-
>  arch/riscv/kernel/module.c        | 207 +++++++++++++++++++++++++++++++++-----
>  2 files changed, 186 insertions(+), 26 deletions(-)
>
> diff --git a/arch/riscv/include/uapi/asm/elf.h b/arch/riscv/include/uapi/asm/elf.h
> index d696d6610231..11a71b8533d5 100644
> --- a/arch/riscv/include/uapi/asm/elf.h
> +++ b/arch/riscv/include/uapi/asm/elf.h
> @@ -49,6 +49,7 @@ typedef union __riscv_fp_state elf_fpregset_t;
>  #define R_RISCV_TLS_DTPREL64	9
>  #define R_RISCV_TLS_TPREL32	10
>  #define R_RISCV_TLS_TPREL64	11
> +#define R_RISCV_IRELATIVE	58
>
>  /* Relocation types not used by the dynamic linker */
>  #define R_RISCV_BRANCH		16
> @@ -81,7 +82,6 @@ typedef union __riscv_fp_state elf_fpregset_t;
>  #define R_RISCV_ALIGN		43
>  #define R_RISCV_RVC_BRANCH	44
>  #define R_RISCV_RVC_JUMP	45
> -#define R_RISCV_LUI		46
>  #define R_RISCV_GPREL_I		47
>  #define R_RISCV_GPREL_S		48
>  #define R_RISCV_TPREL_I		49
> @@ -93,6 +93,9 @@ typedef union __riscv_fp_state elf_fpregset_t;
>  #define R_RISCV_SET16		55
>  #define R_RISCV_SET32		56
>  #define R_RISCV_32_PCREL	57
> +#define R_RISCV_PLT32		59
> +#define R_RISCV_SET_ULEB128	60
> +#define R_RISCV_SUB_ULEB128	61
>
>
>  #endif /* _UAPI_ASM_RISCV_ELF_H */
> diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> index 7c651d55fcbd..e860726352ac 100644
> --- a/arch/riscv/kernel/module.c
> +++ b/arch/riscv/kernel/module.c
> @@ -7,6 +7,7 @@
>  #include <linux/elf.h>
>  #include <linux/err.h>
>  #include <linux/errno.h>
> +#include <linux/kernel.h>
>  #include <linux/moduleloader.h>
>  #include <linux/vmalloc.h>
>  #include <linux/sizes.h>
> @@ -268,6 +269,12 @@ static int apply_r_riscv_align_rela(struct module *me, u32 *location,
>  	return -EINVAL;
>  }
>
> +static int apply_r_riscv_add8_rela(struct module *me, u32 *location, Elf_Addr v)
> +{
> +	*(u8 *)location += (u8)v;
> +	return 0;
> +}
> +
>  static int apply_r_riscv_add16_rela(struct module *me, u32 *location,
>  				    Elf_Addr v)
>  {
> @@ -289,6 +296,12 @@ static int apply_r_riscv_add64_rela(struct module *me, u32 *location,
>  	return 0;
>  }
>
> +static int apply_r_riscv_sub8_rela(struct module *me, u32 *location, Elf_Addr v)
> +{
> +	*(u8 *)location -= (u8)v;
> +	return 0;
> +}
> +
>  static int apply_r_riscv_sub16_rela(struct module *me, u32 *location,
>  				    Elf_Addr v)
>  {
> @@ -310,31 +323,149 @@ static int apply_r_riscv_sub64_rela(struct module *me, u32 *location,
>  	return 0;
>  }
>
> -static int (*reloc_handlers_rela[]) (struct module *me, u32 *location,
> -				Elf_Addr v) = {
> -	[R_RISCV_32]			= apply_r_riscv_32_rela,
> -	[R_RISCV_64]			= apply_r_riscv_64_rela,
> -	[R_RISCV_BRANCH]		= apply_r_riscv_branch_rela,
> -	[R_RISCV_JAL]			= apply_r_riscv_jal_rela,
> -	[R_RISCV_RVC_BRANCH]		= apply_r_riscv_rvc_branch_rela,
> -	[R_RISCV_RVC_JUMP]		= apply_r_riscv_rvc_jump_rela,
> -	[R_RISCV_PCREL_HI20]		= apply_r_riscv_pcrel_hi20_rela,
> -	[R_RISCV_PCREL_LO12_I]		= apply_r_riscv_pcrel_lo12_i_rela,
> -	[R_RISCV_PCREL_LO12_S]		= apply_r_riscv_pcrel_lo12_s_rela,
> -	[R_RISCV_HI20]			= apply_r_riscv_hi20_rela,
> -	[R_RISCV_LO12_I]		= apply_r_riscv_lo12_i_rela,
> -	[R_RISCV_LO12_S]		= apply_r_riscv_lo12_s_rela,
> -	[R_RISCV_GOT_HI20]		= apply_r_riscv_got_hi20_rela,
> -	[R_RISCV_CALL_PLT]		= apply_r_riscv_call_plt_rela,
> -	[R_RISCV_CALL]			= apply_r_riscv_call_rela,
> -	[R_RISCV_RELAX]			= apply_r_riscv_relax_rela,
> -	[R_RISCV_ALIGN]			= apply_r_riscv_align_rela,
> -	[R_RISCV_ADD16]			= apply_r_riscv_add16_rela,
> -	[R_RISCV_ADD32]			= apply_r_riscv_add32_rela,
> -	[R_RISCV_ADD64]			= apply_r_riscv_add64_rela,
> -	[R_RISCV_SUB16]			= apply_r_riscv_sub16_rela,
> -	[R_RISCV_SUB32]			= apply_r_riscv_sub32_rela,
> -	[R_RISCV_SUB64]			= apply_r_riscv_sub64_rela,
> +static int dynamic_linking_not_supported(struct module *me, u32 *location,
> +					 Elf_Addr v)
> +{
> +	pr_err("%s: Dynamic linking not supported in kernel modules PC = %p\n",
> +	       me->name, location);
> +	return -EINVAL;
> +}
> +
> +static int tls_not_supported(struct module *me, u32 *location, Elf_Addr v)
> +{
> +	pr_err("%s: Thread local storage not supported in kernel modules PC = %p\n",
> +	       me->name, location);
> +	return -EINVAL;
> +}
> +
> +static int apply_r_riscv_sub6_rela(struct module *me, u32 *location, Elf_Addr v)
> +{
> +	*(u8 *)location = (*location - ((u8)v & 0x3F)) & 0x3F;
> +	return 0;
> +}
> +
> +static int apply_r_riscv_set6_rela(struct module *me, u32 *location, Elf_Addr v)
> +{
> +	*(u8 *)location = (*(u8 *)location & 0xc0) | ((u8)v & 0x3F);
> +	return 0;
> +}
> +
> +static int apply_r_riscv_set8_rela(struct module *me, u32 *location, Elf_Addr v)
> +{
> +	*(u8 *)location = (u8)v;
> +	return 0;
> +}
> +
> +static int apply_r_riscv_set16_rela(struct module *me, u32 *location,
> +				    Elf_Addr v)
> +{
> +	*(u16 *)location = (u16)v;
> +	return 0;
> +}
> +
> +static int apply_r_riscv_set32_rela(struct module *me, u32 *location,
> +				    Elf_Addr v)
> +{
> +	*(u32 *)location = (u32)v;
> +	return 0;
> +}
> +
> +static int apply_r_riscv_32_pcrel_rela(struct module *me, u32 *location,
> +				       Elf_Addr v)
> +{
> +	*(u32 *)location = (u32)v;
> +	return 0;
> +}
> +
> +static int apply_r_riscv_plt32_rela(struct module *me, u32 *location,
> +				    Elf_Addr v)
> +{
> +	*(u32 *)location = (u32)v;
> +	return 0;
> +}
> +
> +static int apply_r_riscv_set_uleb128(struct module *me, u32 *location, Elf_Addr v)
> +{
> +	/*
> +	 * Relocation is only performed if R_RISCV_SET_ULEB128 is followed by
> +	 * R_RISCV_SUB_ULEB128 so do computation there
> +	 */
> +	return 0;
> +}
> +
> +static int apply_r_riscv_sub_uleb128(struct module *me, u32 *location, Elf_Addr v)
> +{
> +	if (v >= 128) {
> +		pr_err("%s: uleb128 must be in [0, 127] (not %ld) at PC = %p\n",
> +		       me->name, (unsigned long)v, location);
> +		return -EINVAL;
> +	}
> +
> +	*location = v;
> +	return 0;
> +}
> +
> +/*
> + * Relocations defined in the riscv-elf-psabi-doc.
> + * This handles static linking only.
> + */
> +static int (*reloc_handlers_rela[])(struct module *me, u32 *location,
> +				    Elf_Addr v) = {
> +	[R_RISCV_32] =			apply_r_riscv_32_rela,
> +	[R_RISCV_64] =			apply_r_riscv_64_rela,
> +	[R_RISCV_RELATIVE] =		dynamic_linking_not_supported,
> +	[R_RISCV_COPY] =		dynamic_linking_not_supported,
> +	[R_RISCV_JUMP_SLOT] =		dynamic_linking_not_supported,
> +	[R_RISCV_TLS_DTPMOD32] =	dynamic_linking_not_supported,
> +	[R_RISCV_TLS_DTPMOD64] =	dynamic_linking_not_supported,
> +	[R_RISCV_TLS_DTPREL32] =	dynamic_linking_not_supported,
> +	[R_RISCV_TLS_DTPREL64] =	dynamic_linking_not_supported,
> +	[R_RISCV_TLS_TPREL32] =		dynamic_linking_not_supported,
> +	[R_RISCV_TLS_TPREL64] =		dynamic_linking_not_supported,
> +	/* 12-15 undefined */
> +	[R_RISCV_BRANCH] =		apply_r_riscv_branch_rela,
> +	[R_RISCV_JAL] =			apply_r_riscv_jal_rela,
> +	[R_RISCV_CALL] =		apply_r_riscv_call_rela,
> +	[R_RISCV_CALL_PLT] =		apply_r_riscv_call_plt_rela,
> +	[R_RISCV_GOT_HI20] =		apply_r_riscv_got_hi20_rela,
> +	[R_RISCV_TLS_GOT_HI20] =	tls_not_supported,
> +	[R_RISCV_TLS_GD_HI20] =		tls_not_supported,
> +	[R_RISCV_PCREL_HI20] =		apply_r_riscv_pcrel_hi20_rela,
> +	[R_RISCV_PCREL_LO12_I] =	apply_r_riscv_pcrel_lo12_i_rela,
> +	[R_RISCV_PCREL_LO12_S] =	apply_r_riscv_pcrel_lo12_s_rela,
> +	[R_RISCV_HI20] =		apply_r_riscv_hi20_rela,
> +	[R_RISCV_LO12_I] =		apply_r_riscv_lo12_i_rela,
> +	[R_RISCV_LO12_S] =		apply_r_riscv_lo12_s_rela,
> +	[R_RISCV_TPREL_HI20] =		tls_not_supported,
> +	[R_RISCV_TPREL_LO12_I] =	tls_not_supported,
> +	[R_RISCV_TPREL_LO12_S] =	tls_not_supported,
> +	[R_RISCV_TPREL_ADD] =		tls_not_supported,
> +	[R_RISCV_ADD8] =		apply_r_riscv_add8_rela,
> +	[R_RISCV_ADD16] =		apply_r_riscv_add16_rela,
> +	[R_RISCV_ADD32] =		apply_r_riscv_add32_rela,
> +	[R_RISCV_ADD64] =		apply_r_riscv_add64_rela,
> +	[R_RISCV_SUB8] =		apply_r_riscv_sub8_rela,
> +	[R_RISCV_SUB16] =		apply_r_riscv_sub16_rela,
> +	[R_RISCV_SUB32] =		apply_r_riscv_sub32_rela,
> +	[R_RISCV_SUB64] =		apply_r_riscv_sub64_rela,
> +	/* 41-42 reserved for future standard use */
> +	[R_RISCV_ALIGN] =		apply_r_riscv_align_rela,
> +	[R_RISCV_RVC_BRANCH] =		apply_r_riscv_rvc_branch_rela,
> +	[R_RISCV_RVC_JUMP] =		apply_r_riscv_rvc_jump_rela,
> +	/* 46-50 reserved for future standard use */
> +	[R_RISCV_RELAX] =		apply_r_riscv_relax_rela,
> +	[R_RISCV_SUB6] =		apply_r_riscv_sub6_rela,
> +	[R_RISCV_SET6] =		apply_r_riscv_set6_rela,
> +	[R_RISCV_SET8] =		apply_r_riscv_set8_rela,
> +	[R_RISCV_SET16] =		apply_r_riscv_set16_rela,
> +	[R_RISCV_SET32] =		apply_r_riscv_set32_rela,
> +	[R_RISCV_32_PCREL] =		apply_r_riscv_32_pcrel_rela,
> +	[R_RISCV_IRELATIVE] =		dynamic_linking_not_supported,
> +	[R_RISCV_PLT32] =		apply_r_riscv_plt32_rela,
> +	[R_RISCV_SET_ULEB128] =		apply_r_riscv_set_uleb128,
> +	[R_RISCV_SUB_ULEB128] =		apply_r_riscv_sub_uleb128,
> +	/* 62-191 reserved for future standard use */
> +	/* 192-255 nonstandard ABI extensions  */
>  };

Hi Charlie,

This is not a critique of this patch, but all these callbacks take a
u32 *location and
because of the compressed instructions this pointer may not be
aligned, so a lot of
the callbacks end up doing unaligned access which may fault to an
M-mode handler on
some platforms.

I once sent a patch to fix this:
https://lore.kernel.org/linux-riscv/20220224152456.493365-2-kernel@esmil.dk/

Maybe that's something you want to look into while touching this code anyway.

/Emil
>
>  int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> @@ -348,6 +479,10 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
>  	unsigned int i, type;
>  	Elf_Addr v;
>  	int res;
> +	bool uleb128_set_exists = false;
> +	u32 *uleb128_set_loc;
> +	unsigned long uleb128_set_sym_val;
> +
>
>  	pr_debug("Applying relocate section %u to %u\n", relsec,
>  	       sechdrs[relsec].sh_info);
> @@ -425,6 +560,28 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
>  				  me->name);
>  				return -EINVAL;
>  			}
> +		} else if (type == R_RISCV_SET_ULEB128) {
> +			if (uleb128_set_exists) {
> +				pr_err("%s: riscv psABI requires the next ULEB128 relocation to come after a R_RISCV_SET_ULEB128 is an R_RISCV_SUB_ULEB128, not another R_RISCV_SET_ULEB128.\n",
> +				       me->name);
> +				return -EINVAL;
> +			}
> +			uleb128_set_exists = true;
> +			uleb128_set_loc = location;
> +			uleb128_set_sym_val =
> +				((Elf_Sym *)sechdrs[symindex].sh_addr +
> +					ELF_RISCV_R_SYM(rel[i].r_info))
> +					->st_value +
> +				rel[i].r_addend;
> +		} else if (type == R_RISCV_SUB_ULEB128) {
> +			if (uleb128_set_exists && uleb128_set_loc == location) {
> +				/* Calculate set and subtraction */
> +				v = uleb128_set_sym_val - v;
> +			} else {
> +				pr_err("%s: R_RISCV_SUB_ULEB128 must always be paired with the first R_RISCV_SET_ULEB128 that comes before it. PC = %p\n",
> +				       me->name, location);
> +				return -EINVAL;
> +			}
>  		}
>
>  		res = handler(me, location, v);
>
> --
> 2.34.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv


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

* Re: [PATCH v4 1/2] riscv: Add remaining module relocations
  2023-10-18 12:17   ` Emil Renner Berthing
@ 2023-10-18 18:31     ` Charlie Jenkins
  2023-10-18 18:38       ` Emil Renner Berthing
  0 siblings, 1 reply; 16+ messages in thread
From: Charlie Jenkins @ 2023-10-18 18:31 UTC (permalink / raw)
  To: Emil Renner Berthing
  Cc: linux-riscv, linux-mm, linux-kernel, Eric Biederman, Kees Cook,
	Paul Walmsley, Palmer Dabbelt, Albert Ou

On Wed, Oct 18, 2023 at 05:17:44AM -0700, Emil Renner Berthing wrote:
> Charlie Jenkins wrote:
> > Add all final module relocations and add error logs explaining the ones
> > that are not supported.
> >
> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > ---
> >  arch/riscv/include/uapi/asm/elf.h |   5 +-
> >  arch/riscv/kernel/module.c        | 207 +++++++++++++++++++++++++++++++++-----
> >  2 files changed, 186 insertions(+), 26 deletions(-)
> >
> > diff --git a/arch/riscv/include/uapi/asm/elf.h b/arch/riscv/include/uapi/asm/elf.h
> > index d696d6610231..11a71b8533d5 100644
> > --- a/arch/riscv/include/uapi/asm/elf.h
> > +++ b/arch/riscv/include/uapi/asm/elf.h
> > @@ -49,6 +49,7 @@ typedef union __riscv_fp_state elf_fpregset_t;
> >  #define R_RISCV_TLS_DTPREL64	9
> >  #define R_RISCV_TLS_TPREL32	10
> >  #define R_RISCV_TLS_TPREL64	11
> > +#define R_RISCV_IRELATIVE	58
> >
> >  /* Relocation types not used by the dynamic linker */
> >  #define R_RISCV_BRANCH		16
> > @@ -81,7 +82,6 @@ typedef union __riscv_fp_state elf_fpregset_t;
> >  #define R_RISCV_ALIGN		43
> >  #define R_RISCV_RVC_BRANCH	44
> >  #define R_RISCV_RVC_JUMP	45
> > -#define R_RISCV_LUI		46
> >  #define R_RISCV_GPREL_I		47
> >  #define R_RISCV_GPREL_S		48
> >  #define R_RISCV_TPREL_I		49
> > @@ -93,6 +93,9 @@ typedef union __riscv_fp_state elf_fpregset_t;
> >  #define R_RISCV_SET16		55
> >  #define R_RISCV_SET32		56
> >  #define R_RISCV_32_PCREL	57
> > +#define R_RISCV_PLT32		59
> > +#define R_RISCV_SET_ULEB128	60
> > +#define R_RISCV_SUB_ULEB128	61
> >
> >
> >  #endif /* _UAPI_ASM_RISCV_ELF_H */
> > diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> > index 7c651d55fcbd..e860726352ac 100644
> > --- a/arch/riscv/kernel/module.c
> > +++ b/arch/riscv/kernel/module.c
> > @@ -7,6 +7,7 @@
> >  #include <linux/elf.h>
> >  #include <linux/err.h>
> >  #include <linux/errno.h>
> > +#include <linux/kernel.h>
> >  #include <linux/moduleloader.h>
> >  #include <linux/vmalloc.h>
> >  #include <linux/sizes.h>
> > @@ -268,6 +269,12 @@ static int apply_r_riscv_align_rela(struct module *me, u32 *location,
> >  	return -EINVAL;
> >  }
> >
> > +static int apply_r_riscv_add8_rela(struct module *me, u32 *location, Elf_Addr v)
> > +{
> > +	*(u8 *)location += (u8)v;
> > +	return 0;
> > +}
> > +
> >  static int apply_r_riscv_add16_rela(struct module *me, u32 *location,
> >  				    Elf_Addr v)
> >  {
> > @@ -289,6 +296,12 @@ static int apply_r_riscv_add64_rela(struct module *me, u32 *location,
> >  	return 0;
> >  }
> >
> > +static int apply_r_riscv_sub8_rela(struct module *me, u32 *location, Elf_Addr v)
> > +{
> > +	*(u8 *)location -= (u8)v;
> > +	return 0;
> > +}
> > +
> >  static int apply_r_riscv_sub16_rela(struct module *me, u32 *location,
> >  				    Elf_Addr v)
> >  {
> > @@ -310,31 +323,149 @@ static int apply_r_riscv_sub64_rela(struct module *me, u32 *location,
> >  	return 0;
> >  }
> >
> > -static int (*reloc_handlers_rela[]) (struct module *me, u32 *location,
> > -				Elf_Addr v) = {
> > -	[R_RISCV_32]			= apply_r_riscv_32_rela,
> > -	[R_RISCV_64]			= apply_r_riscv_64_rela,
> > -	[R_RISCV_BRANCH]		= apply_r_riscv_branch_rela,
> > -	[R_RISCV_JAL]			= apply_r_riscv_jal_rela,
> > -	[R_RISCV_RVC_BRANCH]		= apply_r_riscv_rvc_branch_rela,
> > -	[R_RISCV_RVC_JUMP]		= apply_r_riscv_rvc_jump_rela,
> > -	[R_RISCV_PCREL_HI20]		= apply_r_riscv_pcrel_hi20_rela,
> > -	[R_RISCV_PCREL_LO12_I]		= apply_r_riscv_pcrel_lo12_i_rela,
> > -	[R_RISCV_PCREL_LO12_S]		= apply_r_riscv_pcrel_lo12_s_rela,
> > -	[R_RISCV_HI20]			= apply_r_riscv_hi20_rela,
> > -	[R_RISCV_LO12_I]		= apply_r_riscv_lo12_i_rela,
> > -	[R_RISCV_LO12_S]		= apply_r_riscv_lo12_s_rela,
> > -	[R_RISCV_GOT_HI20]		= apply_r_riscv_got_hi20_rela,
> > -	[R_RISCV_CALL_PLT]		= apply_r_riscv_call_plt_rela,
> > -	[R_RISCV_CALL]			= apply_r_riscv_call_rela,
> > -	[R_RISCV_RELAX]			= apply_r_riscv_relax_rela,
> > -	[R_RISCV_ALIGN]			= apply_r_riscv_align_rela,
> > -	[R_RISCV_ADD16]			= apply_r_riscv_add16_rela,
> > -	[R_RISCV_ADD32]			= apply_r_riscv_add32_rela,
> > -	[R_RISCV_ADD64]			= apply_r_riscv_add64_rela,
> > -	[R_RISCV_SUB16]			= apply_r_riscv_sub16_rela,
> > -	[R_RISCV_SUB32]			= apply_r_riscv_sub32_rela,
> > -	[R_RISCV_SUB64]			= apply_r_riscv_sub64_rela,
> > +static int dynamic_linking_not_supported(struct module *me, u32 *location,
> > +					 Elf_Addr v)
> > +{
> > +	pr_err("%s: Dynamic linking not supported in kernel modules PC = %p\n",
> > +	       me->name, location);
> > +	return -EINVAL;
> > +}
> > +
> > +static int tls_not_supported(struct module *me, u32 *location, Elf_Addr v)
> > +{
> > +	pr_err("%s: Thread local storage not supported in kernel modules PC = %p\n",
> > +	       me->name, location);
> > +	return -EINVAL;
> > +}
> > +
> > +static int apply_r_riscv_sub6_rela(struct module *me, u32 *location, Elf_Addr v)
> > +{
> > +	*(u8 *)location = (*location - ((u8)v & 0x3F)) & 0x3F;
> > +	return 0;
> > +}
> > +
> > +static int apply_r_riscv_set6_rela(struct module *me, u32 *location, Elf_Addr v)
> > +{
> > +	*(u8 *)location = (*(u8 *)location & 0xc0) | ((u8)v & 0x3F);
> > +	return 0;
> > +}
> > +
> > +static int apply_r_riscv_set8_rela(struct module *me, u32 *location, Elf_Addr v)
> > +{
> > +	*(u8 *)location = (u8)v;
> > +	return 0;
> > +}
> > +
> > +static int apply_r_riscv_set16_rela(struct module *me, u32 *location,
> > +				    Elf_Addr v)
> > +{
> > +	*(u16 *)location = (u16)v;
> > +	return 0;
> > +}
> > +
> > +static int apply_r_riscv_set32_rela(struct module *me, u32 *location,
> > +				    Elf_Addr v)
> > +{
> > +	*(u32 *)location = (u32)v;
> > +	return 0;
> > +}
> > +
> > +static int apply_r_riscv_32_pcrel_rela(struct module *me, u32 *location,
> > +				       Elf_Addr v)
> > +{
> > +	*(u32 *)location = (u32)v;
> > +	return 0;
> > +}
> > +
> > +static int apply_r_riscv_plt32_rela(struct module *me, u32 *location,
> > +				    Elf_Addr v)
> > +{
> > +	*(u32 *)location = (u32)v;
> > +	return 0;
> > +}
> > +
> > +static int apply_r_riscv_set_uleb128(struct module *me, u32 *location, Elf_Addr v)
> > +{
> > +	/*
> > +	 * Relocation is only performed if R_RISCV_SET_ULEB128 is followed by
> > +	 * R_RISCV_SUB_ULEB128 so do computation there
> > +	 */
> > +	return 0;
> > +}
> > +
> > +static int apply_r_riscv_sub_uleb128(struct module *me, u32 *location, Elf_Addr v)
> > +{
> > +	if (v >= 128) {
> > +		pr_err("%s: uleb128 must be in [0, 127] (not %ld) at PC = %p\n",
> > +		       me->name, (unsigned long)v, location);
> > +		return -EINVAL;
> > +	}
> > +
> > +	*location = v;
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Relocations defined in the riscv-elf-psabi-doc.
> > + * This handles static linking only.
> > + */
> > +static int (*reloc_handlers_rela[])(struct module *me, u32 *location,
> > +				    Elf_Addr v) = {
> > +	[R_RISCV_32] =			apply_r_riscv_32_rela,
> > +	[R_RISCV_64] =			apply_r_riscv_64_rela,
> > +	[R_RISCV_RELATIVE] =		dynamic_linking_not_supported,
> > +	[R_RISCV_COPY] =		dynamic_linking_not_supported,
> > +	[R_RISCV_JUMP_SLOT] =		dynamic_linking_not_supported,
> > +	[R_RISCV_TLS_DTPMOD32] =	dynamic_linking_not_supported,
> > +	[R_RISCV_TLS_DTPMOD64] =	dynamic_linking_not_supported,
> > +	[R_RISCV_TLS_DTPREL32] =	dynamic_linking_not_supported,
> > +	[R_RISCV_TLS_DTPREL64] =	dynamic_linking_not_supported,
> > +	[R_RISCV_TLS_TPREL32] =		dynamic_linking_not_supported,
> > +	[R_RISCV_TLS_TPREL64] =		dynamic_linking_not_supported,
> > +	/* 12-15 undefined */
> > +	[R_RISCV_BRANCH] =		apply_r_riscv_branch_rela,
> > +	[R_RISCV_JAL] =			apply_r_riscv_jal_rela,
> > +	[R_RISCV_CALL] =		apply_r_riscv_call_rela,
> > +	[R_RISCV_CALL_PLT] =		apply_r_riscv_call_plt_rela,
> > +	[R_RISCV_GOT_HI20] =		apply_r_riscv_got_hi20_rela,
> > +	[R_RISCV_TLS_GOT_HI20] =	tls_not_supported,
> > +	[R_RISCV_TLS_GD_HI20] =		tls_not_supported,
> > +	[R_RISCV_PCREL_HI20] =		apply_r_riscv_pcrel_hi20_rela,
> > +	[R_RISCV_PCREL_LO12_I] =	apply_r_riscv_pcrel_lo12_i_rela,
> > +	[R_RISCV_PCREL_LO12_S] =	apply_r_riscv_pcrel_lo12_s_rela,
> > +	[R_RISCV_HI20] =		apply_r_riscv_hi20_rela,
> > +	[R_RISCV_LO12_I] =		apply_r_riscv_lo12_i_rela,
> > +	[R_RISCV_LO12_S] =		apply_r_riscv_lo12_s_rela,
> > +	[R_RISCV_TPREL_HI20] =		tls_not_supported,
> > +	[R_RISCV_TPREL_LO12_I] =	tls_not_supported,
> > +	[R_RISCV_TPREL_LO12_S] =	tls_not_supported,
> > +	[R_RISCV_TPREL_ADD] =		tls_not_supported,
> > +	[R_RISCV_ADD8] =		apply_r_riscv_add8_rela,
> > +	[R_RISCV_ADD16] =		apply_r_riscv_add16_rela,
> > +	[R_RISCV_ADD32] =		apply_r_riscv_add32_rela,
> > +	[R_RISCV_ADD64] =		apply_r_riscv_add64_rela,
> > +	[R_RISCV_SUB8] =		apply_r_riscv_sub8_rela,
> > +	[R_RISCV_SUB16] =		apply_r_riscv_sub16_rela,
> > +	[R_RISCV_SUB32] =		apply_r_riscv_sub32_rela,
> > +	[R_RISCV_SUB64] =		apply_r_riscv_sub64_rela,
> > +	/* 41-42 reserved for future standard use */
> > +	[R_RISCV_ALIGN] =		apply_r_riscv_align_rela,
> > +	[R_RISCV_RVC_BRANCH] =		apply_r_riscv_rvc_branch_rela,
> > +	[R_RISCV_RVC_JUMP] =		apply_r_riscv_rvc_jump_rela,
> > +	/* 46-50 reserved for future standard use */
> > +	[R_RISCV_RELAX] =		apply_r_riscv_relax_rela,
> > +	[R_RISCV_SUB6] =		apply_r_riscv_sub6_rela,
> > +	[R_RISCV_SET6] =		apply_r_riscv_set6_rela,
> > +	[R_RISCV_SET8] =		apply_r_riscv_set8_rela,
> > +	[R_RISCV_SET16] =		apply_r_riscv_set16_rela,
> > +	[R_RISCV_SET32] =		apply_r_riscv_set32_rela,
> > +	[R_RISCV_32_PCREL] =		apply_r_riscv_32_pcrel_rela,
> > +	[R_RISCV_IRELATIVE] =		dynamic_linking_not_supported,
> > +	[R_RISCV_PLT32] =		apply_r_riscv_plt32_rela,
> > +	[R_RISCV_SET_ULEB128] =		apply_r_riscv_set_uleb128,
> > +	[R_RISCV_SUB_ULEB128] =		apply_r_riscv_sub_uleb128,
> > +	/* 62-191 reserved for future standard use */
> > +	/* 192-255 nonstandard ABI extensions  */
> >  };
> 
> Hi Charlie,
> 
> This is not a critique of this patch, but all these callbacks take a
> u32 *location and
> because of the compressed instructions this pointer may not be
> aligned, so a lot of
> the callbacks end up doing unaligned access which may fault to an
> M-mode handler on
> some platforms.
> 
> I once sent a patch to fix this:
> https://lore.kernel.org/linux-riscv/20220224152456.493365-2-kernel@esmil.dk/
> 
> Maybe that's something you want to look into while touching this code anyway.
> 
> /Emil

Oh nice, I will pick up that patch and change the "native-endian"
wording to be "little-endian" in the commit.

- Charlie

> >
> >  int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> > @@ -348,6 +479,10 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> >  	unsigned int i, type;
> >  	Elf_Addr v;
> >  	int res;
> > +	bool uleb128_set_exists = false;
> > +	u32 *uleb128_set_loc;
> > +	unsigned long uleb128_set_sym_val;
> > +
> >
> >  	pr_debug("Applying relocate section %u to %u\n", relsec,
> >  	       sechdrs[relsec].sh_info);
> > @@ -425,6 +560,28 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> >  				  me->name);
> >  				return -EINVAL;
> >  			}
> > +		} else if (type == R_RISCV_SET_ULEB128) {
> > +			if (uleb128_set_exists) {
> > +				pr_err("%s: riscv psABI requires the next ULEB128 relocation to come after a R_RISCV_SET_ULEB128 is an R_RISCV_SUB_ULEB128, not another R_RISCV_SET_ULEB128.\n",
> > +				       me->name);
> > +				return -EINVAL;
> > +			}
> > +			uleb128_set_exists = true;
> > +			uleb128_set_loc = location;
> > +			uleb128_set_sym_val =
> > +				((Elf_Sym *)sechdrs[symindex].sh_addr +
> > +					ELF_RISCV_R_SYM(rel[i].r_info))
> > +					->st_value +
> > +				rel[i].r_addend;
> > +		} else if (type == R_RISCV_SUB_ULEB128) {
> > +			if (uleb128_set_exists && uleb128_set_loc == location) {
> > +				/* Calculate set and subtraction */
> > +				v = uleb128_set_sym_val - v;
> > +			} else {
> > +				pr_err("%s: R_RISCV_SUB_ULEB128 must always be paired with the first R_RISCV_SET_ULEB128 that comes before it. PC = %p\n",
> > +				       me->name, location);
> > +				return -EINVAL;
> > +			}
> >  		}
> >
> >  		res = handler(me, location, v);
> >
> > --
> > 2.34.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv


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

* Re: [PATCH v4 1/2] riscv: Add remaining module relocations
  2023-10-18 18:31     ` Charlie Jenkins
@ 2023-10-18 18:38       ` Emil Renner Berthing
  2023-10-18 20:19         ` Charlie Jenkins
  0 siblings, 1 reply; 16+ messages in thread
From: Emil Renner Berthing @ 2023-10-18 18:38 UTC (permalink / raw)
  To: Charlie Jenkins, Emil Renner Berthing
  Cc: linux-riscv, linux-mm, linux-kernel, Eric Biederman, Kees Cook,
	Paul Walmsley, Palmer Dabbelt, Albert Ou

Charlie Jenkins wrote:
> On Wed, Oct 18, 2023 at 05:17:44AM -0700, Emil Renner Berthing wrote:
> > Charlie Jenkins wrote:
> > > Add all final module relocations and add error logs explaining the ones
> > > that are not supported.
> > >
> > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > > ---
> > >  arch/riscv/include/uapi/asm/elf.h |   5 +-
> > >  arch/riscv/kernel/module.c        | 207 +++++++++++++++++++++++++++++++++-----
> > >  2 files changed, 186 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/uapi/asm/elf.h b/arch/riscv/include/uapi/asm/elf.h
> > > index d696d6610231..11a71b8533d5 100644
> > > --- a/arch/riscv/include/uapi/asm/elf.h
> > > +++ b/arch/riscv/include/uapi/asm/elf.h
> > > @@ -49,6 +49,7 @@ typedef union __riscv_fp_state elf_fpregset_t;
> > >  #define R_RISCV_TLS_DTPREL64	9
> > >  #define R_RISCV_TLS_TPREL32	10
> > >  #define R_RISCV_TLS_TPREL64	11
> > > +#define R_RISCV_IRELATIVE	58
> > >
> > >  /* Relocation types not used by the dynamic linker */
> > >  #define R_RISCV_BRANCH		16
> > > @@ -81,7 +82,6 @@ typedef union __riscv_fp_state elf_fpregset_t;
> > >  #define R_RISCV_ALIGN		43
> > >  #define R_RISCV_RVC_BRANCH	44
> > >  #define R_RISCV_RVC_JUMP	45
> > > -#define R_RISCV_LUI		46
> > >  #define R_RISCV_GPREL_I		47
> > >  #define R_RISCV_GPREL_S		48
> > >  #define R_RISCV_TPREL_I		49
> > > @@ -93,6 +93,9 @@ typedef union __riscv_fp_state elf_fpregset_t;
> > >  #define R_RISCV_SET16		55
> > >  #define R_RISCV_SET32		56
> > >  #define R_RISCV_32_PCREL	57
> > > +#define R_RISCV_PLT32		59
> > > +#define R_RISCV_SET_ULEB128	60
> > > +#define R_RISCV_SUB_ULEB128	61
> > >
> > >
> > >  #endif /* _UAPI_ASM_RISCV_ELF_H */
> > > diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> > > index 7c651d55fcbd..e860726352ac 100644
> > > --- a/arch/riscv/kernel/module.c
> > > +++ b/arch/riscv/kernel/module.c
> > > @@ -7,6 +7,7 @@
> > >  #include <linux/elf.h>
> > >  #include <linux/err.h>
> > >  #include <linux/errno.h>
> > > +#include <linux/kernel.h>
> > >  #include <linux/moduleloader.h>
> > >  #include <linux/vmalloc.h>
> > >  #include <linux/sizes.h>
> > > @@ -268,6 +269,12 @@ static int apply_r_riscv_align_rela(struct module *me, u32 *location,
> > >  	return -EINVAL;
> > >  }
> > >
> > > +static int apply_r_riscv_add8_rela(struct module *me, u32 *location, Elf_Addr v)
> > > +{
> > > +	*(u8 *)location += (u8)v;
> > > +	return 0;
> > > +}
> > > +
> > >  static int apply_r_riscv_add16_rela(struct module *me, u32 *location,
> > >  				    Elf_Addr v)
> > >  {
> > > @@ -289,6 +296,12 @@ static int apply_r_riscv_add64_rela(struct module *me, u32 *location,
> > >  	return 0;
> > >  }
> > >
> > > +static int apply_r_riscv_sub8_rela(struct module *me, u32 *location, Elf_Addr v)
> > > +{
> > > +	*(u8 *)location -= (u8)v;
> > > +	return 0;
> > > +}
> > > +
> > >  static int apply_r_riscv_sub16_rela(struct module *me, u32 *location,
> > >  				    Elf_Addr v)
> > >  {
> > > @@ -310,31 +323,149 @@ static int apply_r_riscv_sub64_rela(struct module *me, u32 *location,
> > >  	return 0;
> > >  }
> > >
> > > -static int (*reloc_handlers_rela[]) (struct module *me, u32 *location,
> > > -				Elf_Addr v) = {
> > > -	[R_RISCV_32]			= apply_r_riscv_32_rela,
> > > -	[R_RISCV_64]			= apply_r_riscv_64_rela,
> > > -	[R_RISCV_BRANCH]		= apply_r_riscv_branch_rela,
> > > -	[R_RISCV_JAL]			= apply_r_riscv_jal_rela,
> > > -	[R_RISCV_RVC_BRANCH]		= apply_r_riscv_rvc_branch_rela,
> > > -	[R_RISCV_RVC_JUMP]		= apply_r_riscv_rvc_jump_rela,
> > > -	[R_RISCV_PCREL_HI20]		= apply_r_riscv_pcrel_hi20_rela,
> > > -	[R_RISCV_PCREL_LO12_I]		= apply_r_riscv_pcrel_lo12_i_rela,
> > > -	[R_RISCV_PCREL_LO12_S]		= apply_r_riscv_pcrel_lo12_s_rela,
> > > -	[R_RISCV_HI20]			= apply_r_riscv_hi20_rela,
> > > -	[R_RISCV_LO12_I]		= apply_r_riscv_lo12_i_rela,
> > > -	[R_RISCV_LO12_S]		= apply_r_riscv_lo12_s_rela,
> > > -	[R_RISCV_GOT_HI20]		= apply_r_riscv_got_hi20_rela,
> > > -	[R_RISCV_CALL_PLT]		= apply_r_riscv_call_plt_rela,
> > > -	[R_RISCV_CALL]			= apply_r_riscv_call_rela,
> > > -	[R_RISCV_RELAX]			= apply_r_riscv_relax_rela,
> > > -	[R_RISCV_ALIGN]			= apply_r_riscv_align_rela,
> > > -	[R_RISCV_ADD16]			= apply_r_riscv_add16_rela,
> > > -	[R_RISCV_ADD32]			= apply_r_riscv_add32_rela,
> > > -	[R_RISCV_ADD64]			= apply_r_riscv_add64_rela,
> > > -	[R_RISCV_SUB16]			= apply_r_riscv_sub16_rela,
> > > -	[R_RISCV_SUB32]			= apply_r_riscv_sub32_rela,
> > > -	[R_RISCV_SUB64]			= apply_r_riscv_sub64_rela,
> > > +static int dynamic_linking_not_supported(struct module *me, u32 *location,
> > > +					 Elf_Addr v)
> > > +{
> > > +	pr_err("%s: Dynamic linking not supported in kernel modules PC = %p\n",
> > > +	       me->name, location);
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static int tls_not_supported(struct module *me, u32 *location, Elf_Addr v)
> > > +{
> > > +	pr_err("%s: Thread local storage not supported in kernel modules PC = %p\n",
> > > +	       me->name, location);
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static int apply_r_riscv_sub6_rela(struct module *me, u32 *location, Elf_Addr v)
> > > +{
> > > +	*(u8 *)location = (*location - ((u8)v & 0x3F)) & 0x3F;
> > > +	return 0;
> > > +}
> > > +
> > > +static int apply_r_riscv_set6_rela(struct module *me, u32 *location, Elf_Addr v)
> > > +{
> > > +	*(u8 *)location = (*(u8 *)location & 0xc0) | ((u8)v & 0x3F);
> > > +	return 0;
> > > +}
> > > +
> > > +static int apply_r_riscv_set8_rela(struct module *me, u32 *location, Elf_Addr v)
> > > +{
> > > +	*(u8 *)location = (u8)v;
> > > +	return 0;
> > > +}
> > > +
> > > +static int apply_r_riscv_set16_rela(struct module *me, u32 *location,
> > > +				    Elf_Addr v)
> > > +{
> > > +	*(u16 *)location = (u16)v;
> > > +	return 0;
> > > +}
> > > +
> > > +static int apply_r_riscv_set32_rela(struct module *me, u32 *location,
> > > +				    Elf_Addr v)
> > > +{
> > > +	*(u32 *)location = (u32)v;
> > > +	return 0;
> > > +}
> > > +
> > > +static int apply_r_riscv_32_pcrel_rela(struct module *me, u32 *location,
> > > +				       Elf_Addr v)
> > > +{
> > > +	*(u32 *)location = (u32)v;
> > > +	return 0;
> > > +}
> > > +
> > > +static int apply_r_riscv_plt32_rela(struct module *me, u32 *location,
> > > +				    Elf_Addr v)
> > > +{
> > > +	*(u32 *)location = (u32)v;
> > > +	return 0;
> > > +}
> > > +
> > > +static int apply_r_riscv_set_uleb128(struct module *me, u32 *location, Elf_Addr v)
> > > +{
> > > +	/*
> > > +	 * Relocation is only performed if R_RISCV_SET_ULEB128 is followed by
> > > +	 * R_RISCV_SUB_ULEB128 so do computation there
> > > +	 */
> > > +	return 0;
> > > +}
> > > +
> > > +static int apply_r_riscv_sub_uleb128(struct module *me, u32 *location, Elf_Addr v)
> > > +{
> > > +	if (v >= 128) {
> > > +		pr_err("%s: uleb128 must be in [0, 127] (not %ld) at PC = %p\n",
> > > +		       me->name, (unsigned long)v, location);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	*location = v;
> > > +	return 0;
> > > +}
> > > +
> > > +/*
> > > + * Relocations defined in the riscv-elf-psabi-doc.
> > > + * This handles static linking only.
> > > + */
> > > +static int (*reloc_handlers_rela[])(struct module *me, u32 *location,
> > > +				    Elf_Addr v) = {
> > > +	[R_RISCV_32] =			apply_r_riscv_32_rela,
> > > +	[R_RISCV_64] =			apply_r_riscv_64_rela,
> > > +	[R_RISCV_RELATIVE] =		dynamic_linking_not_supported,
> > > +	[R_RISCV_COPY] =		dynamic_linking_not_supported,
> > > +	[R_RISCV_JUMP_SLOT] =		dynamic_linking_not_supported,
> > > +	[R_RISCV_TLS_DTPMOD32] =	dynamic_linking_not_supported,
> > > +	[R_RISCV_TLS_DTPMOD64] =	dynamic_linking_not_supported,
> > > +	[R_RISCV_TLS_DTPREL32] =	dynamic_linking_not_supported,
> > > +	[R_RISCV_TLS_DTPREL64] =	dynamic_linking_not_supported,
> > > +	[R_RISCV_TLS_TPREL32] =		dynamic_linking_not_supported,
> > > +	[R_RISCV_TLS_TPREL64] =		dynamic_linking_not_supported,
> > > +	/* 12-15 undefined */
> > > +	[R_RISCV_BRANCH] =		apply_r_riscv_branch_rela,
> > > +	[R_RISCV_JAL] =			apply_r_riscv_jal_rela,
> > > +	[R_RISCV_CALL] =		apply_r_riscv_call_rela,
> > > +	[R_RISCV_CALL_PLT] =		apply_r_riscv_call_plt_rela,
> > > +	[R_RISCV_GOT_HI20] =		apply_r_riscv_got_hi20_rela,
> > > +	[R_RISCV_TLS_GOT_HI20] =	tls_not_supported,
> > > +	[R_RISCV_TLS_GD_HI20] =		tls_not_supported,
> > > +	[R_RISCV_PCREL_HI20] =		apply_r_riscv_pcrel_hi20_rela,
> > > +	[R_RISCV_PCREL_LO12_I] =	apply_r_riscv_pcrel_lo12_i_rela,
> > > +	[R_RISCV_PCREL_LO12_S] =	apply_r_riscv_pcrel_lo12_s_rela,
> > > +	[R_RISCV_HI20] =		apply_r_riscv_hi20_rela,
> > > +	[R_RISCV_LO12_I] =		apply_r_riscv_lo12_i_rela,
> > > +	[R_RISCV_LO12_S] =		apply_r_riscv_lo12_s_rela,
> > > +	[R_RISCV_TPREL_HI20] =		tls_not_supported,
> > > +	[R_RISCV_TPREL_LO12_I] =	tls_not_supported,
> > > +	[R_RISCV_TPREL_LO12_S] =	tls_not_supported,
> > > +	[R_RISCV_TPREL_ADD] =		tls_not_supported,
> > > +	[R_RISCV_ADD8] =		apply_r_riscv_add8_rela,
> > > +	[R_RISCV_ADD16] =		apply_r_riscv_add16_rela,
> > > +	[R_RISCV_ADD32] =		apply_r_riscv_add32_rela,
> > > +	[R_RISCV_ADD64] =		apply_r_riscv_add64_rela,
> > > +	[R_RISCV_SUB8] =		apply_r_riscv_sub8_rela,
> > > +	[R_RISCV_SUB16] =		apply_r_riscv_sub16_rela,
> > > +	[R_RISCV_SUB32] =		apply_r_riscv_sub32_rela,
> > > +	[R_RISCV_SUB64] =		apply_r_riscv_sub64_rela,
> > > +	/* 41-42 reserved for future standard use */
> > > +	[R_RISCV_ALIGN] =		apply_r_riscv_align_rela,
> > > +	[R_RISCV_RVC_BRANCH] =		apply_r_riscv_rvc_branch_rela,
> > > +	[R_RISCV_RVC_JUMP] =		apply_r_riscv_rvc_jump_rela,
> > > +	/* 46-50 reserved for future standard use */
> > > +	[R_RISCV_RELAX] =		apply_r_riscv_relax_rela,
> > > +	[R_RISCV_SUB6] =		apply_r_riscv_sub6_rela,
> > > +	[R_RISCV_SET6] =		apply_r_riscv_set6_rela,
> > > +	[R_RISCV_SET8] =		apply_r_riscv_set8_rela,
> > > +	[R_RISCV_SET16] =		apply_r_riscv_set16_rela,
> > > +	[R_RISCV_SET32] =		apply_r_riscv_set32_rela,
> > > +	[R_RISCV_32_PCREL] =		apply_r_riscv_32_pcrel_rela,
> > > +	[R_RISCV_IRELATIVE] =		dynamic_linking_not_supported,
> > > +	[R_RISCV_PLT32] =		apply_r_riscv_plt32_rela,
> > > +	[R_RISCV_SET_ULEB128] =		apply_r_riscv_set_uleb128,
> > > +	[R_RISCV_SUB_ULEB128] =		apply_r_riscv_sub_uleb128,
> > > +	/* 62-191 reserved for future standard use */
> > > +	/* 192-255 nonstandard ABI extensions  */
> > >  };
> >
> > Hi Charlie,
> >
> > This is not a critique of this patch, but all these callbacks take a
> > u32 *location and
> > because of the compressed instructions this pointer may not be
> > aligned, so a lot of
> > the callbacks end up doing unaligned access which may fault to an
> > M-mode handler on
> > some platforms.
> >
> > I once sent a patch to fix this:
> > https://lore.kernel.org/linux-riscv/20220224152456.493365-2-kernel@esmil.dk/
> >
> > Maybe that's something you want to look into while touching this code anyway.
> >
> > /Emil
>
> Oh nice, I will pick up that patch and change the "native-endian"
> wording to be "little-endian" in the commit.

Great, thanks. You'll probably also want the reads to be wrapped in
le16_to_cpu() and similar when writing now that it's decided that the parcels
are always in little-endian byteorder.

/Emil

>
> - Charlie
>
> > >
> > >  int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> > > @@ -348,6 +479,10 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> > >  	unsigned int i, type;
> > >  	Elf_Addr v;
> > >  	int res;
> > > +	bool uleb128_set_exists = false;
> > > +	u32 *uleb128_set_loc;
> > > +	unsigned long uleb128_set_sym_val;
> > > +
> > >
> > >  	pr_debug("Applying relocate section %u to %u\n", relsec,
> > >  	       sechdrs[relsec].sh_info);
> > > @@ -425,6 +560,28 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> > >  				  me->name);
> > >  				return -EINVAL;
> > >  			}
> > > +		} else if (type == R_RISCV_SET_ULEB128) {
> > > +			if (uleb128_set_exists) {
> > > +				pr_err("%s: riscv psABI requires the next ULEB128 relocation to come after a R_RISCV_SET_ULEB128 is an R_RISCV_SUB_ULEB128, not another R_RISCV_SET_ULEB128.\n",
> > > +				       me->name);
> > > +				return -EINVAL;
> > > +			}
> > > +			uleb128_set_exists = true;
> > > +			uleb128_set_loc = location;
> > > +			uleb128_set_sym_val =
> > > +				((Elf_Sym *)sechdrs[symindex].sh_addr +
> > > +					ELF_RISCV_R_SYM(rel[i].r_info))
> > > +					->st_value +
> > > +				rel[i].r_addend;
> > > +		} else if (type == R_RISCV_SUB_ULEB128) {
> > > +			if (uleb128_set_exists && uleb128_set_loc == location) {
> > > +				/* Calculate set and subtraction */
> > > +				v = uleb128_set_sym_val - v;
> > > +			} else {
> > > +				pr_err("%s: R_RISCV_SUB_ULEB128 must always be paired with the first R_RISCV_SET_ULEB128 that comes before it. PC = %p\n",
> > > +				       me->name, location);
> > > +				return -EINVAL;
> > > +			}
> > >  		}
> > >
> > >  		res = handler(me, location, v);
> > >
> > > --
> > > 2.34.1
> > >
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv


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

* Re: [PATCH v4 1/2] riscv: Add remaining module relocations
  2023-10-18 18:38       ` Emil Renner Berthing
@ 2023-10-18 20:19         ` Charlie Jenkins
  2023-10-18 20:28           ` Emil Renner Berthing
  0 siblings, 1 reply; 16+ messages in thread
From: Charlie Jenkins @ 2023-10-18 20:19 UTC (permalink / raw)
  To: Emil Renner Berthing
  Cc: linux-riscv, linux-mm, linux-kernel, Eric Biederman, Kees Cook,
	Paul Walmsley, Palmer Dabbelt, Albert Ou

On Wed, Oct 18, 2023 at 11:38:39AM -0700, Emil Renner Berthing wrote:
> Charlie Jenkins wrote:
> > On Wed, Oct 18, 2023 at 05:17:44AM -0700, Emil Renner Berthing wrote:
> > > Charlie Jenkins wrote:
> > > > Add all final module relocations and add error logs explaining the ones
> > > > that are not supported.
> > > >
> > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > > > ---
> > > >  arch/riscv/include/uapi/asm/elf.h |   5 +-
> > > >  arch/riscv/kernel/module.c        | 207 +++++++++++++++++++++++++++++++++-----
> > > >  2 files changed, 186 insertions(+), 26 deletions(-)
> > > >
> > > > diff --git a/arch/riscv/include/uapi/asm/elf.h b/arch/riscv/include/uapi/asm/elf.h
> > > > index d696d6610231..11a71b8533d5 100644
> > > > --- a/arch/riscv/include/uapi/asm/elf.h
> > > > +++ b/arch/riscv/include/uapi/asm/elf.h
> > > > @@ -49,6 +49,7 @@ typedef union __riscv_fp_state elf_fpregset_t;
> > > >  #define R_RISCV_TLS_DTPREL64	9
> > > >  #define R_RISCV_TLS_TPREL32	10
> > > >  #define R_RISCV_TLS_TPREL64	11
> > > > +#define R_RISCV_IRELATIVE	58
> > > >
> > > >  /* Relocation types not used by the dynamic linker */
> > > >  #define R_RISCV_BRANCH		16
> > > > @@ -81,7 +82,6 @@ typedef union __riscv_fp_state elf_fpregset_t;
> > > >  #define R_RISCV_ALIGN		43
> > > >  #define R_RISCV_RVC_BRANCH	44
> > > >  #define R_RISCV_RVC_JUMP	45
> > > > -#define R_RISCV_LUI		46
> > > >  #define R_RISCV_GPREL_I		47
> > > >  #define R_RISCV_GPREL_S		48
> > > >  #define R_RISCV_TPREL_I		49
> > > > @@ -93,6 +93,9 @@ typedef union __riscv_fp_state elf_fpregset_t;
> > > >  #define R_RISCV_SET16		55
> > > >  #define R_RISCV_SET32		56
> > > >  #define R_RISCV_32_PCREL	57
> > > > +#define R_RISCV_PLT32		59
> > > > +#define R_RISCV_SET_ULEB128	60
> > > > +#define R_RISCV_SUB_ULEB128	61
> > > >
> > > >
> > > >  #endif /* _UAPI_ASM_RISCV_ELF_H */
> > > > diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> > > > index 7c651d55fcbd..e860726352ac 100644
> > > > --- a/arch/riscv/kernel/module.c
> > > > +++ b/arch/riscv/kernel/module.c
> > > > @@ -7,6 +7,7 @@
> > > >  #include <linux/elf.h>
> > > >  #include <linux/err.h>
> > > >  #include <linux/errno.h>
> > > > +#include <linux/kernel.h>
> > > >  #include <linux/moduleloader.h>
> > > >  #include <linux/vmalloc.h>
> > > >  #include <linux/sizes.h>
> > > > @@ -268,6 +269,12 @@ static int apply_r_riscv_align_rela(struct module *me, u32 *location,
> > > >  	return -EINVAL;
> > > >  }
> > > >
> > > > +static int apply_r_riscv_add8_rela(struct module *me, u32 *location, Elf_Addr v)
> > > > +{
> > > > +	*(u8 *)location += (u8)v;
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static int apply_r_riscv_add16_rela(struct module *me, u32 *location,
> > > >  				    Elf_Addr v)
> > > >  {
> > > > @@ -289,6 +296,12 @@ static int apply_r_riscv_add64_rela(struct module *me, u32 *location,
> > > >  	return 0;
> > > >  }
> > > >
> > > > +static int apply_r_riscv_sub8_rela(struct module *me, u32 *location, Elf_Addr v)
> > > > +{
> > > > +	*(u8 *)location -= (u8)v;
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static int apply_r_riscv_sub16_rela(struct module *me, u32 *location,
> > > >  				    Elf_Addr v)
> > > >  {
> > > > @@ -310,31 +323,149 @@ static int apply_r_riscv_sub64_rela(struct module *me, u32 *location,
> > > >  	return 0;
> > > >  }
> > > >
> > > > -static int (*reloc_handlers_rela[]) (struct module *me, u32 *location,
> > > > -				Elf_Addr v) = {
> > > > -	[R_RISCV_32]			= apply_r_riscv_32_rela,
> > > > -	[R_RISCV_64]			= apply_r_riscv_64_rela,
> > > > -	[R_RISCV_BRANCH]		= apply_r_riscv_branch_rela,
> > > > -	[R_RISCV_JAL]			= apply_r_riscv_jal_rela,
> > > > -	[R_RISCV_RVC_BRANCH]		= apply_r_riscv_rvc_branch_rela,
> > > > -	[R_RISCV_RVC_JUMP]		= apply_r_riscv_rvc_jump_rela,
> > > > -	[R_RISCV_PCREL_HI20]		= apply_r_riscv_pcrel_hi20_rela,
> > > > -	[R_RISCV_PCREL_LO12_I]		= apply_r_riscv_pcrel_lo12_i_rela,
> > > > -	[R_RISCV_PCREL_LO12_S]		= apply_r_riscv_pcrel_lo12_s_rela,
> > > > -	[R_RISCV_HI20]			= apply_r_riscv_hi20_rela,
> > > > -	[R_RISCV_LO12_I]		= apply_r_riscv_lo12_i_rela,
> > > > -	[R_RISCV_LO12_S]		= apply_r_riscv_lo12_s_rela,
> > > > -	[R_RISCV_GOT_HI20]		= apply_r_riscv_got_hi20_rela,
> > > > -	[R_RISCV_CALL_PLT]		= apply_r_riscv_call_plt_rela,
> > > > -	[R_RISCV_CALL]			= apply_r_riscv_call_rela,
> > > > -	[R_RISCV_RELAX]			= apply_r_riscv_relax_rela,
> > > > -	[R_RISCV_ALIGN]			= apply_r_riscv_align_rela,
> > > > -	[R_RISCV_ADD16]			= apply_r_riscv_add16_rela,
> > > > -	[R_RISCV_ADD32]			= apply_r_riscv_add32_rela,
> > > > -	[R_RISCV_ADD64]			= apply_r_riscv_add64_rela,
> > > > -	[R_RISCV_SUB16]			= apply_r_riscv_sub16_rela,
> > > > -	[R_RISCV_SUB32]			= apply_r_riscv_sub32_rela,
> > > > -	[R_RISCV_SUB64]			= apply_r_riscv_sub64_rela,
> > > > +static int dynamic_linking_not_supported(struct module *me, u32 *location,
> > > > +					 Elf_Addr v)
> > > > +{
> > > > +	pr_err("%s: Dynamic linking not supported in kernel modules PC = %p\n",
> > > > +	       me->name, location);
> > > > +	return -EINVAL;
> > > > +}
> > > > +
> > > > +static int tls_not_supported(struct module *me, u32 *location, Elf_Addr v)
> > > > +{
> > > > +	pr_err("%s: Thread local storage not supported in kernel modules PC = %p\n",
> > > > +	       me->name, location);
> > > > +	return -EINVAL;
> > > > +}
> > > > +
> > > > +static int apply_r_riscv_sub6_rela(struct module *me, u32 *location, Elf_Addr v)
> > > > +{
> > > > +	*(u8 *)location = (*location - ((u8)v & 0x3F)) & 0x3F;
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int apply_r_riscv_set6_rela(struct module *me, u32 *location, Elf_Addr v)
> > > > +{
> > > > +	*(u8 *)location = (*(u8 *)location & 0xc0) | ((u8)v & 0x3F);
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int apply_r_riscv_set8_rela(struct module *me, u32 *location, Elf_Addr v)
> > > > +{
> > > > +	*(u8 *)location = (u8)v;
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int apply_r_riscv_set16_rela(struct module *me, u32 *location,
> > > > +				    Elf_Addr v)
> > > > +{
> > > > +	*(u16 *)location = (u16)v;
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int apply_r_riscv_set32_rela(struct module *me, u32 *location,
> > > > +				    Elf_Addr v)
> > > > +{
> > > > +	*(u32 *)location = (u32)v;
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int apply_r_riscv_32_pcrel_rela(struct module *me, u32 *location,
> > > > +				       Elf_Addr v)
> > > > +{
> > > > +	*(u32 *)location = (u32)v;
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int apply_r_riscv_plt32_rela(struct module *me, u32 *location,
> > > > +				    Elf_Addr v)
> > > > +{
> > > > +	*(u32 *)location = (u32)v;
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int apply_r_riscv_set_uleb128(struct module *me, u32 *location, Elf_Addr v)
> > > > +{
> > > > +	/*
> > > > +	 * Relocation is only performed if R_RISCV_SET_ULEB128 is followed by
> > > > +	 * R_RISCV_SUB_ULEB128 so do computation there
> > > > +	 */
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int apply_r_riscv_sub_uleb128(struct module *me, u32 *location, Elf_Addr v)
> > > > +{
> > > > +	if (v >= 128) {
> > > > +		pr_err("%s: uleb128 must be in [0, 127] (not %ld) at PC = %p\n",
> > > > +		       me->name, (unsigned long)v, location);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	*location = v;
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Relocations defined in the riscv-elf-psabi-doc.
> > > > + * This handles static linking only.
> > > > + */
> > > > +static int (*reloc_handlers_rela[])(struct module *me, u32 *location,
> > > > +				    Elf_Addr v) = {
> > > > +	[R_RISCV_32] =			apply_r_riscv_32_rela,
> > > > +	[R_RISCV_64] =			apply_r_riscv_64_rela,
> > > > +	[R_RISCV_RELATIVE] =		dynamic_linking_not_supported,
> > > > +	[R_RISCV_COPY] =		dynamic_linking_not_supported,
> > > > +	[R_RISCV_JUMP_SLOT] =		dynamic_linking_not_supported,
> > > > +	[R_RISCV_TLS_DTPMOD32] =	dynamic_linking_not_supported,
> > > > +	[R_RISCV_TLS_DTPMOD64] =	dynamic_linking_not_supported,
> > > > +	[R_RISCV_TLS_DTPREL32] =	dynamic_linking_not_supported,
> > > > +	[R_RISCV_TLS_DTPREL64] =	dynamic_linking_not_supported,
> > > > +	[R_RISCV_TLS_TPREL32] =		dynamic_linking_not_supported,
> > > > +	[R_RISCV_TLS_TPREL64] =		dynamic_linking_not_supported,
> > > > +	/* 12-15 undefined */
> > > > +	[R_RISCV_BRANCH] =		apply_r_riscv_branch_rela,
> > > > +	[R_RISCV_JAL] =			apply_r_riscv_jal_rela,
> > > > +	[R_RISCV_CALL] =		apply_r_riscv_call_rela,
> > > > +	[R_RISCV_CALL_PLT] =		apply_r_riscv_call_plt_rela,
> > > > +	[R_RISCV_GOT_HI20] =		apply_r_riscv_got_hi20_rela,
> > > > +	[R_RISCV_TLS_GOT_HI20] =	tls_not_supported,
> > > > +	[R_RISCV_TLS_GD_HI20] =		tls_not_supported,
> > > > +	[R_RISCV_PCREL_HI20] =		apply_r_riscv_pcrel_hi20_rela,
> > > > +	[R_RISCV_PCREL_LO12_I] =	apply_r_riscv_pcrel_lo12_i_rela,
> > > > +	[R_RISCV_PCREL_LO12_S] =	apply_r_riscv_pcrel_lo12_s_rela,
> > > > +	[R_RISCV_HI20] =		apply_r_riscv_hi20_rela,
> > > > +	[R_RISCV_LO12_I] =		apply_r_riscv_lo12_i_rela,
> > > > +	[R_RISCV_LO12_S] =		apply_r_riscv_lo12_s_rela,
> > > > +	[R_RISCV_TPREL_HI20] =		tls_not_supported,
> > > > +	[R_RISCV_TPREL_LO12_I] =	tls_not_supported,
> > > > +	[R_RISCV_TPREL_LO12_S] =	tls_not_supported,
> > > > +	[R_RISCV_TPREL_ADD] =		tls_not_supported,
> > > > +	[R_RISCV_ADD8] =		apply_r_riscv_add8_rela,
> > > > +	[R_RISCV_ADD16] =		apply_r_riscv_add16_rela,
> > > > +	[R_RISCV_ADD32] =		apply_r_riscv_add32_rela,
> > > > +	[R_RISCV_ADD64] =		apply_r_riscv_add64_rela,
> > > > +	[R_RISCV_SUB8] =		apply_r_riscv_sub8_rela,
> > > > +	[R_RISCV_SUB16] =		apply_r_riscv_sub16_rela,
> > > > +	[R_RISCV_SUB32] =		apply_r_riscv_sub32_rela,
> > > > +	[R_RISCV_SUB64] =		apply_r_riscv_sub64_rela,
> > > > +	/* 41-42 reserved for future standard use */
> > > > +	[R_RISCV_ALIGN] =		apply_r_riscv_align_rela,
> > > > +	[R_RISCV_RVC_BRANCH] =		apply_r_riscv_rvc_branch_rela,
> > > > +	[R_RISCV_RVC_JUMP] =		apply_r_riscv_rvc_jump_rela,
> > > > +	/* 46-50 reserved for future standard use */
> > > > +	[R_RISCV_RELAX] =		apply_r_riscv_relax_rela,
> > > > +	[R_RISCV_SUB6] =		apply_r_riscv_sub6_rela,
> > > > +	[R_RISCV_SET6] =		apply_r_riscv_set6_rela,
> > > > +	[R_RISCV_SET8] =		apply_r_riscv_set8_rela,
> > > > +	[R_RISCV_SET16] =		apply_r_riscv_set16_rela,
> > > > +	[R_RISCV_SET32] =		apply_r_riscv_set32_rela,
> > > > +	[R_RISCV_32_PCREL] =		apply_r_riscv_32_pcrel_rela,
> > > > +	[R_RISCV_IRELATIVE] =		dynamic_linking_not_supported,
> > > > +	[R_RISCV_PLT32] =		apply_r_riscv_plt32_rela,
> > > > +	[R_RISCV_SET_ULEB128] =		apply_r_riscv_set_uleb128,
> > > > +	[R_RISCV_SUB_ULEB128] =		apply_r_riscv_sub_uleb128,
> > > > +	/* 62-191 reserved for future standard use */
> > > > +	/* 192-255 nonstandard ABI extensions  */
> > > >  };
> > >
> > > Hi Charlie,
> > >
> > > This is not a critique of this patch, but all these callbacks take a
> > > u32 *location and
> > > because of the compressed instructions this pointer may not be
> > > aligned, so a lot of
> > > the callbacks end up doing unaligned access which may fault to an
> > > M-mode handler on
> > > some platforms.
> > >
> > > I once sent a patch to fix this:
> > > https://lore.kernel.org/linux-riscv/20220224152456.493365-2-kernel@esmil.dk/
> > >
> > > Maybe that's something you want to look into while touching this code anyway.
> > >
> > > /Emil
> >
> > Oh nice, I will pick up that patch and change the "native-endian"
> > wording to be "little-endian" in the commit.
> 
> Great, thanks. You'll probably also want the reads to be wrapped in
> le16_to_cpu() and similar when writing now that it's decided that the parcels
> are always in little-endian byteorder.
> 
> /Emil

I believe that le16_to_cpu() is only needed when instructions are being modified, and
the relocations that only touch data can be left alone. Is this correct?

- Charlie

> 
> >
> > - Charlie
> >
> > > >
> > > >  int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> > > > @@ -348,6 +479,10 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> > > >  	unsigned int i, type;
> > > >  	Elf_Addr v;
> > > >  	int res;
> > > > +	bool uleb128_set_exists = false;
> > > > +	u32 *uleb128_set_loc;
> > > > +	unsigned long uleb128_set_sym_val;
> > > > +
> > > >
> > > >  	pr_debug("Applying relocate section %u to %u\n", relsec,
> > > >  	       sechdrs[relsec].sh_info);
> > > > @@ -425,6 +560,28 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> > > >  				  me->name);
> > > >  				return -EINVAL;
> > > >  			}
> > > > +		} else if (type == R_RISCV_SET_ULEB128) {
> > > > +			if (uleb128_set_exists) {
> > > > +				pr_err("%s: riscv psABI requires the next ULEB128 relocation to come after a R_RISCV_SET_ULEB128 is an R_RISCV_SUB_ULEB128, not another R_RISCV_SET_ULEB128.\n",
> > > > +				       me->name);
> > > > +				return -EINVAL;
> > > > +			}
> > > > +			uleb128_set_exists = true;
> > > > +			uleb128_set_loc = location;
> > > > +			uleb128_set_sym_val =
> > > > +				((Elf_Sym *)sechdrs[symindex].sh_addr +
> > > > +					ELF_RISCV_R_SYM(rel[i].r_info))
> > > > +					->st_value +
> > > > +				rel[i].r_addend;
> > > > +		} else if (type == R_RISCV_SUB_ULEB128) {
> > > > +			if (uleb128_set_exists && uleb128_set_loc == location) {
> > > > +				/* Calculate set and subtraction */
> > > > +				v = uleb128_set_sym_val - v;
> > > > +			} else {
> > > > +				pr_err("%s: R_RISCV_SUB_ULEB128 must always be paired with the first R_RISCV_SET_ULEB128 that comes before it. PC = %p\n",
> > > > +				       me->name, location);
> > > > +				return -EINVAL;
> > > > +			}
> > > >  		}
> > > >
> > > >  		res = handler(me, location, v);
> > > >
> > > > --
> > > > 2.34.1
> > > >
> > > >
> > > > _______________________________________________
> > > > linux-riscv mailing list
> > > > linux-riscv@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv


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

* Re: [PATCH v4 1/2] riscv: Add remaining module relocations
  2023-10-18 20:19         ` Charlie Jenkins
@ 2023-10-18 20:28           ` Emil Renner Berthing
  0 siblings, 0 replies; 16+ messages in thread
From: Emil Renner Berthing @ 2023-10-18 20:28 UTC (permalink / raw)
  To: Charlie Jenkins, Emil Renner Berthing
  Cc: linux-riscv, linux-mm, linux-kernel, Eric Biederman, Kees Cook,
	Paul Walmsley, Palmer Dabbelt, Albert Ou

Charlie Jenkins wrote:
> On Wed, Oct 18, 2023 at 11:38:39AM -0700, Emil Renner Berthing wrote:
> > Charlie Jenkins wrote:
> > > On Wed, Oct 18, 2023 at 05:17:44AM -0700, Emil Renner Berthing wrote:
> > > > Charlie Jenkins wrote:
> > > > > Add all final module relocations and add error logs explaining the ones
> > > > > that are not supported.
> > > > >
> > > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > > > > ---
> > > > >  arch/riscv/include/uapi/asm/elf.h |   5 +-
> > > > >  arch/riscv/kernel/module.c        | 207 +++++++++++++++++++++++++++++++++-----
> > > > >  2 files changed, 186 insertions(+), 26 deletions(-)
> > > > >
> > > > > diff --git a/arch/riscv/include/uapi/asm/elf.h b/arch/riscv/include/uapi/asm/elf.h
> > > > > index d696d6610231..11a71b8533d5 100644
> > > > > --- a/arch/riscv/include/uapi/asm/elf.h
> > > > > +++ b/arch/riscv/include/uapi/asm/elf.h
> > > > > @@ -49,6 +49,7 @@ typedef union __riscv_fp_state elf_fpregset_t;
> > > > >  #define R_RISCV_TLS_DTPREL64	9
> > > > >  #define R_RISCV_TLS_TPREL32	10
> > > > >  #define R_RISCV_TLS_TPREL64	11
> > > > > +#define R_RISCV_IRELATIVE	58
> > > > >
> > > > >  /* Relocation types not used by the dynamic linker */
> > > > >  #define R_RISCV_BRANCH		16
> > > > > @@ -81,7 +82,6 @@ typedef union __riscv_fp_state elf_fpregset_t;
> > > > >  #define R_RISCV_ALIGN		43
> > > > >  #define R_RISCV_RVC_BRANCH	44
> > > > >  #define R_RISCV_RVC_JUMP	45
> > > > > -#define R_RISCV_LUI		46
> > > > >  #define R_RISCV_GPREL_I		47
> > > > >  #define R_RISCV_GPREL_S		48
> > > > >  #define R_RISCV_TPREL_I		49
> > > > > @@ -93,6 +93,9 @@ typedef union __riscv_fp_state elf_fpregset_t;
> > > > >  #define R_RISCV_SET16		55
> > > > >  #define R_RISCV_SET32		56
> > > > >  #define R_RISCV_32_PCREL	57
> > > > > +#define R_RISCV_PLT32		59
> > > > > +#define R_RISCV_SET_ULEB128	60
> > > > > +#define R_RISCV_SUB_ULEB128	61
> > > > >
> > > > >
> > > > >  #endif /* _UAPI_ASM_RISCV_ELF_H */
> > > > > diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> > > > > index 7c651d55fcbd..e860726352ac 100644
> > > > > --- a/arch/riscv/kernel/module.c
> > > > > +++ b/arch/riscv/kernel/module.c
> > > > > @@ -7,6 +7,7 @@
> > > > >  #include <linux/elf.h>
> > > > >  #include <linux/err.h>
> > > > >  #include <linux/errno.h>
> > > > > +#include <linux/kernel.h>
> > > > >  #include <linux/moduleloader.h>
> > > > >  #include <linux/vmalloc.h>
> > > > >  #include <linux/sizes.h>
> > > > > @@ -268,6 +269,12 @@ static int apply_r_riscv_align_rela(struct module *me, u32 *location,
> > > > >  	return -EINVAL;
> > > > >  }
> > > > >
> > > > > +static int apply_r_riscv_add8_rela(struct module *me, u32 *location, Elf_Addr v)
> > > > > +{
> > > > > +	*(u8 *)location += (u8)v;
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  static int apply_r_riscv_add16_rela(struct module *me, u32 *location,
> > > > >  				    Elf_Addr v)
> > > > >  {
> > > > > @@ -289,6 +296,12 @@ static int apply_r_riscv_add64_rela(struct module *me, u32 *location,
> > > > >  	return 0;
> > > > >  }
> > > > >
> > > > > +static int apply_r_riscv_sub8_rela(struct module *me, u32 *location, Elf_Addr v)
> > > > > +{
> > > > > +	*(u8 *)location -= (u8)v;
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  static int apply_r_riscv_sub16_rela(struct module *me, u32 *location,
> > > > >  				    Elf_Addr v)
> > > > >  {
> > > > > @@ -310,31 +323,149 @@ static int apply_r_riscv_sub64_rela(struct module *me, u32 *location,
> > > > >  	return 0;
> > > > >  }
> > > > >
> > > > > -static int (*reloc_handlers_rela[]) (struct module *me, u32 *location,
> > > > > -				Elf_Addr v) = {
> > > > > -	[R_RISCV_32]			= apply_r_riscv_32_rela,
> > > > > -	[R_RISCV_64]			= apply_r_riscv_64_rela,
> > > > > -	[R_RISCV_BRANCH]		= apply_r_riscv_branch_rela,
> > > > > -	[R_RISCV_JAL]			= apply_r_riscv_jal_rela,
> > > > > -	[R_RISCV_RVC_BRANCH]		= apply_r_riscv_rvc_branch_rela,
> > > > > -	[R_RISCV_RVC_JUMP]		= apply_r_riscv_rvc_jump_rela,
> > > > > -	[R_RISCV_PCREL_HI20]		= apply_r_riscv_pcrel_hi20_rela,
> > > > > -	[R_RISCV_PCREL_LO12_I]		= apply_r_riscv_pcrel_lo12_i_rela,
> > > > > -	[R_RISCV_PCREL_LO12_S]		= apply_r_riscv_pcrel_lo12_s_rela,
> > > > > -	[R_RISCV_HI20]			= apply_r_riscv_hi20_rela,
> > > > > -	[R_RISCV_LO12_I]		= apply_r_riscv_lo12_i_rela,
> > > > > -	[R_RISCV_LO12_S]		= apply_r_riscv_lo12_s_rela,
> > > > > -	[R_RISCV_GOT_HI20]		= apply_r_riscv_got_hi20_rela,
> > > > > -	[R_RISCV_CALL_PLT]		= apply_r_riscv_call_plt_rela,
> > > > > -	[R_RISCV_CALL]			= apply_r_riscv_call_rela,
> > > > > -	[R_RISCV_RELAX]			= apply_r_riscv_relax_rela,
> > > > > -	[R_RISCV_ALIGN]			= apply_r_riscv_align_rela,
> > > > > -	[R_RISCV_ADD16]			= apply_r_riscv_add16_rela,
> > > > > -	[R_RISCV_ADD32]			= apply_r_riscv_add32_rela,
> > > > > -	[R_RISCV_ADD64]			= apply_r_riscv_add64_rela,
> > > > > -	[R_RISCV_SUB16]			= apply_r_riscv_sub16_rela,
> > > > > -	[R_RISCV_SUB32]			= apply_r_riscv_sub32_rela,
> > > > > -	[R_RISCV_SUB64]			= apply_r_riscv_sub64_rela,
> > > > > +static int dynamic_linking_not_supported(struct module *me, u32 *location,
> > > > > +					 Elf_Addr v)
> > > > > +{
> > > > > +	pr_err("%s: Dynamic linking not supported in kernel modules PC = %p\n",
> > > > > +	       me->name, location);
> > > > > +	return -EINVAL;
> > > > > +}
> > > > > +
> > > > > +static int tls_not_supported(struct module *me, u32 *location, Elf_Addr v)
> > > > > +{
> > > > > +	pr_err("%s: Thread local storage not supported in kernel modules PC = %p\n",
> > > > > +	       me->name, location);
> > > > > +	return -EINVAL;
> > > > > +}
> > > > > +
> > > > > +static int apply_r_riscv_sub6_rela(struct module *me, u32 *location, Elf_Addr v)
> > > > > +{
> > > > > +	*(u8 *)location = (*location - ((u8)v & 0x3F)) & 0x3F;
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int apply_r_riscv_set6_rela(struct module *me, u32 *location, Elf_Addr v)
> > > > > +{
> > > > > +	*(u8 *)location = (*(u8 *)location & 0xc0) | ((u8)v & 0x3F);
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int apply_r_riscv_set8_rela(struct module *me, u32 *location, Elf_Addr v)
> > > > > +{
> > > > > +	*(u8 *)location = (u8)v;
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int apply_r_riscv_set16_rela(struct module *me, u32 *location,
> > > > > +				    Elf_Addr v)
> > > > > +{
> > > > > +	*(u16 *)location = (u16)v;
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int apply_r_riscv_set32_rela(struct module *me, u32 *location,
> > > > > +				    Elf_Addr v)
> > > > > +{
> > > > > +	*(u32 *)location = (u32)v;
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int apply_r_riscv_32_pcrel_rela(struct module *me, u32 *location,
> > > > > +				       Elf_Addr v)
> > > > > +{
> > > > > +	*(u32 *)location = (u32)v;
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int apply_r_riscv_plt32_rela(struct module *me, u32 *location,
> > > > > +				    Elf_Addr v)
> > > > > +{
> > > > > +	*(u32 *)location = (u32)v;
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int apply_r_riscv_set_uleb128(struct module *me, u32 *location, Elf_Addr v)
> > > > > +{
> > > > > +	/*
> > > > > +	 * Relocation is only performed if R_RISCV_SET_ULEB128 is followed by
> > > > > +	 * R_RISCV_SUB_ULEB128 so do computation there
> > > > > +	 */
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int apply_r_riscv_sub_uleb128(struct module *me, u32 *location, Elf_Addr v)
> > > > > +{
> > > > > +	if (v >= 128) {
> > > > > +		pr_err("%s: uleb128 must be in [0, 127] (not %ld) at PC = %p\n",
> > > > > +		       me->name, (unsigned long)v, location);
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	*location = v;
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Relocations defined in the riscv-elf-psabi-doc.
> > > > > + * This handles static linking only.
> > > > > + */
> > > > > +static int (*reloc_handlers_rela[])(struct module *me, u32 *location,
> > > > > +				    Elf_Addr v) = {
> > > > > +	[R_RISCV_32] =			apply_r_riscv_32_rela,
> > > > > +	[R_RISCV_64] =			apply_r_riscv_64_rela,
> > > > > +	[R_RISCV_RELATIVE] =		dynamic_linking_not_supported,
> > > > > +	[R_RISCV_COPY] =		dynamic_linking_not_supported,
> > > > > +	[R_RISCV_JUMP_SLOT] =		dynamic_linking_not_supported,
> > > > > +	[R_RISCV_TLS_DTPMOD32] =	dynamic_linking_not_supported,
> > > > > +	[R_RISCV_TLS_DTPMOD64] =	dynamic_linking_not_supported,
> > > > > +	[R_RISCV_TLS_DTPREL32] =	dynamic_linking_not_supported,
> > > > > +	[R_RISCV_TLS_DTPREL64] =	dynamic_linking_not_supported,
> > > > > +	[R_RISCV_TLS_TPREL32] =		dynamic_linking_not_supported,
> > > > > +	[R_RISCV_TLS_TPREL64] =		dynamic_linking_not_supported,
> > > > > +	/* 12-15 undefined */
> > > > > +	[R_RISCV_BRANCH] =		apply_r_riscv_branch_rela,
> > > > > +	[R_RISCV_JAL] =			apply_r_riscv_jal_rela,
> > > > > +	[R_RISCV_CALL] =		apply_r_riscv_call_rela,
> > > > > +	[R_RISCV_CALL_PLT] =		apply_r_riscv_call_plt_rela,
> > > > > +	[R_RISCV_GOT_HI20] =		apply_r_riscv_got_hi20_rela,
> > > > > +	[R_RISCV_TLS_GOT_HI20] =	tls_not_supported,
> > > > > +	[R_RISCV_TLS_GD_HI20] =		tls_not_supported,
> > > > > +	[R_RISCV_PCREL_HI20] =		apply_r_riscv_pcrel_hi20_rela,
> > > > > +	[R_RISCV_PCREL_LO12_I] =	apply_r_riscv_pcrel_lo12_i_rela,
> > > > > +	[R_RISCV_PCREL_LO12_S] =	apply_r_riscv_pcrel_lo12_s_rela,
> > > > > +	[R_RISCV_HI20] =		apply_r_riscv_hi20_rela,
> > > > > +	[R_RISCV_LO12_I] =		apply_r_riscv_lo12_i_rela,
> > > > > +	[R_RISCV_LO12_S] =		apply_r_riscv_lo12_s_rela,
> > > > > +	[R_RISCV_TPREL_HI20] =		tls_not_supported,
> > > > > +	[R_RISCV_TPREL_LO12_I] =	tls_not_supported,
> > > > > +	[R_RISCV_TPREL_LO12_S] =	tls_not_supported,
> > > > > +	[R_RISCV_TPREL_ADD] =		tls_not_supported,
> > > > > +	[R_RISCV_ADD8] =		apply_r_riscv_add8_rela,
> > > > > +	[R_RISCV_ADD16] =		apply_r_riscv_add16_rela,
> > > > > +	[R_RISCV_ADD32] =		apply_r_riscv_add32_rela,
> > > > > +	[R_RISCV_ADD64] =		apply_r_riscv_add64_rela,
> > > > > +	[R_RISCV_SUB8] =		apply_r_riscv_sub8_rela,
> > > > > +	[R_RISCV_SUB16] =		apply_r_riscv_sub16_rela,
> > > > > +	[R_RISCV_SUB32] =		apply_r_riscv_sub32_rela,
> > > > > +	[R_RISCV_SUB64] =		apply_r_riscv_sub64_rela,
> > > > > +	/* 41-42 reserved for future standard use */
> > > > > +	[R_RISCV_ALIGN] =		apply_r_riscv_align_rela,
> > > > > +	[R_RISCV_RVC_BRANCH] =		apply_r_riscv_rvc_branch_rela,
> > > > > +	[R_RISCV_RVC_JUMP] =		apply_r_riscv_rvc_jump_rela,
> > > > > +	/* 46-50 reserved for future standard use */
> > > > > +	[R_RISCV_RELAX] =		apply_r_riscv_relax_rela,
> > > > > +	[R_RISCV_SUB6] =		apply_r_riscv_sub6_rela,
> > > > > +	[R_RISCV_SET6] =		apply_r_riscv_set6_rela,
> > > > > +	[R_RISCV_SET8] =		apply_r_riscv_set8_rela,
> > > > > +	[R_RISCV_SET16] =		apply_r_riscv_set16_rela,
> > > > > +	[R_RISCV_SET32] =		apply_r_riscv_set32_rela,
> > > > > +	[R_RISCV_32_PCREL] =		apply_r_riscv_32_pcrel_rela,
> > > > > +	[R_RISCV_IRELATIVE] =		dynamic_linking_not_supported,
> > > > > +	[R_RISCV_PLT32] =		apply_r_riscv_plt32_rela,
> > > > > +	[R_RISCV_SET_ULEB128] =		apply_r_riscv_set_uleb128,
> > > > > +	[R_RISCV_SUB_ULEB128] =		apply_r_riscv_sub_uleb128,
> > > > > +	/* 62-191 reserved for future standard use */
> > > > > +	/* 192-255 nonstandard ABI extensions  */
> > > > >  };
> > > >
> > > > Hi Charlie,
> > > >
> > > > This is not a critique of this patch, but all these callbacks take a
> > > > u32 *location and
> > > > because of the compressed instructions this pointer may not be
> > > > aligned, so a lot of
> > > > the callbacks end up doing unaligned access which may fault to an
> > > > M-mode handler on
> > > > some platforms.
> > > >
> > > > I once sent a patch to fix this:
> > > > https://lore.kernel.org/linux-riscv/20220224152456.493365-2-kernel@esmil.dk/
> > > >
> > > > Maybe that's something you want to look into while touching this code anyway.
> > > >
> > > > /Emil
> > >
> > > Oh nice, I will pick up that patch and change the "native-endian"
> > > wording to be "little-endian" in the commit.
> >
> > Great, thanks. You'll probably also want the reads to be wrapped in
> > le16_to_cpu() and similar when writing now that it's decided that the parcels
> > are always in little-endian byteorder.
> >
> > /Emil
>
> I believe that le16_to_cpu() is only needed when instructions are being modified, and
> the relocations that only touch data can be left alone. Is this correct?

Yes, that sounds right to me. I only meant in the riscv_insn_rmw() function of
my patch and the callbacks modifying a compressed instruction, but I haven't
gone through all the other reloc types to check if they have a set endianess.

/Emil


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

* Re: [PATCH v4 1/2] riscv: Add remaining module relocations
  2023-10-18  5:34 ` [PATCH v4 1/2] riscv: Add remaining module relocations Charlie Jenkins
  2023-10-18 12:17   ` Emil Renner Berthing
@ 2023-10-18 18:28   ` Samuel Holland
  2023-10-18 22:19     ` Charlie Jenkins
  2023-10-18 18:47   ` Andreas Schwab
  2 siblings, 1 reply; 16+ messages in thread
From: Samuel Holland @ 2023-10-18 18:28 UTC (permalink / raw)
  To: Charlie Jenkins
  Cc: Eric Biederman, Kees Cook, Paul Walmsley, Palmer Dabbelt,
	Albert Ou, linux-riscv, linux-mm, linux-kernel

On 2023-10-18 12:34 AM, Charlie Jenkins wrote:
> Add all final module relocations and add error logs explaining the ones
> that are not supported.
> 
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
>  arch/riscv/include/uapi/asm/elf.h |   5 +-
>  arch/riscv/kernel/module.c        | 207 +++++++++++++++++++++++++++++++++-----
>  2 files changed, 186 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/riscv/include/uapi/asm/elf.h b/arch/riscv/include/uapi/asm/elf.h
> index d696d6610231..11a71b8533d5 100644
> --- a/arch/riscv/include/uapi/asm/elf.h
> +++ b/arch/riscv/include/uapi/asm/elf.h
> @@ -49,6 +49,7 @@ typedef union __riscv_fp_state elf_fpregset_t;
>  #define R_RISCV_TLS_DTPREL64	9
>  #define R_RISCV_TLS_TPREL32	10
>  #define R_RISCV_TLS_TPREL64	11
> +#define R_RISCV_IRELATIVE	58
>  
>  /* Relocation types not used by the dynamic linker */
>  #define R_RISCV_BRANCH		16
> @@ -81,7 +82,6 @@ typedef union __riscv_fp_state elf_fpregset_t;
>  #define R_RISCV_ALIGN		43
>  #define R_RISCV_RVC_BRANCH	44
>  #define R_RISCV_RVC_JUMP	45
> -#define R_RISCV_LUI		46
>  #define R_RISCV_GPREL_I		47
>  #define R_RISCV_GPREL_S		48
>  #define R_RISCV_TPREL_I		49
> @@ -93,6 +93,9 @@ typedef union __riscv_fp_state elf_fpregset_t;
>  #define R_RISCV_SET16		55
>  #define R_RISCV_SET32		56
>  #define R_RISCV_32_PCREL	57
> +#define R_RISCV_PLT32		59
> +#define R_RISCV_SET_ULEB128	60
> +#define R_RISCV_SUB_ULEB128	61
>  
>  
>  #endif /* _UAPI_ASM_RISCV_ELF_H */
> diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> index 7c651d55fcbd..e860726352ac 100644
> --- a/arch/riscv/kernel/module.c
> +++ b/arch/riscv/kernel/module.c
> @@ -7,6 +7,7 @@
>  #include <linux/elf.h>
>  #include <linux/err.h>
>  #include <linux/errno.h>
> +#include <linux/kernel.h>
>  #include <linux/moduleloader.h>
>  #include <linux/vmalloc.h>
>  #include <linux/sizes.h>
> @@ -268,6 +269,12 @@ static int apply_r_riscv_align_rela(struct module *me, u32 *location,
>  	return -EINVAL;
>  }
>  
> +static int apply_r_riscv_add8_rela(struct module *me, u32 *location, Elf_Addr v)
> +{
> +	*(u8 *)location += (u8)v;
> +	return 0;
> +}
> +
>  static int apply_r_riscv_add16_rela(struct module *me, u32 *location,
>  				    Elf_Addr v)
>  {
> @@ -289,6 +296,12 @@ static int apply_r_riscv_add64_rela(struct module *me, u32 *location,
>  	return 0;
>  }
>  
> +static int apply_r_riscv_sub8_rela(struct module *me, u32 *location, Elf_Addr v)
> +{
> +	*(u8 *)location -= (u8)v;
> +	return 0;
> +}
> +
>  static int apply_r_riscv_sub16_rela(struct module *me, u32 *location,
>  				    Elf_Addr v)
>  {
> @@ -310,31 +323,149 @@ static int apply_r_riscv_sub64_rela(struct module *me, u32 *location,
>  	return 0;
>  }
>  
> -static int (*reloc_handlers_rela[]) (struct module *me, u32 *location,
> -				Elf_Addr v) = {
> -	[R_RISCV_32]			= apply_r_riscv_32_rela,
> -	[R_RISCV_64]			= apply_r_riscv_64_rela,
> -	[R_RISCV_BRANCH]		= apply_r_riscv_branch_rela,
> -	[R_RISCV_JAL]			= apply_r_riscv_jal_rela,
> -	[R_RISCV_RVC_BRANCH]		= apply_r_riscv_rvc_branch_rela,
> -	[R_RISCV_RVC_JUMP]		= apply_r_riscv_rvc_jump_rela,
> -	[R_RISCV_PCREL_HI20]		= apply_r_riscv_pcrel_hi20_rela,
> -	[R_RISCV_PCREL_LO12_I]		= apply_r_riscv_pcrel_lo12_i_rela,
> -	[R_RISCV_PCREL_LO12_S]		= apply_r_riscv_pcrel_lo12_s_rela,
> -	[R_RISCV_HI20]			= apply_r_riscv_hi20_rela,
> -	[R_RISCV_LO12_I]		= apply_r_riscv_lo12_i_rela,
> -	[R_RISCV_LO12_S]		= apply_r_riscv_lo12_s_rela,
> -	[R_RISCV_GOT_HI20]		= apply_r_riscv_got_hi20_rela,
> -	[R_RISCV_CALL_PLT]		= apply_r_riscv_call_plt_rela,
> -	[R_RISCV_CALL]			= apply_r_riscv_call_rela,
> -	[R_RISCV_RELAX]			= apply_r_riscv_relax_rela,
> -	[R_RISCV_ALIGN]			= apply_r_riscv_align_rela,
> -	[R_RISCV_ADD16]			= apply_r_riscv_add16_rela,
> -	[R_RISCV_ADD32]			= apply_r_riscv_add32_rela,
> -	[R_RISCV_ADD64]			= apply_r_riscv_add64_rela,
> -	[R_RISCV_SUB16]			= apply_r_riscv_sub16_rela,
> -	[R_RISCV_SUB32]			= apply_r_riscv_sub32_rela,
> -	[R_RISCV_SUB64]			= apply_r_riscv_sub64_rela,
> +static int dynamic_linking_not_supported(struct module *me, u32 *location,
> +					 Elf_Addr v)
> +{
> +	pr_err("%s: Dynamic linking not supported in kernel modules PC = %p\n",
> +	       me->name, location);
> +	return -EINVAL;
> +}
> +
> +static int tls_not_supported(struct module *me, u32 *location, Elf_Addr v)
> +{
> +	pr_err("%s: Thread local storage not supported in kernel modules PC = %p\n",
> +	       me->name, location);
> +	return -EINVAL;
> +}
> +
> +static int apply_r_riscv_sub6_rela(struct module *me, u32 *location, Elf_Addr v)
> +{
> +	*(u8 *)location = (*location - ((u8)v & 0x3F)) & 0x3F;
> +	return 0;
> +}
> +
> +static int apply_r_riscv_set6_rela(struct module *me, u32 *location, Elf_Addr v)
> +{
> +	*(u8 *)location = (*(u8 *)location & 0xc0) | ((u8)v & 0x3F);
> +	return 0;
> +}
> +
> +static int apply_r_riscv_set8_rela(struct module *me, u32 *location, Elf_Addr v)
> +{
> +	*(u8 *)location = (u8)v;
> +	return 0;
> +}
> +
> +static int apply_r_riscv_set16_rela(struct module *me, u32 *location,
> +				    Elf_Addr v)
> +{
> +	*(u16 *)location = (u16)v;
> +	return 0;
> +}
> +
> +static int apply_r_riscv_set32_rela(struct module *me, u32 *location,
> +				    Elf_Addr v)
> +{
> +	*(u32 *)location = (u32)v;

You don't need to cast the pointer, since it's already a `u32 *`.

> +	return 0;
> +}
> +
> +static int apply_r_riscv_32_pcrel_rela(struct module *me, u32 *location,
> +				       Elf_Addr v)
> +{
> +	*(u32 *)location = (u32)v;

This expression should be:

    *location = v - location;

matching the other PC-relative relocations.

(BTW, recent clang generates these relocations for module alternatives.)

> +	return 0;
> +}
> +
> +static int apply_r_riscv_plt32_rela(struct module *me, u32 *location,
> +				    Elf_Addr v)
> +{
> +	*(u32 *)location = (u32)v;

This should look like apply_r_riscv_32_pcrel_rela(), but with the PLT entry
emission code from apply_r_riscv_call_plt_rela(). See the psABI commit[1].

[1]:
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/commit/c3f8269c56d8a2f56c2602f2a44175362024ef9c

> +	return 0;
> +}
> +
> +static int apply_r_riscv_set_uleb128(struct module *me, u32 *location, Elf_Addr v)
> +{
> +	/*
> +	 * Relocation is only performed if R_RISCV_SET_ULEB128 is followed by
> +	 * R_RISCV_SUB_ULEB128 so do computation there
> +	 */
> +	return 0;
> +}
> +
> +static int apply_r_riscv_sub_uleb128(struct module *me, u32 *location, Elf_Addr v)
> +{
> +	if (v >= 128) {
> +		pr_err("%s: uleb128 must be in [0, 127] (not %ld) at PC = %p\n",
> +		       me->name, (unsigned long)v, location);
> +		return -EINVAL;
> +	}
> +
> +	*location = v;
> +	return 0;
> +}
> +
> +/*
> + * Relocations defined in the riscv-elf-psabi-doc.
> + * This handles static linking only.
> + */
> +static int (*reloc_handlers_rela[])(struct module *me, u32 *location,
> +				    Elf_Addr v) = {
> +	[R_RISCV_32] =			apply_r_riscv_32_rela,
> +	[R_RISCV_64] =			apply_r_riscv_64_rela,
> +	[R_RISCV_RELATIVE] =		dynamic_linking_not_supported,
> +	[R_RISCV_COPY] =		dynamic_linking_not_supported,
> +	[R_RISCV_JUMP_SLOT] =		dynamic_linking_not_supported,
> +	[R_RISCV_TLS_DTPMOD32] =	dynamic_linking_not_supported,
> +	[R_RISCV_TLS_DTPMOD64] =	dynamic_linking_not_supported,
> +	[R_RISCV_TLS_DTPREL32] =	dynamic_linking_not_supported,
> +	[R_RISCV_TLS_DTPREL64] =	dynamic_linking_not_supported,
> +	[R_RISCV_TLS_TPREL32] =		dynamic_linking_not_supported,
> +	[R_RISCV_TLS_TPREL64] =		dynamic_linking_not_supported,
> +	/* 12-15 undefined */
> +	[R_RISCV_BRANCH] =		apply_r_riscv_branch_rela,
> +	[R_RISCV_JAL] =			apply_r_riscv_jal_rela,
> +	[R_RISCV_CALL] =		apply_r_riscv_call_rela,
> +	[R_RISCV_CALL_PLT] =		apply_r_riscv_call_plt_rela,
> +	[R_RISCV_GOT_HI20] =		apply_r_riscv_got_hi20_rela,
> +	[R_RISCV_TLS_GOT_HI20] =	tls_not_supported,
> +	[R_RISCV_TLS_GD_HI20] =		tls_not_supported,
> +	[R_RISCV_PCREL_HI20] =		apply_r_riscv_pcrel_hi20_rela,
> +	[R_RISCV_PCREL_LO12_I] =	apply_r_riscv_pcrel_lo12_i_rela,
> +	[R_RISCV_PCREL_LO12_S] =	apply_r_riscv_pcrel_lo12_s_rela,
> +	[R_RISCV_HI20] =		apply_r_riscv_hi20_rela,
> +	[R_RISCV_LO12_I] =		apply_r_riscv_lo12_i_rela,
> +	[R_RISCV_LO12_S] =		apply_r_riscv_lo12_s_rela,
> +	[R_RISCV_TPREL_HI20] =		tls_not_supported,
> +	[R_RISCV_TPREL_LO12_I] =	tls_not_supported,
> +	[R_RISCV_TPREL_LO12_S] =	tls_not_supported,
> +	[R_RISCV_TPREL_ADD] =		tls_not_supported,
> +	[R_RISCV_ADD8] =		apply_r_riscv_add8_rela,
> +	[R_RISCV_ADD16] =		apply_r_riscv_add16_rela,
> +	[R_RISCV_ADD32] =		apply_r_riscv_add32_rela,
> +	[R_RISCV_ADD64] =		apply_r_riscv_add64_rela,
> +	[R_RISCV_SUB8] =		apply_r_riscv_sub8_rela,
> +	[R_RISCV_SUB16] =		apply_r_riscv_sub16_rela,
> +	[R_RISCV_SUB32] =		apply_r_riscv_sub32_rela,
> +	[R_RISCV_SUB64] =		apply_r_riscv_sub64_rela,
> +	/* 41-42 reserved for future standard use */
> +	[R_RISCV_ALIGN] =		apply_r_riscv_align_rela,
> +	[R_RISCV_RVC_BRANCH] =		apply_r_riscv_rvc_branch_rela,
> +	[R_RISCV_RVC_JUMP] =		apply_r_riscv_rvc_jump_rela,
> +	/* 46-50 reserved for future standard use */
> +	[R_RISCV_RELAX] =		apply_r_riscv_relax_rela,
> +	[R_RISCV_SUB6] =		apply_r_riscv_sub6_rela,
> +	[R_RISCV_SET6] =		apply_r_riscv_set6_rela,
> +	[R_RISCV_SET8] =		apply_r_riscv_set8_rela,
> +	[R_RISCV_SET16] =		apply_r_riscv_set16_rela,
> +	[R_RISCV_SET32] =		apply_r_riscv_set32_rela,
> +	[R_RISCV_32_PCREL] =		apply_r_riscv_32_pcrel_rela,
> +	[R_RISCV_IRELATIVE] =		dynamic_linking_not_supported,
> +	[R_RISCV_PLT32] =		apply_r_riscv_plt32_rela,
> +	[R_RISCV_SET_ULEB128] =		apply_r_riscv_set_uleb128,
> +	[R_RISCV_SUB_ULEB128] =		apply_r_riscv_sub_uleb128,
> +	/* 62-191 reserved for future standard use */
> +	/* 192-255 nonstandard ABI extensions  */
>  };
>  
>  int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> @@ -348,6 +479,10 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
>  	unsigned int i, type;
>  	Elf_Addr v;
>  	int res;
> +	bool uleb128_set_exists = false;
> +	u32 *uleb128_set_loc;
> +	unsigned long uleb128_set_sym_val;
> +

Extra blank line.

>  
>  	pr_debug("Applying relocate section %u to %u\n", relsec,
>  	       sechdrs[relsec].sh_info);
> @@ -425,6 +560,28 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
>  				  me->name);
>  				return -EINVAL;
>  			}
> +		} else if (type == R_RISCV_SET_ULEB128) {
> +			if (uleb128_set_exists) {
> +				pr_err("%s: riscv psABI requires the next ULEB128 relocation to come after a R_RISCV_SET_ULEB128 is an R_RISCV_SUB_ULEB128, not another R_RISCV_SET_ULEB128.\n",
> +				       me->name);
> +				return -EINVAL;
> +			}
> +			uleb128_set_exists = true;
> +			uleb128_set_loc = location;
> +			uleb128_set_sym_val =
> +				((Elf_Sym *)sechdrs[symindex].sh_addr +
> +					ELF_RISCV_R_SYM(rel[i].r_info))
> +					->st_value +
> +				rel[i].r_addend;
> +		} else if (type == R_RISCV_SUB_ULEB128) {
> +			if (uleb128_set_exists && uleb128_set_loc == location) {
> +				/* Calculate set and subtraction */
> +				v = uleb128_set_sym_val - v;

You need to set uleb128_set_exists back to false somewhere, or you can only
handle one R_RISCV_SET_ULEB128 relocation per module.

Regards,
Samuel

> +			} else {
> +				pr_err("%s: R_RISCV_SUB_ULEB128 must always be paired with the first R_RISCV_SET_ULEB128 that comes before it. PC = %p\n",
> +				       me->name, location);
> +				return -EINVAL;
> +			}
>  		}
>  
>  		res = handler(me, location, v);
> 



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

* Re: [PATCH v4 1/2] riscv: Add remaining module relocations
  2023-10-18 18:28   ` Samuel Holland
@ 2023-10-18 22:19     ` Charlie Jenkins
  0 siblings, 0 replies; 16+ messages in thread
From: Charlie Jenkins @ 2023-10-18 22:19 UTC (permalink / raw)
  To: Samuel Holland
  Cc: Eric Biederman, Kees Cook, Paul Walmsley, Palmer Dabbelt,
	Albert Ou, linux-riscv, linux-mm, linux-kernel

On Wed, Oct 18, 2023 at 01:28:45PM -0500, Samuel Holland wrote:
> On 2023-10-18 12:34 AM, Charlie Jenkins wrote:
> > Add all final module relocations and add error logs explaining the ones
> > that are not supported.
> > 
> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > ---
> >  arch/riscv/include/uapi/asm/elf.h |   5 +-
> >  arch/riscv/kernel/module.c        | 207 +++++++++++++++++++++++++++++++++-----
> >  2 files changed, 186 insertions(+), 26 deletions(-)
> > 
> > diff --git a/arch/riscv/include/uapi/asm/elf.h b/arch/riscv/include/uapi/asm/elf.h
> > index d696d6610231..11a71b8533d5 100644
> > --- a/arch/riscv/include/uapi/asm/elf.h
> > +++ b/arch/riscv/include/uapi/asm/elf.h
> > @@ -49,6 +49,7 @@ typedef union __riscv_fp_state elf_fpregset_t;
> >  #define R_RISCV_TLS_DTPREL64	9
> >  #define R_RISCV_TLS_TPREL32	10
> >  #define R_RISCV_TLS_TPREL64	11
> > +#define R_RISCV_IRELATIVE	58
> >  
> >  /* Relocation types not used by the dynamic linker */
> >  #define R_RISCV_BRANCH		16
> > @@ -81,7 +82,6 @@ typedef union __riscv_fp_state elf_fpregset_t;
> >  #define R_RISCV_ALIGN		43
> >  #define R_RISCV_RVC_BRANCH	44
> >  #define R_RISCV_RVC_JUMP	45
> > -#define R_RISCV_LUI		46
> >  #define R_RISCV_GPREL_I		47
> >  #define R_RISCV_GPREL_S		48
> >  #define R_RISCV_TPREL_I		49
> > @@ -93,6 +93,9 @@ typedef union __riscv_fp_state elf_fpregset_t;
> >  #define R_RISCV_SET16		55
> >  #define R_RISCV_SET32		56
> >  #define R_RISCV_32_PCREL	57
> > +#define R_RISCV_PLT32		59
> > +#define R_RISCV_SET_ULEB128	60
> > +#define R_RISCV_SUB_ULEB128	61
> >  
> >  
> >  #endif /* _UAPI_ASM_RISCV_ELF_H */
> > diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> > index 7c651d55fcbd..e860726352ac 100644
> > --- a/arch/riscv/kernel/module.c
> > +++ b/arch/riscv/kernel/module.c
> > @@ -7,6 +7,7 @@
> >  #include <linux/elf.h>
> >  #include <linux/err.h>
> >  #include <linux/errno.h>
> > +#include <linux/kernel.h>
> >  #include <linux/moduleloader.h>
> >  #include <linux/vmalloc.h>
> >  #include <linux/sizes.h>
> > @@ -268,6 +269,12 @@ static int apply_r_riscv_align_rela(struct module *me, u32 *location,
> >  	return -EINVAL;
> >  }
> >  
> > +static int apply_r_riscv_add8_rela(struct module *me, u32 *location, Elf_Addr v)
> > +{
> > +	*(u8 *)location += (u8)v;
> > +	return 0;
> > +}
> > +
> >  static int apply_r_riscv_add16_rela(struct module *me, u32 *location,
> >  				    Elf_Addr v)
> >  {
> > @@ -289,6 +296,12 @@ static int apply_r_riscv_add64_rela(struct module *me, u32 *location,
> >  	return 0;
> >  }
> >  
> > +static int apply_r_riscv_sub8_rela(struct module *me, u32 *location, Elf_Addr v)
> > +{
> > +	*(u8 *)location -= (u8)v;
> > +	return 0;
> > +}
> > +
> >  static int apply_r_riscv_sub16_rela(struct module *me, u32 *location,
> >  				    Elf_Addr v)
> >  {
> > @@ -310,31 +323,149 @@ static int apply_r_riscv_sub64_rela(struct module *me, u32 *location,
> >  	return 0;
> >  }
> >  
> > -static int (*reloc_handlers_rela[]) (struct module *me, u32 *location,
> > -				Elf_Addr v) = {
> > -	[R_RISCV_32]			= apply_r_riscv_32_rela,
> > -	[R_RISCV_64]			= apply_r_riscv_64_rela,
> > -	[R_RISCV_BRANCH]		= apply_r_riscv_branch_rela,
> > -	[R_RISCV_JAL]			= apply_r_riscv_jal_rela,
> > -	[R_RISCV_RVC_BRANCH]		= apply_r_riscv_rvc_branch_rela,
> > -	[R_RISCV_RVC_JUMP]		= apply_r_riscv_rvc_jump_rela,
> > -	[R_RISCV_PCREL_HI20]		= apply_r_riscv_pcrel_hi20_rela,
> > -	[R_RISCV_PCREL_LO12_I]		= apply_r_riscv_pcrel_lo12_i_rela,
> > -	[R_RISCV_PCREL_LO12_S]		= apply_r_riscv_pcrel_lo12_s_rela,
> > -	[R_RISCV_HI20]			= apply_r_riscv_hi20_rela,
> > -	[R_RISCV_LO12_I]		= apply_r_riscv_lo12_i_rela,
> > -	[R_RISCV_LO12_S]		= apply_r_riscv_lo12_s_rela,
> > -	[R_RISCV_GOT_HI20]		= apply_r_riscv_got_hi20_rela,
> > -	[R_RISCV_CALL_PLT]		= apply_r_riscv_call_plt_rela,
> > -	[R_RISCV_CALL]			= apply_r_riscv_call_rela,
> > -	[R_RISCV_RELAX]			= apply_r_riscv_relax_rela,
> > -	[R_RISCV_ALIGN]			= apply_r_riscv_align_rela,
> > -	[R_RISCV_ADD16]			= apply_r_riscv_add16_rela,
> > -	[R_RISCV_ADD32]			= apply_r_riscv_add32_rela,
> > -	[R_RISCV_ADD64]			= apply_r_riscv_add64_rela,
> > -	[R_RISCV_SUB16]			= apply_r_riscv_sub16_rela,
> > -	[R_RISCV_SUB32]			= apply_r_riscv_sub32_rela,
> > -	[R_RISCV_SUB64]			= apply_r_riscv_sub64_rela,
> > +static int dynamic_linking_not_supported(struct module *me, u32 *location,
> > +					 Elf_Addr v)
> > +{
> > +	pr_err("%s: Dynamic linking not supported in kernel modules PC = %p\n",
> > +	       me->name, location);
> > +	return -EINVAL;
> > +}
> > +
> > +static int tls_not_supported(struct module *me, u32 *location, Elf_Addr v)
> > +{
> > +	pr_err("%s: Thread local storage not supported in kernel modules PC = %p\n",
> > +	       me->name, location);
> > +	return -EINVAL;
> > +}
> > +
> > +static int apply_r_riscv_sub6_rela(struct module *me, u32 *location, Elf_Addr v)
> > +{
> > +	*(u8 *)location = (*location - ((u8)v & 0x3F)) & 0x3F;
> > +	return 0;
> > +}
> > +
> > +static int apply_r_riscv_set6_rela(struct module *me, u32 *location, Elf_Addr v)
> > +{
> > +	*(u8 *)location = (*(u8 *)location & 0xc0) | ((u8)v & 0x3F);
> > +	return 0;
> > +}
> > +
> > +static int apply_r_riscv_set8_rela(struct module *me, u32 *location, Elf_Addr v)
> > +{
> > +	*(u8 *)location = (u8)v;
> > +	return 0;
> > +}
> > +
> > +static int apply_r_riscv_set16_rela(struct module *me, u32 *location,
> > +				    Elf_Addr v)
> > +{
> > +	*(u16 *)location = (u16)v;
> > +	return 0;
> > +}
> > +
> > +static int apply_r_riscv_set32_rela(struct module *me, u32 *location,
> > +				    Elf_Addr v)
> > +{
> > +	*(u32 *)location = (u32)v;
> 
> You don't need to cast the pointer, since it's already a `u32 *`.
> 
> > +	return 0;
> > +}
> > +
> > +static int apply_r_riscv_32_pcrel_rela(struct module *me, u32 *location,
> > +				       Elf_Addr v)
> > +{
> > +	*(u32 *)location = (u32)v;
> 
> This expression should be:
> 
>     *location = v - location;
> 
> matching the other PC-relative relocations.
> 
> (BTW, recent clang generates these relocations for module alternatives.)
> 
> > +	return 0;
> > +}
> > +
> > +static int apply_r_riscv_plt32_rela(struct module *me, u32 *location,
> > +				    Elf_Addr v)
> > +{
> > +	*(u32 *)location = (u32)v;
> 
> This should look like apply_r_riscv_32_pcrel_rela(), but with the PLT entry
> emission code from apply_r_riscv_call_plt_rela(). See the psABI commit[1].
> 
> [1]:
> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/commit/c3f8269c56d8a2f56c2602f2a44175362024ef9c
> 

Thank you for pointing out these issues.

> > +	return 0;
> > +}
> > +
> > +static int apply_r_riscv_set_uleb128(struct module *me, u32 *location, Elf_Addr v)
> > +{
> > +	/*
> > +	 * Relocation is only performed if R_RISCV_SET_ULEB128 is followed by
> > +	 * R_RISCV_SUB_ULEB128 so do computation there
> > +	 */
> > +	return 0;
> > +}
> > +
> > +static int apply_r_riscv_sub_uleb128(struct module *me, u32 *location, Elf_Addr v)
> > +{
> > +	if (v >= 128) {
> > +		pr_err("%s: uleb128 must be in [0, 127] (not %ld) at PC = %p\n",
> > +		       me->name, (unsigned long)v, location);
> > +		return -EINVAL;
> > +	}
> > +
> > +	*location = v;
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Relocations defined in the riscv-elf-psabi-doc.
> > + * This handles static linking only.
> > + */
> > +static int (*reloc_handlers_rela[])(struct module *me, u32 *location,
> > +				    Elf_Addr v) = {
> > +	[R_RISCV_32] =			apply_r_riscv_32_rela,
> > +	[R_RISCV_64] =			apply_r_riscv_64_rela,
> > +	[R_RISCV_RELATIVE] =		dynamic_linking_not_supported,
> > +	[R_RISCV_COPY] =		dynamic_linking_not_supported,
> > +	[R_RISCV_JUMP_SLOT] =		dynamic_linking_not_supported,
> > +	[R_RISCV_TLS_DTPMOD32] =	dynamic_linking_not_supported,
> > +	[R_RISCV_TLS_DTPMOD64] =	dynamic_linking_not_supported,
> > +	[R_RISCV_TLS_DTPREL32] =	dynamic_linking_not_supported,
> > +	[R_RISCV_TLS_DTPREL64] =	dynamic_linking_not_supported,
> > +	[R_RISCV_TLS_TPREL32] =		dynamic_linking_not_supported,
> > +	[R_RISCV_TLS_TPREL64] =		dynamic_linking_not_supported,
> > +	/* 12-15 undefined */
> > +	[R_RISCV_BRANCH] =		apply_r_riscv_branch_rela,
> > +	[R_RISCV_JAL] =			apply_r_riscv_jal_rela,
> > +	[R_RISCV_CALL] =		apply_r_riscv_call_rela,
> > +	[R_RISCV_CALL_PLT] =		apply_r_riscv_call_plt_rela,
> > +	[R_RISCV_GOT_HI20] =		apply_r_riscv_got_hi20_rela,
> > +	[R_RISCV_TLS_GOT_HI20] =	tls_not_supported,
> > +	[R_RISCV_TLS_GD_HI20] =		tls_not_supported,
> > +	[R_RISCV_PCREL_HI20] =		apply_r_riscv_pcrel_hi20_rela,
> > +	[R_RISCV_PCREL_LO12_I] =	apply_r_riscv_pcrel_lo12_i_rela,
> > +	[R_RISCV_PCREL_LO12_S] =	apply_r_riscv_pcrel_lo12_s_rela,
> > +	[R_RISCV_HI20] =		apply_r_riscv_hi20_rela,
> > +	[R_RISCV_LO12_I] =		apply_r_riscv_lo12_i_rela,
> > +	[R_RISCV_LO12_S] =		apply_r_riscv_lo12_s_rela,
> > +	[R_RISCV_TPREL_HI20] =		tls_not_supported,
> > +	[R_RISCV_TPREL_LO12_I] =	tls_not_supported,
> > +	[R_RISCV_TPREL_LO12_S] =	tls_not_supported,
> > +	[R_RISCV_TPREL_ADD] =		tls_not_supported,
> > +	[R_RISCV_ADD8] =		apply_r_riscv_add8_rela,
> > +	[R_RISCV_ADD16] =		apply_r_riscv_add16_rela,
> > +	[R_RISCV_ADD32] =		apply_r_riscv_add32_rela,
> > +	[R_RISCV_ADD64] =		apply_r_riscv_add64_rela,
> > +	[R_RISCV_SUB8] =		apply_r_riscv_sub8_rela,
> > +	[R_RISCV_SUB16] =		apply_r_riscv_sub16_rela,
> > +	[R_RISCV_SUB32] =		apply_r_riscv_sub32_rela,
> > +	[R_RISCV_SUB64] =		apply_r_riscv_sub64_rela,
> > +	/* 41-42 reserved for future standard use */
> > +	[R_RISCV_ALIGN] =		apply_r_riscv_align_rela,
> > +	[R_RISCV_RVC_BRANCH] =		apply_r_riscv_rvc_branch_rela,
> > +	[R_RISCV_RVC_JUMP] =		apply_r_riscv_rvc_jump_rela,
> > +	/* 46-50 reserved for future standard use */
> > +	[R_RISCV_RELAX] =		apply_r_riscv_relax_rela,
> > +	[R_RISCV_SUB6] =		apply_r_riscv_sub6_rela,
> > +	[R_RISCV_SET6] =		apply_r_riscv_set6_rela,
> > +	[R_RISCV_SET8] =		apply_r_riscv_set8_rela,
> > +	[R_RISCV_SET16] =		apply_r_riscv_set16_rela,
> > +	[R_RISCV_SET32] =		apply_r_riscv_set32_rela,
> > +	[R_RISCV_32_PCREL] =		apply_r_riscv_32_pcrel_rela,
> > +	[R_RISCV_IRELATIVE] =		dynamic_linking_not_supported,
> > +	[R_RISCV_PLT32] =		apply_r_riscv_plt32_rela,
> > +	[R_RISCV_SET_ULEB128] =		apply_r_riscv_set_uleb128,
> > +	[R_RISCV_SUB_ULEB128] =		apply_r_riscv_sub_uleb128,
> > +	/* 62-191 reserved for future standard use */
> > +	/* 192-255 nonstandard ABI extensions  */
> >  };
> >  
> >  int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> > @@ -348,6 +479,10 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> >  	unsigned int i, type;
> >  	Elf_Addr v;
> >  	int res;
> > +	bool uleb128_set_exists = false;
> > +	u32 *uleb128_set_loc;
> > +	unsigned long uleb128_set_sym_val;
> > +
> 
> Extra blank line.
> 
> >  
> >  	pr_debug("Applying relocate section %u to %u\n", relsec,
> >  	       sechdrs[relsec].sh_info);
> > @@ -425,6 +560,28 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> >  				  me->name);
> >  				return -EINVAL;
> >  			}
> > +		} else if (type == R_RISCV_SET_ULEB128) {
> > +			if (uleb128_set_exists) {
> > +				pr_err("%s: riscv psABI requires the next ULEB128 relocation to come after a R_RISCV_SET_ULEB128 is an R_RISCV_SUB_ULEB128, not another R_RISCV_SET_ULEB128.\n",
> > +				       me->name);
> > +				return -EINVAL;
> > +			}
> > +			uleb128_set_exists = true;
> > +			uleb128_set_loc = location;
> > +			uleb128_set_sym_val =
> > +				((Elf_Sym *)sechdrs[symindex].sh_addr +
> > +					ELF_RISCV_R_SYM(rel[i].r_info))
> > +					->st_value +
> > +				rel[i].r_addend;
> > +		} else if (type == R_RISCV_SUB_ULEB128) {
> > +			if (uleb128_set_exists && uleb128_set_loc == location) {
> > +				/* Calculate set and subtraction */
> > +				v = uleb128_set_sym_val - v;
> 
> You need to set uleb128_set_exists back to false somewhere, or you can only
> handle one R_RISCV_SET_ULEB128 relocation per module.
> 

I guess that never made it out of my thoughts and into the code...

- Charlie

> Regards,
> Samuel
> 
> > +			} else {
> > +				pr_err("%s: R_RISCV_SUB_ULEB128 must always be paired with the first R_RISCV_SET_ULEB128 that comes before it. PC = %p\n",
> > +				       me->name, location);
> > +				return -EINVAL;
> > +			}
> >  		}
> >  
> >  		res = handler(me, location, v);
> > 
> 


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

* Re: [PATCH v4 1/2] riscv: Add remaining module relocations
  2023-10-18  5:34 ` [PATCH v4 1/2] riscv: Add remaining module relocations Charlie Jenkins
  2023-10-18 12:17   ` Emil Renner Berthing
  2023-10-18 18:28   ` Samuel Holland
@ 2023-10-18 18:47   ` Andreas Schwab
  2023-10-18 22:19     ` Charlie Jenkins
  2 siblings, 1 reply; 16+ messages in thread
From: Andreas Schwab @ 2023-10-18 18:47 UTC (permalink / raw)
  To: Charlie Jenkins
  Cc: linux-riscv, linux-mm, linux-kernel, Eric Biederman, Kees Cook,
	Paul Walmsley, Palmer Dabbelt, Albert Ou

On Okt 17 2023, Charlie Jenkins wrote:

> +static int apply_r_riscv_sub6_rela(struct module *me, u32 *location, Elf_Addr v)
> +{
> +	*(u8 *)location = (*location - ((u8)v & 0x3F)) & 0x3F;

I think that should use *(u8*) on both sides.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."


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

* Re: [PATCH v4 1/2] riscv: Add remaining module relocations
  2023-10-18 18:47   ` Andreas Schwab
@ 2023-10-18 22:19     ` Charlie Jenkins
  0 siblings, 0 replies; 16+ messages in thread
From: Charlie Jenkins @ 2023-10-18 22:19 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: linux-riscv, linux-mm, linux-kernel, Eric Biederman, Kees Cook,
	Paul Walmsley, Palmer Dabbelt, Albert Ou

On Wed, Oct 18, 2023 at 08:47:58PM +0200, Andreas Schwab wrote:
> On Okt 17 2023, Charlie Jenkins wrote:
> 
> > +static int apply_r_riscv_sub6_rela(struct module *me, u32 *location, Elf_Addr v)
> > +{
> > +	*(u8 *)location = (*location - ((u8)v & 0x3F)) & 0x3F;
> 
> I think that should use *(u8*) on both sides.

Yep, thank you.

- Charlie

> 
> -- 
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."


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

* [PATCH v4 2/2] riscv: Add tests for riscv module loading
  2023-10-18  5:34 [PATCH v4 0/2] riscv: Add remaining module relocations and tests Charlie Jenkins
  2023-10-18  5:34 ` [PATCH v4 1/2] riscv: Add remaining module relocations Charlie Jenkins
@ 2023-10-18  5:34 ` Charlie Jenkins
  2023-10-18 11:35 ` [PATCH v4 0/2] riscv: Add remaining module relocations and tests Conor Dooley
  2 siblings, 0 replies; 16+ messages in thread
From: Charlie Jenkins @ 2023-10-18  5:34 UTC (permalink / raw)
  To: linux-riscv, linux-mm, linux-kernel
  Cc: Eric Biederman, Kees Cook, Paul Walmsley, Palmer Dabbelt,
	Albert Ou, Charlie Jenkins

Add test cases for the two main groups of relocations added: SUB and
SET, along with uleb128 which is a bit different because SUB and SET are
required to happen together.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
 arch/riscv/Kconfig.debug                           |  1 +
 arch/riscv/kernel/Makefile                         |  1 +
 arch/riscv/kernel/tests/Kconfig.debug              | 35 +++++++++
 arch/riscv/kernel/tests/Makefile                   |  1 +
 arch/riscv/kernel/tests/module_test/Makefile       | 15 ++++
 .../tests/module_test/test_module_linking_main.c   | 85 ++++++++++++++++++++++
 arch/riscv/kernel/tests/module_test/test_set16.S   | 23 ++++++
 arch/riscv/kernel/tests/module_test/test_set32.S   | 20 +++++
 arch/riscv/kernel/tests/module_test/test_set6.S    | 23 ++++++
 arch/riscv/kernel/tests/module_test/test_set8.S    | 23 ++++++
 arch/riscv/kernel/tests/module_test/test_sub16.S   | 22 ++++++
 arch/riscv/kernel/tests/module_test/test_sub32.S   | 22 ++++++
 arch/riscv/kernel/tests/module_test/test_sub6.S    | 22 ++++++
 arch/riscv/kernel/tests/module_test/test_sub64.S   | 27 +++++++
 arch/riscv/kernel/tests/module_test/test_sub8.S    | 22 ++++++
 arch/riscv/kernel/tests/module_test/test_uleb128.S | 20 +++++
 16 files changed, 362 insertions(+)

diff --git a/arch/riscv/Kconfig.debug b/arch/riscv/Kconfig.debug
index e69de29bb2d1..eafe17ebf710 100644
--- a/arch/riscv/Kconfig.debug
+++ b/arch/riscv/Kconfig.debug
@@ -0,0 +1 @@
+source "arch/riscv/kernel/tests/Kconfig.debug"
diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
index 95cf25d48405..bb99657252f4 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -57,6 +57,7 @@ obj-y	+= stacktrace.o
 obj-y	+= cacheinfo.o
 obj-y	+= patch.o
 obj-y	+= probes/
+obj-y	+= tests/
 obj-$(CONFIG_MMU) += vdso.o vdso/
 
 obj-$(CONFIG_RISCV_M_MODE)	+= traps_misaligned.o
diff --git a/arch/riscv/kernel/tests/Kconfig.debug b/arch/riscv/kernel/tests/Kconfig.debug
new file mode 100644
index 000000000000..5dba64e8e977
--- /dev/null
+++ b/arch/riscv/kernel/tests/Kconfig.debug
@@ -0,0 +1,35 @@
+# SPDX-License-Identifier: GPL-2.0-only
+menu "arch/riscv/kernel Testing and Coverage"
+
+config AS_HAS_ULEB128
+	def_bool $(as-instr,.reloc label$(comma) R_RISCV_SET_ULEB128$(comma) 127\n.reloc label$(comma) R_RISCV_SUB_ULEB128$(comma) 127\nlabel:\n.word 0)
+
+menuconfig RUNTIME_KERNEL_TESTING_MENU
+       bool "arch/riscv/kernel runtime Testing"
+       def_bool y
+       help
+         Enable riscv kernel runtime testing.
+
+if RUNTIME_KERNEL_TESTING_MENU
+
+config RISCV_MODULE_LINKING_KUNIT
+       bool "KUnit test riscv module linking at runtime" if !KUNIT_ALL_TESTS
+       depends on KUNIT
+       default KUNIT_ALL_TESTS
+       help
+         Enable this option to test riscv module linking at boot. This will
+	 enable a module called "test_module_linking".
+
+         KUnit tests run during boot and output the results to the debug log
+         in TAP format (http://testanything.org/). Only useful for kernel devs
+         running the KUnit test harness, and not intended for inclusion into a
+         production build.
+
+         For more information on KUnit and unit tests in general please refer
+         to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+         If unsure, say N.
+
+endif # RUNTIME_TESTING_MENU
+
+endmenu # "arch/riscv/kernel runtime Testing"
diff --git a/arch/riscv/kernel/tests/Makefile b/arch/riscv/kernel/tests/Makefile
new file mode 100644
index 000000000000..7d6c76cffe20
--- /dev/null
+++ b/arch/riscv/kernel/tests/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_RISCV_MODULE_LINKING_KUNIT)	+= module_test/
diff --git a/arch/riscv/kernel/tests/module_test/Makefile b/arch/riscv/kernel/tests/module_test/Makefile
new file mode 100644
index 000000000000..d7a6fd8943de
--- /dev/null
+++ b/arch/riscv/kernel/tests/module_test/Makefile
@@ -0,0 +1,15 @@
+obj-m += test_module_linking.o
+
+test_sub := test_sub6.o test_sub8.o test_sub16.o test_sub32.o test_sub64.o
+
+test_set := test_set6.o test_set8.o test_set16.o test_set32.o
+
+test_module_linking-objs += $(test_sub)
+
+test_module_linking-objs += $(test_set)
+
+ifeq ($(CONFIG_AS_HAS_ULEB128),y)
+test_module_linking-objs += test_uleb128.o
+endif
+
+test_module_linking-objs += test_module_linking_main.o
diff --git a/arch/riscv/kernel/tests/module_test/test_module_linking_main.c b/arch/riscv/kernel/tests/module_test/test_module_linking_main.c
new file mode 100644
index 000000000000..49820352f1df
--- /dev/null
+++ b/arch/riscv/kernel/tests/module_test/test_module_linking_main.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023 Rivos Inc.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <kunit/test.h>
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Test module linking");
+
+extern int test_set32(void);
+extern int test_set16(void);
+extern int test_set8(void);
+extern int test_set6(void);
+extern long test_sub64(void);
+extern int test_sub32(void);
+extern int test_sub16(void);
+extern int test_sub8(void);
+extern int test_sub6(void);
+
+#ifdef CONFIG_AS_HAS_ULEB128
+extern int test_uleb(void);
+#endif
+
+#define CHECK_EQ(lhs, rhs) KUNIT_ASSERT_EQ(test, lhs, rhs)
+
+void run_test_set(struct kunit *test);
+void run_test_sub(struct kunit *test);
+void run_test_uleb(struct kunit *test);
+
+void run_test_set(struct kunit *test)
+{
+	int val32 = test_set32();
+	int val16 = test_set16();
+	int val8 = test_set8();
+	int val6 = test_set6();
+
+	CHECK_EQ(val32, 0);
+	CHECK_EQ(val16, 0);
+	CHECK_EQ(val8, 0);
+	CHECK_EQ(val6, 0);
+}
+
+void run_test_sub(struct kunit *test)
+{
+	int val64 = test_sub64();
+	int val32 = test_sub32();
+	int val16 = test_sub16();
+	int val8 = test_sub8();
+	int val6 = test_sub6();
+
+	CHECK_EQ(val64, 0);
+	CHECK_EQ(val32, 0);
+	CHECK_EQ(val16, 0);
+	CHECK_EQ(val8, 0);
+	CHECK_EQ(val6, 0);
+}
+
+#ifdef CONFIG_AS_HAS_ULEB128
+void run_test_uleb(struct kunit *test)
+{
+	int valuleb = test_uleb();
+
+	CHECK_EQ(valuleb, 0);
+}
+#endif
+
+static struct kunit_case __refdata riscv_module_linking_test_cases[] = {
+	KUNIT_CASE(run_test_set),
+	KUNIT_CASE(run_test_sub),
+#ifdef CONFIG_AS_HAS_ULEB128
+	KUNIT_CASE(run_test_uleb),
+#endif
+	{}
+};
+
+static struct kunit_suite riscv_module_linking_test_suite = {
+	.name = "riscv_checksum",
+	.test_cases = riscv_module_linking_test_cases,
+};
+
+kunit_test_suites(&riscv_module_linking_test_suite);
diff --git a/arch/riscv/kernel/tests/module_test/test_set16.S b/arch/riscv/kernel/tests/module_test/test_set16.S
new file mode 100644
index 000000000000..2be0e441a12e
--- /dev/null
+++ b/arch/riscv/kernel/tests/module_test/test_set16.S
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2023 Rivos Inc.
+ */
+
+.text
+.global test_set16
+test_set16:
+	lw	a0, set16
+	la	t0, set16
+#ifdef CONFIG_32BIT
+	slli	t0, t0, 16
+	srli	t0, t0, 16
+#else
+	slli	t0, t0, 48
+	srli	t0, t0, 48
+#endif
+	sub	a0, a0, t0
+	ret
+.data
+set16:
+	.reloc set16, R_RISCV_SET16, set16
+	.word 0
diff --git a/arch/riscv/kernel/tests/module_test/test_set32.S b/arch/riscv/kernel/tests/module_test/test_set32.S
new file mode 100644
index 000000000000..de0444537e67
--- /dev/null
+++ b/arch/riscv/kernel/tests/module_test/test_set32.S
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2023 Rivos Inc.
+ */
+
+.text
+.global test_set32
+test_set32:
+	lw	a0, set32
+	la	t0, set32
+#ifndef CONFIG_32BIT
+	slli	t0, t0, 32
+	srli	t0, t0, 32
+#endif
+	sub	a0, a0, t0
+	ret
+.data
+set32:
+	.reloc set32, R_RISCV_SET32, set32
+	.word 0
diff --git a/arch/riscv/kernel/tests/module_test/test_set6.S b/arch/riscv/kernel/tests/module_test/test_set6.S
new file mode 100644
index 000000000000..c39ce4c219eb
--- /dev/null
+++ b/arch/riscv/kernel/tests/module_test/test_set6.S
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2023 Rivos Inc.
+ */
+
+.text
+.global test_set6
+test_set6:
+	lw	a0, set6
+	la	t0, set6
+#ifdef CONFIG_32BIT
+	slli	t0, t0, 26
+	srli	t0, t0, 26
+#else
+	slli	t0, t0, 58
+	srli	t0, t0, 58
+#endif
+	sub	a0, a0, t0
+	ret
+.data
+set6:
+	.reloc set6, R_RISCV_SET6, set6
+	.word 0
diff --git a/arch/riscv/kernel/tests/module_test/test_set8.S b/arch/riscv/kernel/tests/module_test/test_set8.S
new file mode 100644
index 000000000000..a656173f6f99
--- /dev/null
+++ b/arch/riscv/kernel/tests/module_test/test_set8.S
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2023 Rivos Inc.
+ */
+
+.text
+.global test_set8
+test_set8:
+	lw	a0, set8
+	la	t0, set8
+#ifdef CONFIG_32BIT
+	slli	t0, t0, 24
+	srli	t0, t0, 24
+#else
+	slli	t0, t0, 56
+	srli	t0, t0, 56
+#endif
+	sub	a0, a0, t0
+	ret
+.data
+set8:
+	.reloc set8, R_RISCV_SET8, set8
+	.word 0
diff --git a/arch/riscv/kernel/tests/module_test/test_sub16.S b/arch/riscv/kernel/tests/module_test/test_sub16.S
new file mode 100644
index 000000000000..c561e155d1db
--- /dev/null
+++ b/arch/riscv/kernel/tests/module_test/test_sub16.S
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2023 Rivos Inc.
+ */
+
+.text
+.global test_sub16
+test_sub16:
+	lh	a0, sub16
+	addi	a0, a0, -32
+	ret
+first:
+	.rept 8
+	.word 0
+	.endr
+second:
+
+.data
+sub16:
+	.reloc		sub16, R_RISCV_ADD16, second
+	.reloc		sub16, R_RISCV_SUB16, first
+	.half		0
diff --git a/arch/riscv/kernel/tests/module_test/test_sub32.S b/arch/riscv/kernel/tests/module_test/test_sub32.S
new file mode 100644
index 000000000000..93232c70cae6
--- /dev/null
+++ b/arch/riscv/kernel/tests/module_test/test_sub32.S
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2023 Rivos Inc.
+ */
+
+.text
+.global test_sub32
+test_sub32:
+	lw	a0, sub32
+	addi	a0, a0, -32
+	ret
+first:
+	.rept 8
+	.word 0
+	.endr
+second:
+
+.data
+sub32:
+	.reloc		sub32, R_RISCV_ADD32, second
+	.reloc		sub32, R_RISCV_SUB32, first
+	.word		0
diff --git a/arch/riscv/kernel/tests/module_test/test_sub6.S b/arch/riscv/kernel/tests/module_test/test_sub6.S
new file mode 100644
index 000000000000..d9c9526ceb62
--- /dev/null
+++ b/arch/riscv/kernel/tests/module_test/test_sub6.S
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2023 Rivos Inc.
+ */
+
+.text
+.global test_sub6
+test_sub6:
+	lb	a0, sub6
+	addi	a0, a0, -32
+	ret
+first:
+	.rept 8
+	.word 0
+	.endr
+second:
+
+.data
+sub6:
+	.reloc		sub6, R_RISCV_SET6, second
+	.reloc		sub6, R_RISCV_SUB6, first
+	.byte		0
diff --git a/arch/riscv/kernel/tests/module_test/test_sub64.S b/arch/riscv/kernel/tests/module_test/test_sub64.S
new file mode 100644
index 000000000000..6d260e2a5d98
--- /dev/null
+++ b/arch/riscv/kernel/tests/module_test/test_sub64.S
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2023 Rivos Inc.
+ */
+
+.text
+.global test_sub64
+test_sub64:
+#ifdef CONFIG_32BIT
+	lw	a0, sub64
+#else
+	ld	a0, sub64
+#endif
+	addi	a0, a0, -32
+	ret
+first:
+	.rept 8
+	.word 0
+	.endr
+second:
+
+.data
+sub64:
+	.reloc		sub64, R_RISCV_ADD64, second
+	.reloc		sub64, R_RISCV_SUB64, first
+	.word		0
+	.word		0
diff --git a/arch/riscv/kernel/tests/module_test/test_sub8.S b/arch/riscv/kernel/tests/module_test/test_sub8.S
new file mode 100644
index 000000000000..af7849115d4d
--- /dev/null
+++ b/arch/riscv/kernel/tests/module_test/test_sub8.S
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2023 Rivos Inc.
+ */
+
+.text
+.global test_sub8
+test_sub8:
+	lb	a0, sub8
+	addi	a0, a0, -32
+	ret
+first:
+	.rept 8
+	.word 0
+	.endr
+second:
+
+.data
+sub8:
+	.reloc		sub8, R_RISCV_ADD8, second
+	.reloc		sub8, R_RISCV_SUB8, first
+	.byte		0
diff --git a/arch/riscv/kernel/tests/module_test/test_uleb128.S b/arch/riscv/kernel/tests/module_test/test_uleb128.S
new file mode 100644
index 000000000000..db9f301092d0
--- /dev/null
+++ b/arch/riscv/kernel/tests/module_test/test_uleb128.S
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2023 Rivos Inc.
+ */
+
+.text
+.global test_uleb
+test_uleb:
+	ld	a0, second
+	addi	a0, a0, -127
+	ret
+.data
+first:
+	.rept 127
+	.byte 0
+	.endr
+second:
+	.reloc second, R_RISCV_SET_ULEB128, second
+	.reloc second, R_RISCV_SUB_ULEB128, first
+	.dword 0

-- 
2.34.1



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

* Re: [PATCH v4 0/2] riscv: Add remaining module relocations and tests
  2023-10-18  5:34 [PATCH v4 0/2] riscv: Add remaining module relocations and tests Charlie Jenkins
  2023-10-18  5:34 ` [PATCH v4 1/2] riscv: Add remaining module relocations Charlie Jenkins
  2023-10-18  5:34 ` [PATCH v4 2/2] riscv: Add tests for riscv module loading Charlie Jenkins
@ 2023-10-18 11:35 ` Conor Dooley
  2023-10-18 17:31   ` Charlie Jenkins
  2 siblings, 1 reply; 16+ messages in thread
From: Conor Dooley @ 2023-10-18 11:35 UTC (permalink / raw)
  To: Charlie Jenkins
  Cc: linux-riscv, linux-mm, linux-kernel, Eric Biederman, Kees Cook,
	Paul Walmsley, Palmer Dabbelt, Albert Ou

[-- Attachment #1: Type: text/plain, Size: 3389 bytes --]

Hey Charlie,

On Tue, Oct 17, 2023 at 10:34:15PM -0700, Charlie Jenkins wrote:
> A handful of module relocations were missing, this patch includes the
> remaining ones. I also wrote some test cases to ensure that module
> loading works properly. Some relocations cannot be supported in the
> kernel, these include the ones that rely on thread local storage and
> dynamic linking.
> 
> ULEB128 handling is a bit special because SET and SUB relocations must
> happen together, and SET must happen before SUB. A psABI proposal [1]
> mandates that the first SET_ULEB128 that appears before a SUB_ULEB128
> is the associated SET_ULEB128.
> 
> This can be tested by enabling KUNIT, RUNTIME_KERNEL_TESTING_MENU, and
> RISCV_MODULE_LINKING_KUNIT.
> 
> [1] https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/403
> 
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
> Changes in v4:
> - Complete removal of R_RISCV_RVC_LUI
> - Fix bug in R_RISCV_SUB6 linking
> - Only build ULEB128 tests if supported by toolchain
> - Link to v3: https://lore.kernel.org/r/20231016-module_relocations-v3-0-a667fd6071e9@rivosinc.com

On patch 2/2:

../arch/riscv/kernel/tests/module_test/test_uleb128.S:18:17: error: unknown relocation name
../arch/riscv/kernel/tests/module_test/test_uleb128.S:19:17: error: unknown relocation name

Same toolchain configuration in the patchwork automation as before.

Cheers,
Conor.

> 
> Changes in v3:
> - Add prototypes to test_module_linking_main as recommended by intel
>   zero day bot
> - Improve efficiency of ULEB128 pair matching
> - Link to v2: https://lore.kernel.org/r/20231006-module_relocations-v2-0-47566453fedc@rivosinc.com
> 
> Changes in v2:
> - Added ULEB128 relocations
> - Link to v1: https://lore.kernel.org/r/20230913-module_relocations-v1-0-bb3d8467e793@rivosinc.com
> 
> ---
> Charlie Jenkins (2):
>       riscv: Add remaining module relocations
>       riscv: Add tests for riscv module loading
> 
>  arch/riscv/Kconfig.debug                           |   1 +
>  arch/riscv/include/uapi/asm/elf.h                  |   5 +-
>  arch/riscv/kernel/Makefile                         |   1 +
>  arch/riscv/kernel/module.c                         | 207 ++++++++++++++++++---
>  arch/riscv/kernel/tests/Kconfig.debug              |  35 ++++
>  arch/riscv/kernel/tests/Makefile                   |   1 +
>  arch/riscv/kernel/tests/module_test/Makefile       |  15 ++
>  .../tests/module_test/test_module_linking_main.c   |  85 +++++++++
>  arch/riscv/kernel/tests/module_test/test_set16.S   |  23 +++
>  arch/riscv/kernel/tests/module_test/test_set32.S   |  20 ++
>  arch/riscv/kernel/tests/module_test/test_set6.S    |  23 +++
>  arch/riscv/kernel/tests/module_test/test_set8.S    |  23 +++
>  arch/riscv/kernel/tests/module_test/test_sub16.S   |  22 +++
>  arch/riscv/kernel/tests/module_test/test_sub32.S   |  22 +++
>  arch/riscv/kernel/tests/module_test/test_sub6.S    |  22 +++
>  arch/riscv/kernel/tests/module_test/test_sub64.S   |  27 +++
>  arch/riscv/kernel/tests/module_test/test_sub8.S    |  22 +++
>  arch/riscv/kernel/tests/module_test/test_uleb128.S |  20 ++
>  18 files changed, 548 insertions(+), 26 deletions(-)
> ---
> base-commit: 4d320c2d9a2b22f53523a1b012cda17a50220965
> change-id: 20230908-module_relocations-f63ced651bd7
> -- 
> - Charlie
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

* Re: [PATCH v4 0/2] riscv: Add remaining module relocations and tests
  2023-10-18 11:35 ` [PATCH v4 0/2] riscv: Add remaining module relocations and tests Conor Dooley
@ 2023-10-18 17:31   ` Charlie Jenkins
  2023-10-18 17:41     ` Conor Dooley
  0 siblings, 1 reply; 16+ messages in thread
From: Charlie Jenkins @ 2023-10-18 17:31 UTC (permalink / raw)
  To: Conor Dooley
  Cc: linux-riscv, linux-mm, linux-kernel, Eric Biederman, Kees Cook,
	Paul Walmsley, Palmer Dabbelt, Albert Ou

On Wed, Oct 18, 2023 at 12:35:55PM +0100, Conor Dooley wrote:
> Hey Charlie,
> 
> On Tue, Oct 17, 2023 at 10:34:15PM -0700, Charlie Jenkins wrote:
> > A handful of module relocations were missing, this patch includes the
> > remaining ones. I also wrote some test cases to ensure that module
> > loading works properly. Some relocations cannot be supported in the
> > kernel, these include the ones that rely on thread local storage and
> > dynamic linking.
> > 
> > ULEB128 handling is a bit special because SET and SUB relocations must
> > happen together, and SET must happen before SUB. A psABI proposal [1]
> > mandates that the first SET_ULEB128 that appears before a SUB_ULEB128
> > is the associated SET_ULEB128.
> > 
> > This can be tested by enabling KUNIT, RUNTIME_KERNEL_TESTING_MENU, and
> > RISCV_MODULE_LINKING_KUNIT.
> > 
> > [1] https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/403
> > 
> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > ---
> > Changes in v4:
> > - Complete removal of R_RISCV_RVC_LUI
> > - Fix bug in R_RISCV_SUB6 linking
> > - Only build ULEB128 tests if supported by toolchain
> > - Link to v3: https://lore.kernel.org/r/20231016-module_relocations-v3-0-a667fd6071e9@rivosinc.com
> 
> On patch 2/2:
> 
> ../arch/riscv/kernel/tests/module_test/test_uleb128.S:18:17: error: unknown relocation name
> ../arch/riscv/kernel/tests/module_test/test_uleb128.S:19:17: error: unknown relocation name
> 
> Same toolchain configuration in the patchwork automation as before.
> 
> Cheers,
> Conor.

Where do you see this error? On Patchwork I see a success [1].

[1] https://patchwork.kernel.org/project/linux-riscv/patch/20231017-module_relocations-v4-2-937f5ef316f0@rivosinc.com/

> 
> > 
> > Changes in v3:
> > - Add prototypes to test_module_linking_main as recommended by intel
> >   zero day bot
> > - Improve efficiency of ULEB128 pair matching
> > - Link to v2: https://lore.kernel.org/r/20231006-module_relocations-v2-0-47566453fedc@rivosinc.com
> > 
> > Changes in v2:
> > - Added ULEB128 relocations
> > - Link to v1: https://lore.kernel.org/r/20230913-module_relocations-v1-0-bb3d8467e793@rivosinc.com
> > 
> > ---
> > Charlie Jenkins (2):
> >       riscv: Add remaining module relocations
> >       riscv: Add tests for riscv module loading
> > 
> >  arch/riscv/Kconfig.debug                           |   1 +
> >  arch/riscv/include/uapi/asm/elf.h                  |   5 +-
> >  arch/riscv/kernel/Makefile                         |   1 +
> >  arch/riscv/kernel/module.c                         | 207 ++++++++++++++++++---
> >  arch/riscv/kernel/tests/Kconfig.debug              |  35 ++++
> >  arch/riscv/kernel/tests/Makefile                   |   1 +
> >  arch/riscv/kernel/tests/module_test/Makefile       |  15 ++
> >  .../tests/module_test/test_module_linking_main.c   |  85 +++++++++
> >  arch/riscv/kernel/tests/module_test/test_set16.S   |  23 +++
> >  arch/riscv/kernel/tests/module_test/test_set32.S   |  20 ++
> >  arch/riscv/kernel/tests/module_test/test_set6.S    |  23 +++
> >  arch/riscv/kernel/tests/module_test/test_set8.S    |  23 +++
> >  arch/riscv/kernel/tests/module_test/test_sub16.S   |  22 +++
> >  arch/riscv/kernel/tests/module_test/test_sub32.S   |  22 +++
> >  arch/riscv/kernel/tests/module_test/test_sub6.S    |  22 +++
> >  arch/riscv/kernel/tests/module_test/test_sub64.S   |  27 +++
> >  arch/riscv/kernel/tests/module_test/test_sub8.S    |  22 +++
> >  arch/riscv/kernel/tests/module_test/test_uleb128.S |  20 ++
> >  18 files changed, 548 insertions(+), 26 deletions(-)
> > ---
> > base-commit: 4d320c2d9a2b22f53523a1b012cda17a50220965
> > change-id: 20230908-module_relocations-f63ced651bd7
> > -- 
> > - Charlie
> > 




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

* Re: [PATCH v4 0/2] riscv: Add remaining module relocations and tests
  2023-10-18 17:31   ` Charlie Jenkins
@ 2023-10-18 17:41     ` Conor Dooley
  2023-10-19  5:56       ` Björn Töpel
  0 siblings, 1 reply; 16+ messages in thread
From: Conor Dooley @ 2023-10-18 17:41 UTC (permalink / raw)
  To: Charlie Jenkins
  Cc: linux-riscv, linux-mm, linux-kernel, Eric Biederman, Kees Cook,
	Paul Walmsley, Palmer Dabbelt, Albert Ou, bjorn

[-- Attachment #1: Type: text/plain, Size: 2262 bytes --]

On Wed, Oct 18, 2023 at 10:31:29AM -0700, Charlie Jenkins wrote:
> On Wed, Oct 18, 2023 at 12:35:55PM +0100, Conor Dooley wrote:
> > Hey Charlie,
> > 
> > On Tue, Oct 17, 2023 at 10:34:15PM -0700, Charlie Jenkins wrote:
> > > A handful of module relocations were missing, this patch includes the
> > > remaining ones. I also wrote some test cases to ensure that module
> > > loading works properly. Some relocations cannot be supported in the
> > > kernel, these include the ones that rely on thread local storage and
> > > dynamic linking.
> > > 
> > > ULEB128 handling is a bit special because SET and SUB relocations must
> > > happen together, and SET must happen before SUB. A psABI proposal [1]
> > > mandates that the first SET_ULEB128 that appears before a SUB_ULEB128
> > > is the associated SET_ULEB128.
> > > 
> > > This can be tested by enabling KUNIT, RUNTIME_KERNEL_TESTING_MENU, and
> > > RISCV_MODULE_LINKING_KUNIT.
> > > 
> > > [1] https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/403
> > > 
> > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > > ---
> > > Changes in v4:
> > > - Complete removal of R_RISCV_RVC_LUI
> > > - Fix bug in R_RISCV_SUB6 linking
> > > - Only build ULEB128 tests if supported by toolchain
> > > - Link to v3: https://lore.kernel.org/r/20231016-module_relocations-v3-0-a667fd6071e9@rivosinc.com
> > 
> > On patch 2/2:
> > 
> > ../arch/riscv/kernel/tests/module_test/test_uleb128.S:18:17: error: unknown relocation name
> > ../arch/riscv/kernel/tests/module_test/test_uleb128.S:19:17: error: unknown relocation name
> > 
> > Same toolchain configuration in the patchwork automation as before.
> > 
> > Cheers,
> > Conor.
> 
> Where do you see this error? On Patchwork I see a success [1].
> 
> [1] https://patchwork.kernel.org/project/linux-riscv/patch/20231017-module_relocations-v4-2-937f5ef316f0@rivosinc.com/

It was a failure this morning!
See
<https://github.com/linux-riscv/linux-riscv/actions/runs/6549280771/job/17785844013>

I wonder if there is something wrong with the CI code, where it
erroneously reports the state from previous patches and then runs the
tests again with the new patches and re-pushes the results.

Bjorn, any idea?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

* Re: [PATCH v4 0/2] riscv: Add remaining module relocations and tests
  2023-10-18 17:41     ` Conor Dooley
@ 2023-10-19  5:56       ` Björn Töpel
  0 siblings, 0 replies; 16+ messages in thread
From: Björn Töpel @ 2023-10-19  5:56 UTC (permalink / raw)
  To: Conor Dooley, Charlie Jenkins
  Cc: linux-riscv, linux-mm, linux-kernel, Eric Biederman, Kees Cook,
	Paul Walmsley, Palmer Dabbelt, Albert Ou

Conor Dooley <conor@kernel.org> writes:

> On Wed, Oct 18, 2023 at 10:31:29AM -0700, Charlie Jenkins wrote:
>> On Wed, Oct 18, 2023 at 12:35:55PM +0100, Conor Dooley wrote:
>> > Hey Charlie,
>> > 
>> > On Tue, Oct 17, 2023 at 10:34:15PM -0700, Charlie Jenkins wrote:
>> > > A handful of module relocations were missing, this patch includes the
>> > > remaining ones. I also wrote some test cases to ensure that module
>> > > loading works properly. Some relocations cannot be supported in the
>> > > kernel, these include the ones that rely on thread local storage and
>> > > dynamic linking.
>> > > 
>> > > ULEB128 handling is a bit special because SET and SUB relocations must
>> > > happen together, and SET must happen before SUB. A psABI proposal [1]
>> > > mandates that the first SET_ULEB128 that appears before a SUB_ULEB128
>> > > is the associated SET_ULEB128.
>> > > 
>> > > This can be tested by enabling KUNIT, RUNTIME_KERNEL_TESTING_MENU, and
>> > > RISCV_MODULE_LINKING_KUNIT.
>> > > 
>> > > [1] https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/403
>> > > 
>> > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
>> > > ---
>> > > Changes in v4:
>> > > - Complete removal of R_RISCV_RVC_LUI
>> > > - Fix bug in R_RISCV_SUB6 linking
>> > > - Only build ULEB128 tests if supported by toolchain
>> > > - Link to v3: https://lore.kernel.org/r/20231016-module_relocations-v3-0-a667fd6071e9@rivosinc.com
>> > 
>> > On patch 2/2:
>> > 
>> > ../arch/riscv/kernel/tests/module_test/test_uleb128.S:18:17: error: unknown relocation name
>> > ../arch/riscv/kernel/tests/module_test/test_uleb128.S:19:17: error: unknown relocation name
>> > 
>> > Same toolchain configuration in the patchwork automation as before.
>> > 
>> > Cheers,
>> > Conor.
>> 
>> Where do you see this error? On Patchwork I see a success [1].
>> 
>> [1] https://patchwork.kernel.org/project/linux-riscv/patch/20231017-module_relocations-v4-2-937f5ef316f0@rivosinc.com/
>
> It was a failure this morning!
> See
> <https://github.com/linux-riscv/linux-riscv/actions/runs/6549280771/job/17785844013>
>
> I wonder if there is something wrong with the CI code, where it
> erroneously reports the state from previous patches and then runs the
> tests again with the new patches and re-pushes the results.
>
> Bjorn, any idea?

The PW syncher tries to reuse the Github PR branch for newer versions.
Say that v4 has some set of results, and v5 some set of results. Then,
it'll be a bit of flux until v5 is fully built.

Hmm, I'll try to improve that. The PW v4 should never get results from
PW v5...

FWIW, the v5 of the series
https://patchwork.kernel.org/project/linux-riscv/list/?series=794521 has
a bunch of errors.


Björn


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

end of thread, other threads:[~2023-10-19  5:57 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-18  5:34 [PATCH v4 0/2] riscv: Add remaining module relocations and tests Charlie Jenkins
2023-10-18  5:34 ` [PATCH v4 1/2] riscv: Add remaining module relocations Charlie Jenkins
2023-10-18 12:17   ` Emil Renner Berthing
2023-10-18 18:31     ` Charlie Jenkins
2023-10-18 18:38       ` Emil Renner Berthing
2023-10-18 20:19         ` Charlie Jenkins
2023-10-18 20:28           ` Emil Renner Berthing
2023-10-18 18:28   ` Samuel Holland
2023-10-18 22:19     ` Charlie Jenkins
2023-10-18 18:47   ` Andreas Schwab
2023-10-18 22:19     ` Charlie Jenkins
2023-10-18  5:34 ` [PATCH v4 2/2] riscv: Add tests for riscv module loading Charlie Jenkins
2023-10-18 11:35 ` [PATCH v4 0/2] riscv: Add remaining module relocations and tests Conor Dooley
2023-10-18 17:31   ` Charlie Jenkins
2023-10-18 17:41     ` Conor Dooley
2023-10-19  5:56       ` Björn Töpel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox