All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/2] powerpc/64: Option to use ELF V2 ABI for big-endian
@ 2021-06-11  9:39 ` Nicholas Piggin
  0 siblings, 0 replies; 33+ messages in thread
From: Nicholas Piggin @ 2021-06-11  9:39 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: Nicholas Piggin, linux-kernel, Jessica Yu, Michal Suchánek

Since v3 I added Michael's module check for ELF ABI level. This requires
a change to core module code. If Jessica is happy with it it could go
via the powerpc tree.

Thanks,
Nick

Nicholas Piggin (2):
  module: add elf_check_module_arch for module specific elf arch checks
  powerpc/64: Option to use ELF V2 ABI for big-endian kernels

 arch/powerpc/Kconfig                | 22 ++++++++++++++++++++++
 arch/powerpc/Makefile               | 18 ++++++++++++------
 arch/powerpc/boot/Makefile          |  4 +++-
 arch/powerpc/include/asm/module.h   | 24 ++++++++++++++++++++++++
 arch/powerpc/kernel/vdso64/Makefile | 13 +++++++++++++
 drivers/crypto/vmx/Makefile         |  8 ++++++--
 drivers/crypto/vmx/ppc-xlate.pl     | 10 ++++++----
 include/linux/moduleloader.h        |  5 +++++
 kernel/module.c                     |  2 +-
 9 files changed, 92 insertions(+), 14 deletions(-)

-- 
2.23.0


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

* [PATCH v4 0/2] powerpc/64: Option to use ELF V2 ABI for big-endian
@ 2021-06-11  9:39 ` Nicholas Piggin
  0 siblings, 0 replies; 33+ messages in thread
From: Nicholas Piggin @ 2021-06-11  9:39 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: Michal Suchánek, linux-kernel, Nicholas Piggin, Jessica Yu

Since v3 I added Michael's module check for ELF ABI level. This requires
a change to core module code. If Jessica is happy with it it could go
via the powerpc tree.

Thanks,
Nick

Nicholas Piggin (2):
  module: add elf_check_module_arch for module specific elf arch checks
  powerpc/64: Option to use ELF V2 ABI for big-endian kernels

 arch/powerpc/Kconfig                | 22 ++++++++++++++++++++++
 arch/powerpc/Makefile               | 18 ++++++++++++------
 arch/powerpc/boot/Makefile          |  4 +++-
 arch/powerpc/include/asm/module.h   | 24 ++++++++++++++++++++++++
 arch/powerpc/kernel/vdso64/Makefile | 13 +++++++++++++
 drivers/crypto/vmx/Makefile         |  8 ++++++--
 drivers/crypto/vmx/ppc-xlate.pl     | 10 ++++++----
 include/linux/moduleloader.h        |  5 +++++
 kernel/module.c                     |  2 +-
 9 files changed, 92 insertions(+), 14 deletions(-)

-- 
2.23.0


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

* [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
  2021-06-11  9:39 ` Nicholas Piggin
@ 2021-06-11  9:39   ` Nicholas Piggin
  -1 siblings, 0 replies; 33+ messages in thread
From: Nicholas Piggin @ 2021-06-11  9:39 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: Nicholas Piggin, linux-kernel, Jessica Yu, Michal Suchánek,
	Michael Ellerman

The elf_check_arch() function is used to test usermode binaries, but
kernel modules may have more specific requirements. powerpc would like
to test for ABI version compatibility.

Add an arch-overridable function elf_check_module_arch() that defaults
to elf_check_arch() and use it in elf_validity_check().

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
[np: split patch, added changelog]
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 include/linux/moduleloader.h | 5 +++++
 kernel/module.c              | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 9e09d11ffe5b..fdc042a84562 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -13,6 +13,11 @@
  * must be implemented by each architecture.
  */
 
+// Allow arch to optionally do additional checking of module ELF header
+#ifndef elf_check_module_arch
+#define elf_check_module_arch elf_check_arch
+#endif
+
 /* Adjust arch-specific sections.  Return 0 on success.  */
 int module_frob_arch_sections(Elf_Ehdr *hdr,
 			      Elf_Shdr *sechdrs,
diff --git a/kernel/module.c b/kernel/module.c
index 7e78dfabca97..7c3f9b7478dc 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2946,7 +2946,7 @@ static int elf_validity_check(struct load_info *info)
 
 	if (memcmp(info->hdr->e_ident, ELFMAG, SELFMAG) != 0
 	    || info->hdr->e_type != ET_REL
-	    || !elf_check_arch(info->hdr)
+	    || !elf_check_module_arch(info->hdr)
 	    || info->hdr->e_shentsize != sizeof(Elf_Shdr))
 		return -ENOEXEC;
 
-- 
2.23.0


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

* [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
@ 2021-06-11  9:39   ` Nicholas Piggin
  0 siblings, 0 replies; 33+ messages in thread
From: Nicholas Piggin @ 2021-06-11  9:39 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: Michal Suchánek, linux-kernel, Nicholas Piggin, Jessica Yu

The elf_check_arch() function is used to test usermode binaries, but
kernel modules may have more specific requirements. powerpc would like
to test for ABI version compatibility.

Add an arch-overridable function elf_check_module_arch() that defaults
to elf_check_arch() and use it in elf_validity_check().

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
[np: split patch, added changelog]
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 include/linux/moduleloader.h | 5 +++++
 kernel/module.c              | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 9e09d11ffe5b..fdc042a84562 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -13,6 +13,11 @@
  * must be implemented by each architecture.
  */
 
+// Allow arch to optionally do additional checking of module ELF header
+#ifndef elf_check_module_arch
+#define elf_check_module_arch elf_check_arch
+#endif
+
 /* Adjust arch-specific sections.  Return 0 on success.  */
 int module_frob_arch_sections(Elf_Ehdr *hdr,
 			      Elf_Shdr *sechdrs,
diff --git a/kernel/module.c b/kernel/module.c
index 7e78dfabca97..7c3f9b7478dc 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2946,7 +2946,7 @@ static int elf_validity_check(struct load_info *info)
 
 	if (memcmp(info->hdr->e_ident, ELFMAG, SELFMAG) != 0
 	    || info->hdr->e_type != ET_REL
-	    || !elf_check_arch(info->hdr)
+	    || !elf_check_module_arch(info->hdr)
 	    || info->hdr->e_shentsize != sizeof(Elf_Shdr))
 		return -ENOEXEC;
 
-- 
2.23.0


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

* [PATCH v4 2/2] powerpc/64: Option to use ELF V2 ABI for big-endian kernels
  2021-06-11  9:39 ` Nicholas Piggin
@ 2021-06-11  9:39   ` Nicholas Piggin
  -1 siblings, 0 replies; 33+ messages in thread
From: Nicholas Piggin @ 2021-06-11  9:39 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: Nicholas Piggin, linux-kernel, Jessica Yu, Michal Suchánek,
	Segher Boessenkool

Provide an option to build big-endian kernels using the ELFv2 ABI. This
works on GCC only so far, although it is rumored to work with clang
that's not been tested yet. A new module version check ensures the
module ELF ABI level matches the kernel build.

This can give big-endian kernels some useful advantages of the ELFv2 ABI
(e.g., less stack usage, -mprofile-kernel, better compatibility with eBPF
tools).

BE+ELFv2 is not officially supported by the GNU toolchain, but it works
fine in testing and has been used by some userspace for some time (e.g.,
Void Linux).

Tested-by: Michal Suchánek <msuchanek@suse.de>
Reviewed-by: Segher Boessenkool <segher@kernel.crashing.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/Kconfig                | 22 ++++++++++++++++++++++
 arch/powerpc/Makefile               | 18 ++++++++++++------
 arch/powerpc/boot/Makefile          |  4 +++-
 arch/powerpc/include/asm/module.h   | 24 ++++++++++++++++++++++++
 arch/powerpc/kernel/vdso64/Makefile | 13 +++++++++++++
 drivers/crypto/vmx/Makefile         |  8 ++++++--
 drivers/crypto/vmx/ppc-xlate.pl     | 10 ++++++----
 7 files changed, 86 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 088dd2afcfe4..093f973a28b9 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -163,6 +163,7 @@ config PPC
 	select ARCH_WEAK_RELEASE_ACQUIRE
 	select BINFMT_ELF
 	select BUILDTIME_TABLE_SORT
+	select PPC64_BUILD_ELF_V2_ABI		if PPC64 && CPU_LITTLE_ENDIAN
 	select CLONE_BACKWARDS
 	select DCACHE_WORD_ACCESS		if PPC64 && CPU_LITTLE_ENDIAN
 	select DMA_OPS_BYPASS			if PPC64
@@ -561,6 +562,27 @@ config KEXEC_FILE
 config ARCH_HAS_KEXEC_PURGATORY
 	def_bool KEXEC_FILE
 
+config PPC64_BUILD_ELF_V2_ABI
+	bool
+
+config PPC64_BUILD_BIG_ENDIAN_ELF_V2_ABI
+	bool "Build big-endian kernel using ELF ABI V2 (EXPERIMENTAL)"
+	depends on PPC64 && CPU_BIG_ENDIAN && EXPERT
+	depends on CC_IS_GCC && LD_VERSION >= 22400
+	default n
+	select PPC64_BUILD_ELF_V2_ABI
+	help
+	  This builds the kernel image using the "Power Architecture 64-Bit ELF
+	  V2 ABI Specification", which has a reduced stack overhead and faster
+	  function calls. This internal kernel ABI option does not affect
+          userspace compatibility.
+
+	  The V2 ABI is standard for 64-bit little-endian, but for big-endian
+	  it is less well tested by kernel and toolchain. However some distros
+	  build userspace this way, and it can produce a functioning kernel.
+
+	  This requires GCC and binutils 2.24 or newer.
+
 config RELOCATABLE
 	bool "Build a relocatable kernel"
 	depends on PPC64 || (FLATMEM && (44x || FSL_BOOKE))
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 3212d076ac6a..b90b5cb799aa 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -91,10 +91,14 @@ endif
 
 ifdef CONFIG_PPC64
 ifndef CONFIG_CC_IS_CLANG
-cflags-$(CONFIG_CPU_BIG_ENDIAN)		+= $(call cc-option,-mabi=elfv1)
-cflags-$(CONFIG_CPU_BIG_ENDIAN)		+= $(call cc-option,-mcall-aixdesc)
-aflags-$(CONFIG_CPU_BIG_ENDIAN)		+= $(call cc-option,-mabi=elfv1)
-aflags-$(CONFIG_CPU_LITTLE_ENDIAN)	+= -mabi=elfv2
+ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
+cflags-y				+= $(call cc-option,-mabi=elfv2)
+aflags-y				+= $(call cc-option,-mabi=elfv2)
+else
+cflags-y				+= $(call cc-option,-mabi=elfv1)
+cflags-y				+= $(call cc-option,-mcall-aixdesc)
+aflags-y				+= $(call cc-option,-mabi=elfv1)
+endif
 endif
 endif
 
@@ -142,15 +146,17 @@ endif
 
 CFLAGS-$(CONFIG_PPC64)	:= $(call cc-option,-mtraceback=no)
 ifndef CONFIG_CC_IS_CLANG
-ifdef CONFIG_CPU_LITTLE_ENDIAN
-CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv2,$(call cc-option,-mcall-aixdesc))
+ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
+CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv2)
 AFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv2)
 else
+# Keep these in synch with arch/powerpc/kernel/vdso64/Makefile
 CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv1)
 CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mcall-aixdesc)
 AFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv1)
 endif
 endif
+
 CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mcmodel=medium,$(call cc-option,-mminimal-toc))
 CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mno-pointers-to-nested-functions)
 
diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile
index 2b8da923ceca..be84a72f8258 100644
--- a/arch/powerpc/boot/Makefile
+++ b/arch/powerpc/boot/Makefile
@@ -40,6 +40,9 @@ BOOTCFLAGS    := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
 
 ifdef CONFIG_PPC64_BOOT_WRAPPER
 BOOTCFLAGS	+= -m64
+ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
+BOOTCFLAGS	+= $(call cc-option,-mabi=elfv2)
+endif
 else
 BOOTCFLAGS	+= -m32
 endif
@@ -50,7 +53,6 @@ ifdef CONFIG_CPU_BIG_ENDIAN
 BOOTCFLAGS	+= -mbig-endian
 else
 BOOTCFLAGS	+= -mlittle-endian
-BOOTCFLAGS	+= $(call cc-option,-mabi=elfv2)
 endif
 
 BOOTAFLAGS	:= -D__ASSEMBLY__ $(BOOTCFLAGS) -nostdinc
diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
index 857d9ff24295..043e11068ff4 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -52,6 +52,30 @@ struct mod_arch_specific {
 	unsigned int num_bugs;
 };
 
+/*
+ * Check kernel module ELF header architecture specific compatibility.
+ */
+static inline bool elf_check_module_arch(Elf_Ehdr *hdr)
+{
+	if (!elf_check_arch(hdr))
+		return false;
+
+	if (IS_ENABLED(CONFIG_PPC64)) {
+		unsigned long abi_level = hdr->e_flags & 0x3;
+
+		if (IS_ENABLED(CONFIG_PPC64_BUILD_ELF_V2_ABI)) {
+			if (abi_level != 2)
+				return false;
+		} else {
+			if (abi_level >= 2)
+				return false;
+		}
+	}
+
+	return true;
+}
+#define elf_check_module_arch elf_check_module_arch
+
 /*
  * Select ELF headers.
  * Make empty section for module_frob_arch_sections to expand.
diff --git a/arch/powerpc/kernel/vdso64/Makefile b/arch/powerpc/kernel/vdso64/Makefile
index 2813e3f98db6..d783c07e558f 100644
--- a/arch/powerpc/kernel/vdso64/Makefile
+++ b/arch/powerpc/kernel/vdso64/Makefile
@@ -25,6 +25,19 @@ KCOV_INSTRUMENT := n
 UBSAN_SANITIZE := n
 KASAN_SANITIZE := n
 
+# Always build vdso64 with ELFv1 ABI for BE kernels
+ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
+ifdef CONFIG_CPU_BIG_ENDIAN
+KBUILD_CFLAGS := $(filter-out -mabi=elfv2,$(KBUILD_CFLAGS))
+KBUILD_AFLAGS := $(filter-out -mabi=elfv2,$(KBUILD_AFLAGS))
+
+# These are derived from arch/powerpc/Makefile
+KBUILD_CFLAGS += $(call cc-option,-mabi=elfv1)
+KBUILD_CFLAGS += $(call cc-option,-mcall-aixdesc)
+KBUILD_AFLAGS += $(call cc-option,-mabi=elfv1)
+endif
+endif
+
 ccflags-y := -shared -fno-common -fno-builtin -nostdlib \
 	-Wl,-soname=linux-vdso64.so.1 -Wl,--hash-style=both
 asflags-y := -D__VDSO64__ -s
diff --git a/drivers/crypto/vmx/Makefile b/drivers/crypto/vmx/Makefile
index 709670d2b553..d9ccf9fc3483 100644
--- a/drivers/crypto/vmx/Makefile
+++ b/drivers/crypto/vmx/Makefile
@@ -5,18 +5,22 @@ vmx-crypto-objs := vmx.o aesp8-ppc.o ghashp8-ppc.o aes.o aes_cbc.o aes_ctr.o aes
 ifeq ($(CONFIG_CPU_LITTLE_ENDIAN),y)
 override flavour := linux-ppc64le
 else
+ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
+override flavour := linux-ppc64-elfv2
+else
 override flavour := linux-ppc64
 endif
+endif
 
 quiet_cmd_perl = PERL $@
       cmd_perl = $(PERL) $(<) $(flavour) > $(@)
 
 targets += aesp8-ppc.S ghashp8-ppc.S
 
-$(obj)/aesp8-ppc.S: $(src)/aesp8-ppc.pl FORCE
+$(obj)/aesp8-ppc.S: $(src)/aesp8-ppc.pl $(src)/ppc-xlate.pl FORCE
 	$(call if_changed,perl)
   
-$(obj)/ghashp8-ppc.S: $(src)/ghashp8-ppc.pl FORCE
+$(obj)/ghashp8-ppc.S: $(src)/ghashp8-ppc.pl $(src)/ppc-xlate.pl FORCE
 	$(call if_changed,perl)
 
 clean-files := aesp8-ppc.S ghashp8-ppc.S
diff --git a/drivers/crypto/vmx/ppc-xlate.pl b/drivers/crypto/vmx/ppc-xlate.pl
index 36db2ef09e5b..b583898c11ae 100644
--- a/drivers/crypto/vmx/ppc-xlate.pl
+++ b/drivers/crypto/vmx/ppc-xlate.pl
@@ -9,6 +9,8 @@ open STDOUT,">$output" || die "can't open $output: $!";
 
 my %GLOBALS;
 my $dotinlocallabels=($flavour=~/linux/)?1:0;
+my $elfv2abi=(($flavour =~ /linux-ppc64le/) or ($flavour =~ /linux-ppc64-elfv2/))?1:0;
+my $dotfunctions=($elfv2abi=~1)?0:1;
 
 ################################################################
 # directives which need special treatment on different platforms
@@ -40,7 +42,7 @@ my $globl = sub {
 };
 my $text = sub {
     my $ret = ($flavour =~ /aix/) ? ".csect\t.text[PR],7" : ".text";
-    $ret = ".abiversion	2\n".$ret	if ($flavour =~ /linux.*64le/);
+    $ret = ".abiversion	2\n".$ret	if ($elfv2abi);
     $ret;
 };
 my $machine = sub {
@@ -56,8 +58,8 @@ my $size = sub {
     if ($flavour =~ /linux/)
     {	shift;
 	my $name = shift; $name =~ s|^[\.\_]||;
-	my $ret  = ".size	$name,.-".($flavour=~/64$/?".":"").$name;
-	$ret .= "\n.size	.$name,.-.$name" if ($flavour=~/64$/);
+	my $ret  = ".size	$name,.-".($dotfunctions?".":"").$name;
+	$ret .= "\n.size	.$name,.-.$name" if ($dotfunctions);
 	$ret;
     }
     else
@@ -142,7 +144,7 @@ my $vmr = sub {
 
 # Some ABIs specify vrsave, special-purpose register #256, as reserved
 # for system use.
-my $no_vrsave = ($flavour =~ /linux-ppc64le/);
+my $no_vrsave = ($elfv2abi);
 my $mtspr = sub {
     my ($f,$idx,$ra) = @_;
     if ($idx == 256 && $no_vrsave) {
-- 
2.23.0


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

* [PATCH v4 2/2] powerpc/64: Option to use ELF V2 ABI for big-endian kernels
@ 2021-06-11  9:39   ` Nicholas Piggin
  0 siblings, 0 replies; 33+ messages in thread
From: Nicholas Piggin @ 2021-06-11  9:39 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: Michal Suchánek, linux-kernel, Nicholas Piggin, Jessica Yu

Provide an option to build big-endian kernels using the ELFv2 ABI. This
works on GCC only so far, although it is rumored to work with clang
that's not been tested yet. A new module version check ensures the
module ELF ABI level matches the kernel build.

This can give big-endian kernels some useful advantages of the ELFv2 ABI
(e.g., less stack usage, -mprofile-kernel, better compatibility with eBPF
tools).

BE+ELFv2 is not officially supported by the GNU toolchain, but it works
fine in testing and has been used by some userspace for some time (e.g.,
Void Linux).

Tested-by: Michal Suchánek <msuchanek@suse.de>
Reviewed-by: Segher Boessenkool <segher@kernel.crashing.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/Kconfig                | 22 ++++++++++++++++++++++
 arch/powerpc/Makefile               | 18 ++++++++++++------
 arch/powerpc/boot/Makefile          |  4 +++-
 arch/powerpc/include/asm/module.h   | 24 ++++++++++++++++++++++++
 arch/powerpc/kernel/vdso64/Makefile | 13 +++++++++++++
 drivers/crypto/vmx/Makefile         |  8 ++++++--
 drivers/crypto/vmx/ppc-xlate.pl     | 10 ++++++----
 7 files changed, 86 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 088dd2afcfe4..093f973a28b9 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -163,6 +163,7 @@ config PPC
 	select ARCH_WEAK_RELEASE_ACQUIRE
 	select BINFMT_ELF
 	select BUILDTIME_TABLE_SORT
+	select PPC64_BUILD_ELF_V2_ABI		if PPC64 && CPU_LITTLE_ENDIAN
 	select CLONE_BACKWARDS
 	select DCACHE_WORD_ACCESS		if PPC64 && CPU_LITTLE_ENDIAN
 	select DMA_OPS_BYPASS			if PPC64
@@ -561,6 +562,27 @@ config KEXEC_FILE
 config ARCH_HAS_KEXEC_PURGATORY
 	def_bool KEXEC_FILE
 
+config PPC64_BUILD_ELF_V2_ABI
+	bool
+
+config PPC64_BUILD_BIG_ENDIAN_ELF_V2_ABI
+	bool "Build big-endian kernel using ELF ABI V2 (EXPERIMENTAL)"
+	depends on PPC64 && CPU_BIG_ENDIAN && EXPERT
+	depends on CC_IS_GCC && LD_VERSION >= 22400
+	default n
+	select PPC64_BUILD_ELF_V2_ABI
+	help
+	  This builds the kernel image using the "Power Architecture 64-Bit ELF
+	  V2 ABI Specification", which has a reduced stack overhead and faster
+	  function calls. This internal kernel ABI option does not affect
+          userspace compatibility.
+
+	  The V2 ABI is standard for 64-bit little-endian, but for big-endian
+	  it is less well tested by kernel and toolchain. However some distros
+	  build userspace this way, and it can produce a functioning kernel.
+
+	  This requires GCC and binutils 2.24 or newer.
+
 config RELOCATABLE
 	bool "Build a relocatable kernel"
 	depends on PPC64 || (FLATMEM && (44x || FSL_BOOKE))
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 3212d076ac6a..b90b5cb799aa 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -91,10 +91,14 @@ endif
 
 ifdef CONFIG_PPC64
 ifndef CONFIG_CC_IS_CLANG
-cflags-$(CONFIG_CPU_BIG_ENDIAN)		+= $(call cc-option,-mabi=elfv1)
-cflags-$(CONFIG_CPU_BIG_ENDIAN)		+= $(call cc-option,-mcall-aixdesc)
-aflags-$(CONFIG_CPU_BIG_ENDIAN)		+= $(call cc-option,-mabi=elfv1)
-aflags-$(CONFIG_CPU_LITTLE_ENDIAN)	+= -mabi=elfv2
+ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
+cflags-y				+= $(call cc-option,-mabi=elfv2)
+aflags-y				+= $(call cc-option,-mabi=elfv2)
+else
+cflags-y				+= $(call cc-option,-mabi=elfv1)
+cflags-y				+= $(call cc-option,-mcall-aixdesc)
+aflags-y				+= $(call cc-option,-mabi=elfv1)
+endif
 endif
 endif
 
@@ -142,15 +146,17 @@ endif
 
 CFLAGS-$(CONFIG_PPC64)	:= $(call cc-option,-mtraceback=no)
 ifndef CONFIG_CC_IS_CLANG
-ifdef CONFIG_CPU_LITTLE_ENDIAN
-CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv2,$(call cc-option,-mcall-aixdesc))
+ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
+CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv2)
 AFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv2)
 else
+# Keep these in synch with arch/powerpc/kernel/vdso64/Makefile
 CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv1)
 CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mcall-aixdesc)
 AFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv1)
 endif
 endif
+
 CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mcmodel=medium,$(call cc-option,-mminimal-toc))
 CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mno-pointers-to-nested-functions)
 
diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile
index 2b8da923ceca..be84a72f8258 100644
--- a/arch/powerpc/boot/Makefile
+++ b/arch/powerpc/boot/Makefile
@@ -40,6 +40,9 @@ BOOTCFLAGS    := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
 
 ifdef CONFIG_PPC64_BOOT_WRAPPER
 BOOTCFLAGS	+= -m64
+ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
+BOOTCFLAGS	+= $(call cc-option,-mabi=elfv2)
+endif
 else
 BOOTCFLAGS	+= -m32
 endif
@@ -50,7 +53,6 @@ ifdef CONFIG_CPU_BIG_ENDIAN
 BOOTCFLAGS	+= -mbig-endian
 else
 BOOTCFLAGS	+= -mlittle-endian
-BOOTCFLAGS	+= $(call cc-option,-mabi=elfv2)
 endif
 
 BOOTAFLAGS	:= -D__ASSEMBLY__ $(BOOTCFLAGS) -nostdinc
diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
index 857d9ff24295..043e11068ff4 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -52,6 +52,30 @@ struct mod_arch_specific {
 	unsigned int num_bugs;
 };
 
+/*
+ * Check kernel module ELF header architecture specific compatibility.
+ */
+static inline bool elf_check_module_arch(Elf_Ehdr *hdr)
+{
+	if (!elf_check_arch(hdr))
+		return false;
+
+	if (IS_ENABLED(CONFIG_PPC64)) {
+		unsigned long abi_level = hdr->e_flags & 0x3;
+
+		if (IS_ENABLED(CONFIG_PPC64_BUILD_ELF_V2_ABI)) {
+			if (abi_level != 2)
+				return false;
+		} else {
+			if (abi_level >= 2)
+				return false;
+		}
+	}
+
+	return true;
+}
+#define elf_check_module_arch elf_check_module_arch
+
 /*
  * Select ELF headers.
  * Make empty section for module_frob_arch_sections to expand.
diff --git a/arch/powerpc/kernel/vdso64/Makefile b/arch/powerpc/kernel/vdso64/Makefile
index 2813e3f98db6..d783c07e558f 100644
--- a/arch/powerpc/kernel/vdso64/Makefile
+++ b/arch/powerpc/kernel/vdso64/Makefile
@@ -25,6 +25,19 @@ KCOV_INSTRUMENT := n
 UBSAN_SANITIZE := n
 KASAN_SANITIZE := n
 
+# Always build vdso64 with ELFv1 ABI for BE kernels
+ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
+ifdef CONFIG_CPU_BIG_ENDIAN
+KBUILD_CFLAGS := $(filter-out -mabi=elfv2,$(KBUILD_CFLAGS))
+KBUILD_AFLAGS := $(filter-out -mabi=elfv2,$(KBUILD_AFLAGS))
+
+# These are derived from arch/powerpc/Makefile
+KBUILD_CFLAGS += $(call cc-option,-mabi=elfv1)
+KBUILD_CFLAGS += $(call cc-option,-mcall-aixdesc)
+KBUILD_AFLAGS += $(call cc-option,-mabi=elfv1)
+endif
+endif
+
 ccflags-y := -shared -fno-common -fno-builtin -nostdlib \
 	-Wl,-soname=linux-vdso64.so.1 -Wl,--hash-style=both
 asflags-y := -D__VDSO64__ -s
diff --git a/drivers/crypto/vmx/Makefile b/drivers/crypto/vmx/Makefile
index 709670d2b553..d9ccf9fc3483 100644
--- a/drivers/crypto/vmx/Makefile
+++ b/drivers/crypto/vmx/Makefile
@@ -5,18 +5,22 @@ vmx-crypto-objs := vmx.o aesp8-ppc.o ghashp8-ppc.o aes.o aes_cbc.o aes_ctr.o aes
 ifeq ($(CONFIG_CPU_LITTLE_ENDIAN),y)
 override flavour := linux-ppc64le
 else
+ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
+override flavour := linux-ppc64-elfv2
+else
 override flavour := linux-ppc64
 endif
+endif
 
 quiet_cmd_perl = PERL $@
       cmd_perl = $(PERL) $(<) $(flavour) > $(@)
 
 targets += aesp8-ppc.S ghashp8-ppc.S
 
-$(obj)/aesp8-ppc.S: $(src)/aesp8-ppc.pl FORCE
+$(obj)/aesp8-ppc.S: $(src)/aesp8-ppc.pl $(src)/ppc-xlate.pl FORCE
 	$(call if_changed,perl)
   
-$(obj)/ghashp8-ppc.S: $(src)/ghashp8-ppc.pl FORCE
+$(obj)/ghashp8-ppc.S: $(src)/ghashp8-ppc.pl $(src)/ppc-xlate.pl FORCE
 	$(call if_changed,perl)
 
 clean-files := aesp8-ppc.S ghashp8-ppc.S
diff --git a/drivers/crypto/vmx/ppc-xlate.pl b/drivers/crypto/vmx/ppc-xlate.pl
index 36db2ef09e5b..b583898c11ae 100644
--- a/drivers/crypto/vmx/ppc-xlate.pl
+++ b/drivers/crypto/vmx/ppc-xlate.pl
@@ -9,6 +9,8 @@ open STDOUT,">$output" || die "can't open $output: $!";
 
 my %GLOBALS;
 my $dotinlocallabels=($flavour=~/linux/)?1:0;
+my $elfv2abi=(($flavour =~ /linux-ppc64le/) or ($flavour =~ /linux-ppc64-elfv2/))?1:0;
+my $dotfunctions=($elfv2abi=~1)?0:1;
 
 ################################################################
 # directives which need special treatment on different platforms
@@ -40,7 +42,7 @@ my $globl = sub {
 };
 my $text = sub {
     my $ret = ($flavour =~ /aix/) ? ".csect\t.text[PR],7" : ".text";
-    $ret = ".abiversion	2\n".$ret	if ($flavour =~ /linux.*64le/);
+    $ret = ".abiversion	2\n".$ret	if ($elfv2abi);
     $ret;
 };
 my $machine = sub {
@@ -56,8 +58,8 @@ my $size = sub {
     if ($flavour =~ /linux/)
     {	shift;
 	my $name = shift; $name =~ s|^[\.\_]||;
-	my $ret  = ".size	$name,.-".($flavour=~/64$/?".":"").$name;
-	$ret .= "\n.size	.$name,.-.$name" if ($flavour=~/64$/);
+	my $ret  = ".size	$name,.-".($dotfunctions?".":"").$name;
+	$ret .= "\n.size	.$name,.-.$name" if ($dotfunctions);
 	$ret;
     }
     else
@@ -142,7 +144,7 @@ my $vmr = sub {
 
 # Some ABIs specify vrsave, special-purpose register #256, as reserved
 # for system use.
-my $no_vrsave = ($flavour =~ /linux-ppc64le/);
+my $no_vrsave = ($elfv2abi);
 my $mtspr = sub {
     my ($f,$idx,$ra) = @_;
     if ($idx == 256 && $no_vrsave) {
-- 
2.23.0


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

* Re: [PATCH v4 2/2] powerpc/64: Option to use ELF V2 ABI for big-endian kernels
  2021-06-11  9:39   ` Nicholas Piggin
@ 2021-06-11  9:58     ` Michal Suchánek
  -1 siblings, 0 replies; 33+ messages in thread
From: Michal Suchánek @ 2021-06-11  9:58 UTC (permalink / raw)
  To: Nicholas Piggin
  Cc: linuxppc-dev, linux-kernel, Jessica Yu, Segher Boessenkool

On Fri, Jun 11, 2021 at 07:39:59PM +1000, Nicholas Piggin wrote:
> Provide an option to build big-endian kernels using the ELFv2 ABI. This
> works on GCC only so far, although it is rumored to work with clang
> that's not been tested yet. A new module version check ensures the
> module ELF ABI level matches the kernel build.
> 
> This can give big-endian kernels some useful advantages of the ELFv2 ABI
> (e.g., less stack usage, -mprofile-kernel, better compatibility with eBPF
> tools).
> 
> BE+ELFv2 is not officially supported by the GNU toolchain, but it works
> fine in testing and has been used by some userspace for some time (e.g.,
> Void Linux).
> 
> Tested-by: Michal Suchánek <msuchanek@suse.de>
> Reviewed-by: Segher Boessenkool <segher@kernel.crashing.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/Kconfig                | 22 ++++++++++++++++++++++
>  arch/powerpc/Makefile               | 18 ++++++++++++------
>  arch/powerpc/boot/Makefile          |  4 +++-
>  arch/powerpc/include/asm/module.h   | 24 ++++++++++++++++++++++++
>  arch/powerpc/kernel/vdso64/Makefile | 13 +++++++++++++
>  drivers/crypto/vmx/Makefile         |  8 ++++++--
>  drivers/crypto/vmx/ppc-xlate.pl     | 10 ++++++----
>  7 files changed, 86 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 088dd2afcfe4..093f973a28b9 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -163,6 +163,7 @@ config PPC
>  	select ARCH_WEAK_RELEASE_ACQUIRE
>  	select BINFMT_ELF
>  	select BUILDTIME_TABLE_SORT
> +	select PPC64_BUILD_ELF_V2_ABI		if PPC64 && CPU_LITTLE_ENDIAN
>  	select CLONE_BACKWARDS
>  	select DCACHE_WORD_ACCESS		if PPC64 && CPU_LITTLE_ENDIAN
>  	select DMA_OPS_BYPASS			if PPC64
> @@ -561,6 +562,27 @@ config KEXEC_FILE
>  config ARCH_HAS_KEXEC_PURGATORY
>  	def_bool KEXEC_FILE
>  
> +config PPC64_BUILD_ELF_V2_ABI
> +	bool
> +
> +config PPC64_BUILD_BIG_ENDIAN_ELF_V2_ABI
> +	bool "Build big-endian kernel using ELF ABI V2 (EXPERIMENTAL)"
> +	depends on PPC64 && CPU_BIG_ENDIAN && EXPERT
> +	depends on CC_IS_GCC && LD_VERSION >= 22400
> +	default n
> +	select PPC64_BUILD_ELF_V2_ABI
> +	help
> +	  This builds the kernel image using the "Power Architecture 64-Bit ELF
> +	  V2 ABI Specification", which has a reduced stack overhead and faster
> +	  function calls. This internal kernel ABI option does not affect
> +          userspace compatibility.
> +
> +	  The V2 ABI is standard for 64-bit little-endian, but for big-endian
> +	  it is less well tested by kernel and toolchain. However some distros
> +	  build userspace this way, and it can produce a functioning kernel.
> +
> +	  This requires GCC and binutils 2.24 or newer.
> +
>  config RELOCATABLE
>  	bool "Build a relocatable kernel"
>  	depends on PPC64 || (FLATMEM && (44x || FSL_BOOKE))
> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> index 3212d076ac6a..b90b5cb799aa 100644
> --- a/arch/powerpc/Makefile
> +++ b/arch/powerpc/Makefile
> @@ -91,10 +91,14 @@ endif
>  
>  ifdef CONFIG_PPC64
>  ifndef CONFIG_CC_IS_CLANG
> -cflags-$(CONFIG_CPU_BIG_ENDIAN)		+= $(call cc-option,-mabi=elfv1)
> -cflags-$(CONFIG_CPU_BIG_ENDIAN)		+= $(call cc-option,-mcall-aixdesc)
> -aflags-$(CONFIG_CPU_BIG_ENDIAN)		+= $(call cc-option,-mabi=elfv1)
> -aflags-$(CONFIG_CPU_LITTLE_ENDIAN)	+= -mabi=elfv2
> +ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
> +cflags-y				+= $(call cc-option,-mabi=elfv2)
> +aflags-y				+= $(call cc-option,-mabi=elfv2)
> +else
> +cflags-y				+= $(call cc-option,-mabi=elfv1)
> +cflags-y				+= $(call cc-option,-mcall-aixdesc)
> +aflags-y				+= $(call cc-option,-mabi=elfv1)
> +endif
>  endif
>  endif
>  
> @@ -142,15 +146,17 @@ endif
>  
>  CFLAGS-$(CONFIG_PPC64)	:= $(call cc-option,-mtraceback=no)
>  ifndef CONFIG_CC_IS_CLANG
> -ifdef CONFIG_CPU_LITTLE_ENDIAN
> -CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv2,$(call cc-option,-mcall-aixdesc))
> +ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
> +CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv2)
>  AFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv2)
>  else
> +# Keep these in synch with arch/powerpc/kernel/vdso64/Makefile
>  CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv1)
>  CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mcall-aixdesc)
>  AFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv1)
>  endif
>  endif
> +
>  CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mcmodel=medium,$(call cc-option,-mminimal-toc))
>  CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mno-pointers-to-nested-functions)
>  
> diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile
> index 2b8da923ceca..be84a72f8258 100644
> --- a/arch/powerpc/boot/Makefile
> +++ b/arch/powerpc/boot/Makefile
> @@ -40,6 +40,9 @@ BOOTCFLAGS    := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
>  
>  ifdef CONFIG_PPC64_BOOT_WRAPPER
>  BOOTCFLAGS	+= -m64
> +ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
> +BOOTCFLAGS	+= $(call cc-option,-mabi=elfv2)
> +endif
>  else
>  BOOTCFLAGS	+= -m32
>  endif
> @@ -50,7 +53,6 @@ ifdef CONFIG_CPU_BIG_ENDIAN
>  BOOTCFLAGS	+= -mbig-endian
>  else
>  BOOTCFLAGS	+= -mlittle-endian
> -BOOTCFLAGS	+= $(call cc-option,-mabi=elfv2)
>  endif
>  
>  BOOTAFLAGS	:= -D__ASSEMBLY__ $(BOOTCFLAGS) -nostdinc
> diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
> index 857d9ff24295..043e11068ff4 100644
> --- a/arch/powerpc/include/asm/module.h
> +++ b/arch/powerpc/include/asm/module.h
> @@ -52,6 +52,30 @@ struct mod_arch_specific {
>  	unsigned int num_bugs;
>  };
>  
> +/*
> + * Check kernel module ELF header architecture specific compatibility.
> + */
> +static inline bool elf_check_module_arch(Elf_Ehdr *hdr)
> +{
> +	if (!elf_check_arch(hdr))
> +		return false;
> +
> +	if (IS_ENABLED(CONFIG_PPC64)) {
> +		unsigned long abi_level = hdr->e_flags & 0x3;
> +
> +		if (IS_ENABLED(CONFIG_PPC64_BUILD_ELF_V2_ABI)) {
> +			if (abi_level != 2)
> +				return false;
> +		} else {
> +			if (abi_level >= 2)
> +				return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +#define elf_check_module_arch elf_check_module_arch
> +
>  /*
>   * Select ELF headers.
>   * Make empty section for module_frob_arch_sections to expand.
Shouldn't this part go to the second patch?

Thanks

Michal
> diff --git a/arch/powerpc/kernel/vdso64/Makefile b/arch/powerpc/kernel/vdso64/Makefile
> index 2813e3f98db6..d783c07e558f 100644
> --- a/arch/powerpc/kernel/vdso64/Makefile
> +++ b/arch/powerpc/kernel/vdso64/Makefile
> @@ -25,6 +25,19 @@ KCOV_INSTRUMENT := n
>  UBSAN_SANITIZE := n
>  KASAN_SANITIZE := n
>  
> +# Always build vdso64 with ELFv1 ABI for BE kernels
> +ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
> +ifdef CONFIG_CPU_BIG_ENDIAN
> +KBUILD_CFLAGS := $(filter-out -mabi=elfv2,$(KBUILD_CFLAGS))
> +KBUILD_AFLAGS := $(filter-out -mabi=elfv2,$(KBUILD_AFLAGS))
> +
> +# These are derived from arch/powerpc/Makefile
> +KBUILD_CFLAGS += $(call cc-option,-mabi=elfv1)
> +KBUILD_CFLAGS += $(call cc-option,-mcall-aixdesc)
> +KBUILD_AFLAGS += $(call cc-option,-mabi=elfv1)
> +endif
> +endif
> +
>  ccflags-y := -shared -fno-common -fno-builtin -nostdlib \
>  	-Wl,-soname=linux-vdso64.so.1 -Wl,--hash-style=both
>  asflags-y := -D__VDSO64__ -s
> diff --git a/drivers/crypto/vmx/Makefile b/drivers/crypto/vmx/Makefile
> index 709670d2b553..d9ccf9fc3483 100644
> --- a/drivers/crypto/vmx/Makefile
> +++ b/drivers/crypto/vmx/Makefile
> @@ -5,18 +5,22 @@ vmx-crypto-objs := vmx.o aesp8-ppc.o ghashp8-ppc.o aes.o aes_cbc.o aes_ctr.o aes
>  ifeq ($(CONFIG_CPU_LITTLE_ENDIAN),y)
>  override flavour := linux-ppc64le
>  else
> +ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
> +override flavour := linux-ppc64-elfv2
> +else
>  override flavour := linux-ppc64
>  endif
> +endif
>  
>  quiet_cmd_perl = PERL $@
>        cmd_perl = $(PERL) $(<) $(flavour) > $(@)
>  
>  targets += aesp8-ppc.S ghashp8-ppc.S
>  
> -$(obj)/aesp8-ppc.S: $(src)/aesp8-ppc.pl FORCE
> +$(obj)/aesp8-ppc.S: $(src)/aesp8-ppc.pl $(src)/ppc-xlate.pl FORCE
>  	$(call if_changed,perl)
>    
> -$(obj)/ghashp8-ppc.S: $(src)/ghashp8-ppc.pl FORCE
> +$(obj)/ghashp8-ppc.S: $(src)/ghashp8-ppc.pl $(src)/ppc-xlate.pl FORCE
>  	$(call if_changed,perl)
>  
>  clean-files := aesp8-ppc.S ghashp8-ppc.S
> diff --git a/drivers/crypto/vmx/ppc-xlate.pl b/drivers/crypto/vmx/ppc-xlate.pl
> index 36db2ef09e5b..b583898c11ae 100644
> --- a/drivers/crypto/vmx/ppc-xlate.pl
> +++ b/drivers/crypto/vmx/ppc-xlate.pl
> @@ -9,6 +9,8 @@ open STDOUT,">$output" || die "can't open $output: $!";
>  
>  my %GLOBALS;
>  my $dotinlocallabels=($flavour=~/linux/)?1:0;
> +my $elfv2abi=(($flavour =~ /linux-ppc64le/) or ($flavour =~ /linux-ppc64-elfv2/))?1:0;
> +my $dotfunctions=($elfv2abi=~1)?0:1;
>  
>  ################################################################
>  # directives which need special treatment on different platforms
> @@ -40,7 +42,7 @@ my $globl = sub {
>  };
>  my $text = sub {
>      my $ret = ($flavour =~ /aix/) ? ".csect\t.text[PR],7" : ".text";
> -    $ret = ".abiversion	2\n".$ret	if ($flavour =~ /linux.*64le/);
> +    $ret = ".abiversion	2\n".$ret	if ($elfv2abi);
>      $ret;
>  };
>  my $machine = sub {
> @@ -56,8 +58,8 @@ my $size = sub {
>      if ($flavour =~ /linux/)
>      {	shift;
>  	my $name = shift; $name =~ s|^[\.\_]||;
> -	my $ret  = ".size	$name,.-".($flavour=~/64$/?".":"").$name;
> -	$ret .= "\n.size	.$name,.-.$name" if ($flavour=~/64$/);
> +	my $ret  = ".size	$name,.-".($dotfunctions?".":"").$name;
> +	$ret .= "\n.size	.$name,.-.$name" if ($dotfunctions);
>  	$ret;
>      }
>      else
> @@ -142,7 +144,7 @@ my $vmr = sub {
>  
>  # Some ABIs specify vrsave, special-purpose register #256, as reserved
>  # for system use.
> -my $no_vrsave = ($flavour =~ /linux-ppc64le/);
> +my $no_vrsave = ($elfv2abi);
>  my $mtspr = sub {
>      my ($f,$idx,$ra) = @_;
>      if ($idx == 256 && $no_vrsave) {
> -- 
> 2.23.0
> 

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

* Re: [PATCH v4 2/2] powerpc/64: Option to use ELF V2 ABI for big-endian kernels
@ 2021-06-11  9:58     ` Michal Suchánek
  0 siblings, 0 replies; 33+ messages in thread
From: Michal Suchánek @ 2021-06-11  9:58 UTC (permalink / raw)
  To: Nicholas Piggin; +Cc: linuxppc-dev, linux-kernel, Jessica Yu

On Fri, Jun 11, 2021 at 07:39:59PM +1000, Nicholas Piggin wrote:
> Provide an option to build big-endian kernels using the ELFv2 ABI. This
> works on GCC only so far, although it is rumored to work with clang
> that's not been tested yet. A new module version check ensures the
> module ELF ABI level matches the kernel build.
> 
> This can give big-endian kernels some useful advantages of the ELFv2 ABI
> (e.g., less stack usage, -mprofile-kernel, better compatibility with eBPF
> tools).
> 
> BE+ELFv2 is not officially supported by the GNU toolchain, but it works
> fine in testing and has been used by some userspace for some time (e.g.,
> Void Linux).
> 
> Tested-by: Michal Suchánek <msuchanek@suse.de>
> Reviewed-by: Segher Boessenkool <segher@kernel.crashing.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/Kconfig                | 22 ++++++++++++++++++++++
>  arch/powerpc/Makefile               | 18 ++++++++++++------
>  arch/powerpc/boot/Makefile          |  4 +++-
>  arch/powerpc/include/asm/module.h   | 24 ++++++++++++++++++++++++
>  arch/powerpc/kernel/vdso64/Makefile | 13 +++++++++++++
>  drivers/crypto/vmx/Makefile         |  8 ++++++--
>  drivers/crypto/vmx/ppc-xlate.pl     | 10 ++++++----
>  7 files changed, 86 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 088dd2afcfe4..093f973a28b9 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -163,6 +163,7 @@ config PPC
>  	select ARCH_WEAK_RELEASE_ACQUIRE
>  	select BINFMT_ELF
>  	select BUILDTIME_TABLE_SORT
> +	select PPC64_BUILD_ELF_V2_ABI		if PPC64 && CPU_LITTLE_ENDIAN
>  	select CLONE_BACKWARDS
>  	select DCACHE_WORD_ACCESS		if PPC64 && CPU_LITTLE_ENDIAN
>  	select DMA_OPS_BYPASS			if PPC64
> @@ -561,6 +562,27 @@ config KEXEC_FILE
>  config ARCH_HAS_KEXEC_PURGATORY
>  	def_bool KEXEC_FILE
>  
> +config PPC64_BUILD_ELF_V2_ABI
> +	bool
> +
> +config PPC64_BUILD_BIG_ENDIAN_ELF_V2_ABI
> +	bool "Build big-endian kernel using ELF ABI V2 (EXPERIMENTAL)"
> +	depends on PPC64 && CPU_BIG_ENDIAN && EXPERT
> +	depends on CC_IS_GCC && LD_VERSION >= 22400
> +	default n
> +	select PPC64_BUILD_ELF_V2_ABI
> +	help
> +	  This builds the kernel image using the "Power Architecture 64-Bit ELF
> +	  V2 ABI Specification", which has a reduced stack overhead and faster
> +	  function calls. This internal kernel ABI option does not affect
> +          userspace compatibility.
> +
> +	  The V2 ABI is standard for 64-bit little-endian, but for big-endian
> +	  it is less well tested by kernel and toolchain. However some distros
> +	  build userspace this way, and it can produce a functioning kernel.
> +
> +	  This requires GCC and binutils 2.24 or newer.
> +
>  config RELOCATABLE
>  	bool "Build a relocatable kernel"
>  	depends on PPC64 || (FLATMEM && (44x || FSL_BOOKE))
> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> index 3212d076ac6a..b90b5cb799aa 100644
> --- a/arch/powerpc/Makefile
> +++ b/arch/powerpc/Makefile
> @@ -91,10 +91,14 @@ endif
>  
>  ifdef CONFIG_PPC64
>  ifndef CONFIG_CC_IS_CLANG
> -cflags-$(CONFIG_CPU_BIG_ENDIAN)		+= $(call cc-option,-mabi=elfv1)
> -cflags-$(CONFIG_CPU_BIG_ENDIAN)		+= $(call cc-option,-mcall-aixdesc)
> -aflags-$(CONFIG_CPU_BIG_ENDIAN)		+= $(call cc-option,-mabi=elfv1)
> -aflags-$(CONFIG_CPU_LITTLE_ENDIAN)	+= -mabi=elfv2
> +ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
> +cflags-y				+= $(call cc-option,-mabi=elfv2)
> +aflags-y				+= $(call cc-option,-mabi=elfv2)
> +else
> +cflags-y				+= $(call cc-option,-mabi=elfv1)
> +cflags-y				+= $(call cc-option,-mcall-aixdesc)
> +aflags-y				+= $(call cc-option,-mabi=elfv1)
> +endif
>  endif
>  endif
>  
> @@ -142,15 +146,17 @@ endif
>  
>  CFLAGS-$(CONFIG_PPC64)	:= $(call cc-option,-mtraceback=no)
>  ifndef CONFIG_CC_IS_CLANG
> -ifdef CONFIG_CPU_LITTLE_ENDIAN
> -CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv2,$(call cc-option,-mcall-aixdesc))
> +ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
> +CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv2)
>  AFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv2)
>  else
> +# Keep these in synch with arch/powerpc/kernel/vdso64/Makefile
>  CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv1)
>  CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mcall-aixdesc)
>  AFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv1)
>  endif
>  endif
> +
>  CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mcmodel=medium,$(call cc-option,-mminimal-toc))
>  CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mno-pointers-to-nested-functions)
>  
> diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile
> index 2b8da923ceca..be84a72f8258 100644
> --- a/arch/powerpc/boot/Makefile
> +++ b/arch/powerpc/boot/Makefile
> @@ -40,6 +40,9 @@ BOOTCFLAGS    := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
>  
>  ifdef CONFIG_PPC64_BOOT_WRAPPER
>  BOOTCFLAGS	+= -m64
> +ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
> +BOOTCFLAGS	+= $(call cc-option,-mabi=elfv2)
> +endif
>  else
>  BOOTCFLAGS	+= -m32
>  endif
> @@ -50,7 +53,6 @@ ifdef CONFIG_CPU_BIG_ENDIAN
>  BOOTCFLAGS	+= -mbig-endian
>  else
>  BOOTCFLAGS	+= -mlittle-endian
> -BOOTCFLAGS	+= $(call cc-option,-mabi=elfv2)
>  endif
>  
>  BOOTAFLAGS	:= -D__ASSEMBLY__ $(BOOTCFLAGS) -nostdinc
> diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
> index 857d9ff24295..043e11068ff4 100644
> --- a/arch/powerpc/include/asm/module.h
> +++ b/arch/powerpc/include/asm/module.h
> @@ -52,6 +52,30 @@ struct mod_arch_specific {
>  	unsigned int num_bugs;
>  };
>  
> +/*
> + * Check kernel module ELF header architecture specific compatibility.
> + */
> +static inline bool elf_check_module_arch(Elf_Ehdr *hdr)
> +{
> +	if (!elf_check_arch(hdr))
> +		return false;
> +
> +	if (IS_ENABLED(CONFIG_PPC64)) {
> +		unsigned long abi_level = hdr->e_flags & 0x3;
> +
> +		if (IS_ENABLED(CONFIG_PPC64_BUILD_ELF_V2_ABI)) {
> +			if (abi_level != 2)
> +				return false;
> +		} else {
> +			if (abi_level >= 2)
> +				return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +#define elf_check_module_arch elf_check_module_arch
> +
>  /*
>   * Select ELF headers.
>   * Make empty section for module_frob_arch_sections to expand.
Shouldn't this part go to the second patch?

Thanks

Michal
> diff --git a/arch/powerpc/kernel/vdso64/Makefile b/arch/powerpc/kernel/vdso64/Makefile
> index 2813e3f98db6..d783c07e558f 100644
> --- a/arch/powerpc/kernel/vdso64/Makefile
> +++ b/arch/powerpc/kernel/vdso64/Makefile
> @@ -25,6 +25,19 @@ KCOV_INSTRUMENT := n
>  UBSAN_SANITIZE := n
>  KASAN_SANITIZE := n
>  
> +# Always build vdso64 with ELFv1 ABI for BE kernels
> +ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
> +ifdef CONFIG_CPU_BIG_ENDIAN
> +KBUILD_CFLAGS := $(filter-out -mabi=elfv2,$(KBUILD_CFLAGS))
> +KBUILD_AFLAGS := $(filter-out -mabi=elfv2,$(KBUILD_AFLAGS))
> +
> +# These are derived from arch/powerpc/Makefile
> +KBUILD_CFLAGS += $(call cc-option,-mabi=elfv1)
> +KBUILD_CFLAGS += $(call cc-option,-mcall-aixdesc)
> +KBUILD_AFLAGS += $(call cc-option,-mabi=elfv1)
> +endif
> +endif
> +
>  ccflags-y := -shared -fno-common -fno-builtin -nostdlib \
>  	-Wl,-soname=linux-vdso64.so.1 -Wl,--hash-style=both
>  asflags-y := -D__VDSO64__ -s
> diff --git a/drivers/crypto/vmx/Makefile b/drivers/crypto/vmx/Makefile
> index 709670d2b553..d9ccf9fc3483 100644
> --- a/drivers/crypto/vmx/Makefile
> +++ b/drivers/crypto/vmx/Makefile
> @@ -5,18 +5,22 @@ vmx-crypto-objs := vmx.o aesp8-ppc.o ghashp8-ppc.o aes.o aes_cbc.o aes_ctr.o aes
>  ifeq ($(CONFIG_CPU_LITTLE_ENDIAN),y)
>  override flavour := linux-ppc64le
>  else
> +ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
> +override flavour := linux-ppc64-elfv2
> +else
>  override flavour := linux-ppc64
>  endif
> +endif
>  
>  quiet_cmd_perl = PERL $@
>        cmd_perl = $(PERL) $(<) $(flavour) > $(@)
>  
>  targets += aesp8-ppc.S ghashp8-ppc.S
>  
> -$(obj)/aesp8-ppc.S: $(src)/aesp8-ppc.pl FORCE
> +$(obj)/aesp8-ppc.S: $(src)/aesp8-ppc.pl $(src)/ppc-xlate.pl FORCE
>  	$(call if_changed,perl)
>    
> -$(obj)/ghashp8-ppc.S: $(src)/ghashp8-ppc.pl FORCE
> +$(obj)/ghashp8-ppc.S: $(src)/ghashp8-ppc.pl $(src)/ppc-xlate.pl FORCE
>  	$(call if_changed,perl)
>  
>  clean-files := aesp8-ppc.S ghashp8-ppc.S
> diff --git a/drivers/crypto/vmx/ppc-xlate.pl b/drivers/crypto/vmx/ppc-xlate.pl
> index 36db2ef09e5b..b583898c11ae 100644
> --- a/drivers/crypto/vmx/ppc-xlate.pl
> +++ b/drivers/crypto/vmx/ppc-xlate.pl
> @@ -9,6 +9,8 @@ open STDOUT,">$output" || die "can't open $output: $!";
>  
>  my %GLOBALS;
>  my $dotinlocallabels=($flavour=~/linux/)?1:0;
> +my $elfv2abi=(($flavour =~ /linux-ppc64le/) or ($flavour =~ /linux-ppc64-elfv2/))?1:0;
> +my $dotfunctions=($elfv2abi=~1)?0:1;
>  
>  ################################################################
>  # directives which need special treatment on different platforms
> @@ -40,7 +42,7 @@ my $globl = sub {
>  };
>  my $text = sub {
>      my $ret = ($flavour =~ /aix/) ? ".csect\t.text[PR],7" : ".text";
> -    $ret = ".abiversion	2\n".$ret	if ($flavour =~ /linux.*64le/);
> +    $ret = ".abiversion	2\n".$ret	if ($elfv2abi);
>      $ret;
>  };
>  my $machine = sub {
> @@ -56,8 +58,8 @@ my $size = sub {
>      if ($flavour =~ /linux/)
>      {	shift;
>  	my $name = shift; $name =~ s|^[\.\_]||;
> -	my $ret  = ".size	$name,.-".($flavour=~/64$/?".":"").$name;
> -	$ret .= "\n.size	.$name,.-.$name" if ($flavour=~/64$/);
> +	my $ret  = ".size	$name,.-".($dotfunctions?".":"").$name;
> +	$ret .= "\n.size	.$name,.-.$name" if ($dotfunctions);
>  	$ret;
>      }
>      else
> @@ -142,7 +144,7 @@ my $vmr = sub {
>  
>  # Some ABIs specify vrsave, special-purpose register #256, as reserved
>  # for system use.
> -my $no_vrsave = ($flavour =~ /linux-ppc64le/);
> +my $no_vrsave = ($elfv2abi);
>  my $mtspr = sub {
>      my ($f,$idx,$ra) = @_;
>      if ($idx == 256 && $no_vrsave) {
> -- 
> 2.23.0
> 

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

* Re: [PATCH v4 2/2] powerpc/64: Option to use ELF V2 ABI for big-endian kernels
  2021-06-11  9:58     ` Michal Suchánek
  (?)
@ 2021-06-11 10:20     ` Michal Suchánek
  -1 siblings, 0 replies; 33+ messages in thread
From: Michal Suchánek @ 2021-06-11 10:20 UTC (permalink / raw)
  To: Nicholas Piggin; +Cc: linuxppc-dev, linux-kernel, Jessica Yu

On Fri, Jun 11, 2021 at 11:58:19AM +0200, Michal Suchánek wrote:
> On Fri, Jun 11, 2021 at 07:39:59PM +1000, Nicholas Piggin wrote:
> > Provide an option to build big-endian kernels using the ELFv2 ABI. This
> > works on GCC only so far, although it is rumored to work with clang
> > that's not been tested yet. A new module version check ensures the
> > module ELF ABI level matches the kernel build.
> > 
> > This can give big-endian kernels some useful advantages of the ELFv2 ABI
> > (e.g., less stack usage, -mprofile-kernel, better compatibility with eBPF
> > tools).
> > 
> > BE+ELFv2 is not officially supported by the GNU toolchain, but it works
> > fine in testing and has been used by some userspace for some time (e.g.,
> > Void Linux).
> > 
> > Tested-by: Michal Suchánek <msuchanek@suse.de>
> > Reviewed-by: Segher Boessenkool <segher@kernel.crashing.org>
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  arch/powerpc/Kconfig                | 22 ++++++++++++++++++++++
> >  arch/powerpc/Makefile               | 18 ++++++++++++------
> >  arch/powerpc/boot/Makefile          |  4 +++-
> >  arch/powerpc/include/asm/module.h   | 24 ++++++++++++++++++++++++
> >  arch/powerpc/kernel/vdso64/Makefile | 13 +++++++++++++
> >  drivers/crypto/vmx/Makefile         |  8 ++++++--
> >  drivers/crypto/vmx/ppc-xlate.pl     | 10 ++++++----
> >  7 files changed, 86 insertions(+), 13 deletions(-)
> > 
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index 088dd2afcfe4..093f973a28b9 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -163,6 +163,7 @@ config PPC
> >  	select ARCH_WEAK_RELEASE_ACQUIRE
> >  	select BINFMT_ELF
> >  	select BUILDTIME_TABLE_SORT
> > +	select PPC64_BUILD_ELF_V2_ABI		if PPC64 && CPU_LITTLE_ENDIAN
> >  	select CLONE_BACKWARDS
> >  	select DCACHE_WORD_ACCESS		if PPC64 && CPU_LITTLE_ENDIAN
> >  	select DMA_OPS_BYPASS			if PPC64
> > @@ -561,6 +562,27 @@ config KEXEC_FILE
> >  config ARCH_HAS_KEXEC_PURGATORY
> >  	def_bool KEXEC_FILE
> >  
> > +config PPC64_BUILD_ELF_V2_ABI
> > +	bool
> > +
> > +config PPC64_BUILD_BIG_ENDIAN_ELF_V2_ABI
> > +	bool "Build big-endian kernel using ELF ABI V2 (EXPERIMENTAL)"
> > +	depends on PPC64 && CPU_BIG_ENDIAN && EXPERT
> > +	depends on CC_IS_GCC && LD_VERSION >= 22400
> > +	default n
> > +	select PPC64_BUILD_ELF_V2_ABI
> > +	help
> > +	  This builds the kernel image using the "Power Architecture 64-Bit ELF
> > +	  V2 ABI Specification", which has a reduced stack overhead and faster
> > +	  function calls. This internal kernel ABI option does not affect
> > +          userspace compatibility.
> > +
> > +	  The V2 ABI is standard for 64-bit little-endian, but for big-endian
> > +	  it is less well tested by kernel and toolchain. However some distros
> > +	  build userspace this way, and it can produce a functioning kernel.
> > +
> > +	  This requires GCC and binutils 2.24 or newer.
> > +
> >  config RELOCATABLE
> >  	bool "Build a relocatable kernel"
> >  	depends on PPC64 || (FLATMEM && (44x || FSL_BOOKE))
> > diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> > index 3212d076ac6a..b90b5cb799aa 100644
> > --- a/arch/powerpc/Makefile
> > +++ b/arch/powerpc/Makefile
> > @@ -91,10 +91,14 @@ endif
> >  
> >  ifdef CONFIG_PPC64
> >  ifndef CONFIG_CC_IS_CLANG
> > -cflags-$(CONFIG_CPU_BIG_ENDIAN)		+= $(call cc-option,-mabi=elfv1)
> > -cflags-$(CONFIG_CPU_BIG_ENDIAN)		+= $(call cc-option,-mcall-aixdesc)
> > -aflags-$(CONFIG_CPU_BIG_ENDIAN)		+= $(call cc-option,-mabi=elfv1)
> > -aflags-$(CONFIG_CPU_LITTLE_ENDIAN)	+= -mabi=elfv2
> > +ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
> > +cflags-y				+= $(call cc-option,-mabi=elfv2)
> > +aflags-y				+= $(call cc-option,-mabi=elfv2)
> > +else
> > +cflags-y				+= $(call cc-option,-mabi=elfv1)
> > +cflags-y				+= $(call cc-option,-mcall-aixdesc)
> > +aflags-y				+= $(call cc-option,-mabi=elfv1)
> > +endif
> >  endif
> >  endif
> >  
> > @@ -142,15 +146,17 @@ endif
> >  
> >  CFLAGS-$(CONFIG_PPC64)	:= $(call cc-option,-mtraceback=no)
> >  ifndef CONFIG_CC_IS_CLANG
> > -ifdef CONFIG_CPU_LITTLE_ENDIAN
> > -CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv2,$(call cc-option,-mcall-aixdesc))
> > +ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
> > +CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv2)
> >  AFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv2)
> >  else
> > +# Keep these in synch with arch/powerpc/kernel/vdso64/Makefile
> >  CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv1)
> >  CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mcall-aixdesc)
> >  AFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mabi=elfv1)
> >  endif
> >  endif
> > +
> >  CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mcmodel=medium,$(call cc-option,-mminimal-toc))
> >  CFLAGS-$(CONFIG_PPC64)	+= $(call cc-option,-mno-pointers-to-nested-functions)
> >  
> > diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile
> > index 2b8da923ceca..be84a72f8258 100644
> > --- a/arch/powerpc/boot/Makefile
> > +++ b/arch/powerpc/boot/Makefile
> > @@ -40,6 +40,9 @@ BOOTCFLAGS    := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
> >  
> >  ifdef CONFIG_PPC64_BOOT_WRAPPER
> >  BOOTCFLAGS	+= -m64
> > +ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
> > +BOOTCFLAGS	+= $(call cc-option,-mabi=elfv2)
> > +endif
> >  else
> >  BOOTCFLAGS	+= -m32
> >  endif
> > @@ -50,7 +53,6 @@ ifdef CONFIG_CPU_BIG_ENDIAN
> >  BOOTCFLAGS	+= -mbig-endian
> >  else
> >  BOOTCFLAGS	+= -mlittle-endian
> > -BOOTCFLAGS	+= $(call cc-option,-mabi=elfv2)
> >  endif
> >  
> >  BOOTAFLAGS	:= -D__ASSEMBLY__ $(BOOTCFLAGS) -nostdinc
> > diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
> > index 857d9ff24295..043e11068ff4 100644
> > --- a/arch/powerpc/include/asm/module.h
> > +++ b/arch/powerpc/include/asm/module.h
> > @@ -52,6 +52,30 @@ struct mod_arch_specific {
> >  	unsigned int num_bugs;
> >  };
> >  
> > +/*
> > + * Check kernel module ELF header architecture specific compatibility.
> > + */
> > +static inline bool elf_check_module_arch(Elf_Ehdr *hdr)
> > +{
> > +	if (!elf_check_arch(hdr))
> > +		return false;
> > +
> > +	if (IS_ENABLED(CONFIG_PPC64)) {
> > +		unsigned long abi_level = hdr->e_flags & 0x3;
> > +
> > +		if (IS_ENABLED(CONFIG_PPC64_BUILD_ELF_V2_ABI)) {
> > +			if (abi_level != 2)
> > +				return false;
> > +		} else {
> > +			if (abi_level >= 2)
> > +				return false;
> > +		}
> > +	}
> > +
> > +	return true;
> > +}
> > +#define elf_check_module_arch elf_check_module_arch
> > +
> >  /*
> >   * Select ELF headers.
> >   * Make empty section for module_frob_arch_sections to expand.
> Shouldn't this part go to the second patch?
Nevermind, this is the second patch now.
> 
> Thanks
> 
> Michal
> > diff --git a/arch/powerpc/kernel/vdso64/Makefile b/arch/powerpc/kernel/vdso64/Makefile
> > index 2813e3f98db6..d783c07e558f 100644
> > --- a/arch/powerpc/kernel/vdso64/Makefile
> > +++ b/arch/powerpc/kernel/vdso64/Makefile
> > @@ -25,6 +25,19 @@ KCOV_INSTRUMENT := n
> >  UBSAN_SANITIZE := n
> >  KASAN_SANITIZE := n
> >  
> > +# Always build vdso64 with ELFv1 ABI for BE kernels
> > +ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
> > +ifdef CONFIG_CPU_BIG_ENDIAN
> > +KBUILD_CFLAGS := $(filter-out -mabi=elfv2,$(KBUILD_CFLAGS))
> > +KBUILD_AFLAGS := $(filter-out -mabi=elfv2,$(KBUILD_AFLAGS))
> > +
> > +# These are derived from arch/powerpc/Makefile
> > +KBUILD_CFLAGS += $(call cc-option,-mabi=elfv1)
> > +KBUILD_CFLAGS += $(call cc-option,-mcall-aixdesc)
> > +KBUILD_AFLAGS += $(call cc-option,-mabi=elfv1)
> > +endif
> > +endif
> > +
> >  ccflags-y := -shared -fno-common -fno-builtin -nostdlib \
> >  	-Wl,-soname=linux-vdso64.so.1 -Wl,--hash-style=both
> >  asflags-y := -D__VDSO64__ -s
> > diff --git a/drivers/crypto/vmx/Makefile b/drivers/crypto/vmx/Makefile
> > index 709670d2b553..d9ccf9fc3483 100644
> > --- a/drivers/crypto/vmx/Makefile
> > +++ b/drivers/crypto/vmx/Makefile
> > @@ -5,18 +5,22 @@ vmx-crypto-objs := vmx.o aesp8-ppc.o ghashp8-ppc.o aes.o aes_cbc.o aes_ctr.o aes
> >  ifeq ($(CONFIG_CPU_LITTLE_ENDIAN),y)
> >  override flavour := linux-ppc64le
> >  else
> > +ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
> > +override flavour := linux-ppc64-elfv2
> > +else
> >  override flavour := linux-ppc64
> >  endif
> > +endif
> >  
> >  quiet_cmd_perl = PERL $@
> >        cmd_perl = $(PERL) $(<) $(flavour) > $(@)
> >  
> >  targets += aesp8-ppc.S ghashp8-ppc.S
> >  
> > -$(obj)/aesp8-ppc.S: $(src)/aesp8-ppc.pl FORCE
> > +$(obj)/aesp8-ppc.S: $(src)/aesp8-ppc.pl $(src)/ppc-xlate.pl FORCE
> >  	$(call if_changed,perl)
> >    
> > -$(obj)/ghashp8-ppc.S: $(src)/ghashp8-ppc.pl FORCE
> > +$(obj)/ghashp8-ppc.S: $(src)/ghashp8-ppc.pl $(src)/ppc-xlate.pl FORCE
> >  	$(call if_changed,perl)
> >  
> >  clean-files := aesp8-ppc.S ghashp8-ppc.S
> > diff --git a/drivers/crypto/vmx/ppc-xlate.pl b/drivers/crypto/vmx/ppc-xlate.pl
> > index 36db2ef09e5b..b583898c11ae 100644
> > --- a/drivers/crypto/vmx/ppc-xlate.pl
> > +++ b/drivers/crypto/vmx/ppc-xlate.pl
> > @@ -9,6 +9,8 @@ open STDOUT,">$output" || die "can't open $output: $!";
> >  
> >  my %GLOBALS;
> >  my $dotinlocallabels=($flavour=~/linux/)?1:0;
> > +my $elfv2abi=(($flavour =~ /linux-ppc64le/) or ($flavour =~ /linux-ppc64-elfv2/))?1:0;
> > +my $dotfunctions=($elfv2abi=~1)?0:1;
> >  
> >  ################################################################
> >  # directives which need special treatment on different platforms
> > @@ -40,7 +42,7 @@ my $globl = sub {
> >  };
> >  my $text = sub {
> >      my $ret = ($flavour =~ /aix/) ? ".csect\t.text[PR],7" : ".text";
> > -    $ret = ".abiversion	2\n".$ret	if ($flavour =~ /linux.*64le/);
> > +    $ret = ".abiversion	2\n".$ret	if ($elfv2abi);
> >      $ret;
> >  };
> >  my $machine = sub {
> > @@ -56,8 +58,8 @@ my $size = sub {
> >      if ($flavour =~ /linux/)
> >      {	shift;
> >  	my $name = shift; $name =~ s|^[\.\_]||;
> > -	my $ret  = ".size	$name,.-".($flavour=~/64$/?".":"").$name;
> > -	$ret .= "\n.size	.$name,.-.$name" if ($flavour=~/64$/);
> > +	my $ret  = ".size	$name,.-".($dotfunctions?".":"").$name;
> > +	$ret .= "\n.size	.$name,.-.$name" if ($dotfunctions);
> >  	$ret;
> >      }
> >      else
> > @@ -142,7 +144,7 @@ my $vmr = sub {
> >  
> >  # Some ABIs specify vrsave, special-purpose register #256, as reserved
> >  # for system use.
> > -my $no_vrsave = ($flavour =~ /linux-ppc64le/);
> > +my $no_vrsave = ($elfv2abi);
> >  my $mtspr = sub {
> >      my ($f,$idx,$ra) = @_;
> >      if ($idx == 256 && $no_vrsave) {
> > -- 
> > 2.23.0
> > 

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
  2021-06-11  9:39   ` Nicholas Piggin
@ 2021-06-14 12:06     ` Jessica Yu
  -1 siblings, 0 replies; 33+ messages in thread
From: Jessica Yu @ 2021-06-14 12:06 UTC (permalink / raw)
  To: Nicholas Piggin
  Cc: linuxppc-dev, linux-kernel, Michal Suchánek, Michael Ellerman

+++ Nicholas Piggin [11/06/21 19:39 +1000]:
>The elf_check_arch() function is used to test usermode binaries, but
>kernel modules may have more specific requirements. powerpc would like
>to test for ABI version compatibility.
>
>Add an arch-overridable function elf_check_module_arch() that defaults
>to elf_check_arch() and use it in elf_validity_check().
>
>Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>[np: split patch, added changelog]
>Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>---
> include/linux/moduleloader.h | 5 +++++
> kernel/module.c              | 2 +-
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>index 9e09d11ffe5b..fdc042a84562 100644
>--- a/include/linux/moduleloader.h
>+++ b/include/linux/moduleloader.h
>@@ -13,6 +13,11 @@
>  * must be implemented by each architecture.
>  */
>
>+// Allow arch to optionally do additional checking of module ELF header
>+#ifndef elf_check_module_arch
>+#define elf_check_module_arch elf_check_arch
>+#endif

Hi Nicholas,

Why not make elf_check_module_arch() consistent with the other
arch-specific functions? Please see module_frob_arch_sections(),
module_{init,exit}_section(), etc in moduleloader.h. That is, they are
all __weak functions that are overridable by arches. We can maybe make
elf_check_module_arch() a weak symbol, available for arches to
override if they want to perform additional elf checks. Then we don't
have to have this one-off #define.

Thanks,

Jessica

>+
> /* Adjust arch-specific sections.  Return 0 on success.  */
> int module_frob_arch_sections(Elf_Ehdr *hdr,
> 			      Elf_Shdr *sechdrs,
>diff --git a/kernel/module.c b/kernel/module.c
>index 7e78dfabca97..7c3f9b7478dc 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -2946,7 +2946,7 @@ static int elf_validity_check(struct load_info *info)
>
> 	if (memcmp(info->hdr->e_ident, ELFMAG, SELFMAG) != 0
> 	    || info->hdr->e_type != ET_REL
>-	    || !elf_check_arch(info->hdr)
>+	    || !elf_check_module_arch(info->hdr)
> 	    || info->hdr->e_shentsize != sizeof(Elf_Shdr))
> 		return -ENOEXEC;
>
>-- 
>2.23.0
>

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
@ 2021-06-14 12:06     ` Jessica Yu
  0 siblings, 0 replies; 33+ messages in thread
From: Jessica Yu @ 2021-06-14 12:06 UTC (permalink / raw)
  To: Nicholas Piggin; +Cc: Michal Suchánek, linuxppc-dev, linux-kernel

+++ Nicholas Piggin [11/06/21 19:39 +1000]:
>The elf_check_arch() function is used to test usermode binaries, but
>kernel modules may have more specific requirements. powerpc would like
>to test for ABI version compatibility.
>
>Add an arch-overridable function elf_check_module_arch() that defaults
>to elf_check_arch() and use it in elf_validity_check().
>
>Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>[np: split patch, added changelog]
>Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>---
> include/linux/moduleloader.h | 5 +++++
> kernel/module.c              | 2 +-
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>index 9e09d11ffe5b..fdc042a84562 100644
>--- a/include/linux/moduleloader.h
>+++ b/include/linux/moduleloader.h
>@@ -13,6 +13,11 @@
>  * must be implemented by each architecture.
>  */
>
>+// Allow arch to optionally do additional checking of module ELF header
>+#ifndef elf_check_module_arch
>+#define elf_check_module_arch elf_check_arch
>+#endif

Hi Nicholas,

Why not make elf_check_module_arch() consistent with the other
arch-specific functions? Please see module_frob_arch_sections(),
module_{init,exit}_section(), etc in moduleloader.h. That is, they are
all __weak functions that are overridable by arches. We can maybe make
elf_check_module_arch() a weak symbol, available for arches to
override if they want to perform additional elf checks. Then we don't
have to have this one-off #define.

Thanks,

Jessica

>+
> /* Adjust arch-specific sections.  Return 0 on success.  */
> int module_frob_arch_sections(Elf_Ehdr *hdr,
> 			      Elf_Shdr *sechdrs,
>diff --git a/kernel/module.c b/kernel/module.c
>index 7e78dfabca97..7c3f9b7478dc 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -2946,7 +2946,7 @@ static int elf_validity_check(struct load_info *info)
>
> 	if (memcmp(info->hdr->e_ident, ELFMAG, SELFMAG) != 0
> 	    || info->hdr->e_type != ET_REL
>-	    || !elf_check_arch(info->hdr)
>+	    || !elf_check_module_arch(info->hdr)
> 	    || info->hdr->e_shentsize != sizeof(Elf_Shdr))
> 		return -ENOEXEC;
>
>-- 
>2.23.0
>

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
  2021-06-14 12:06     ` Jessica Yu
@ 2021-06-15  2:05       ` Nicholas Piggin
  -1 siblings, 0 replies; 33+ messages in thread
From: Nicholas Piggin @ 2021-06-15  2:05 UTC (permalink / raw)
  To: Jessica Yu
  Cc: linux-kernel, linuxppc-dev, Michael Ellerman, Michal Suchánek

Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:
> +++ Nicholas Piggin [11/06/21 19:39 +1000]:
>>The elf_check_arch() function is used to test usermode binaries, but
>>kernel modules may have more specific requirements. powerpc would like
>>to test for ABI version compatibility.
>>
>>Add an arch-overridable function elf_check_module_arch() that defaults
>>to elf_check_arch() and use it in elf_validity_check().
>>
>>Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>[np: split patch, added changelog]
>>Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>---
>> include/linux/moduleloader.h | 5 +++++
>> kernel/module.c              | 2 +-
>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>
>>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>>index 9e09d11ffe5b..fdc042a84562 100644
>>--- a/include/linux/moduleloader.h
>>+++ b/include/linux/moduleloader.h
>>@@ -13,6 +13,11 @@
>>  * must be implemented by each architecture.
>>  */
>>
>>+// Allow arch to optionally do additional checking of module ELF header
>>+#ifndef elf_check_module_arch
>>+#define elf_check_module_arch elf_check_arch
>>+#endif
> 
> Hi Nicholas,
> 
> Why not make elf_check_module_arch() consistent with the other
> arch-specific functions? Please see module_frob_arch_sections(),
> module_{init,exit}_section(), etc in moduleloader.h. That is, they are
> all __weak functions that are overridable by arches. We can maybe make
> elf_check_module_arch() a weak symbol, available for arches to
> override if they want to perform additional elf checks. Then we don't
> have to have this one-off #define.


Like this? I like it. Good idea.

Thanks,
Nick

diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 9e09d11ffe5b..7b4587a19189 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -13,6 +13,9 @@
  * must be implemented by each architecture.
  */
 
+/* arch may override to do additional checking of ELF header architecture */
+bool module_elf_check_arch(Elf_Ehdr *hdr);
+
 /* Adjust arch-specific sections.  Return 0 on success.  */
 int module_frob_arch_sections(Elf_Ehdr *hdr,
 			      Elf_Shdr *sechdrs,
diff --git a/kernel/module.c b/kernel/module.c
index 7e78dfabca97..8b31c0b7c2a0 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3459,6 +3459,11 @@ static void flush_module_icache(const struct module *mod)
 			   (unsigned long)mod->core_layout.base + mod->core_layout.size);
 }
 
+bool __weak module_elf_check_arch(Elf_Ehdr *hdr)
+{
+	return elf_check_arch(hdr);
+}
+
 int __weak module_frob_arch_sections(Elf_Ehdr *hdr,
 				     Elf_Shdr *sechdrs,
 				     char *secstrings,

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
@ 2021-06-15  2:05       ` Nicholas Piggin
  0 siblings, 0 replies; 33+ messages in thread
From: Nicholas Piggin @ 2021-06-15  2:05 UTC (permalink / raw)
  To: Jessica Yu; +Cc: Michal Suchánek, linuxppc-dev, linux-kernel

Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:
> +++ Nicholas Piggin [11/06/21 19:39 +1000]:
>>The elf_check_arch() function is used to test usermode binaries, but
>>kernel modules may have more specific requirements. powerpc would like
>>to test for ABI version compatibility.
>>
>>Add an arch-overridable function elf_check_module_arch() that defaults
>>to elf_check_arch() and use it in elf_validity_check().
>>
>>Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>[np: split patch, added changelog]
>>Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>---
>> include/linux/moduleloader.h | 5 +++++
>> kernel/module.c              | 2 +-
>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>
>>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>>index 9e09d11ffe5b..fdc042a84562 100644
>>--- a/include/linux/moduleloader.h
>>+++ b/include/linux/moduleloader.h
>>@@ -13,6 +13,11 @@
>>  * must be implemented by each architecture.
>>  */
>>
>>+// Allow arch to optionally do additional checking of module ELF header
>>+#ifndef elf_check_module_arch
>>+#define elf_check_module_arch elf_check_arch
>>+#endif
> 
> Hi Nicholas,
> 
> Why not make elf_check_module_arch() consistent with the other
> arch-specific functions? Please see module_frob_arch_sections(),
> module_{init,exit}_section(), etc in moduleloader.h. That is, they are
> all __weak functions that are overridable by arches. We can maybe make
> elf_check_module_arch() a weak symbol, available for arches to
> override if they want to perform additional elf checks. Then we don't
> have to have this one-off #define.


Like this? I like it. Good idea.

Thanks,
Nick

diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 9e09d11ffe5b..7b4587a19189 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -13,6 +13,9 @@
  * must be implemented by each architecture.
  */
 
+/* arch may override to do additional checking of ELF header architecture */
+bool module_elf_check_arch(Elf_Ehdr *hdr);
+
 /* Adjust arch-specific sections.  Return 0 on success.  */
 int module_frob_arch_sections(Elf_Ehdr *hdr,
 			      Elf_Shdr *sechdrs,
diff --git a/kernel/module.c b/kernel/module.c
index 7e78dfabca97..8b31c0b7c2a0 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3459,6 +3459,11 @@ static void flush_module_icache(const struct module *mod)
 			   (unsigned long)mod->core_layout.base + mod->core_layout.size);
 }
 
+bool __weak module_elf_check_arch(Elf_Ehdr *hdr)
+{
+	return elf_check_arch(hdr);
+}
+
 int __weak module_frob_arch_sections(Elf_Ehdr *hdr,
 				     Elf_Shdr *sechdrs,
 				     char *secstrings,

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
  2021-06-15  2:05       ` Nicholas Piggin
@ 2021-06-15 12:17         ` Jessica Yu
  -1 siblings, 0 replies; 33+ messages in thread
From: Jessica Yu @ 2021-06-15 12:17 UTC (permalink / raw)
  To: Nicholas Piggin
  Cc: linux-kernel, linuxppc-dev, Michael Ellerman, Michal Suchánek

+++ Nicholas Piggin [15/06/21 12:05 +1000]:
>Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:
>> +++ Nicholas Piggin [11/06/21 19:39 +1000]:
>>>The elf_check_arch() function is used to test usermode binaries, but
>>>kernel modules may have more specific requirements. powerpc would like
>>>to test for ABI version compatibility.
>>>
>>>Add an arch-overridable function elf_check_module_arch() that defaults
>>>to elf_check_arch() and use it in elf_validity_check().
>>>
>>>Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>[np: split patch, added changelog]
>>>Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>---
>>> include/linux/moduleloader.h | 5 +++++
>>> kernel/module.c              | 2 +-
>>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>>
>>>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>>>index 9e09d11ffe5b..fdc042a84562 100644
>>>--- a/include/linux/moduleloader.h
>>>+++ b/include/linux/moduleloader.h
>>>@@ -13,6 +13,11 @@
>>>  * must be implemented by each architecture.
>>>  */
>>>
>>>+// Allow arch to optionally do additional checking of module ELF header
>>>+#ifndef elf_check_module_arch
>>>+#define elf_check_module_arch elf_check_arch
>>>+#endif
>>
>> Hi Nicholas,
>>
>> Why not make elf_check_module_arch() consistent with the other
>> arch-specific functions? Please see module_frob_arch_sections(),
>> module_{init,exit}_section(), etc in moduleloader.h. That is, they are
>> all __weak functions that are overridable by arches. We can maybe make
>> elf_check_module_arch() a weak symbol, available for arches to
>> override if they want to perform additional elf checks. Then we don't
>> have to have this one-off #define.
>
>
>Like this? I like it. Good idea.

Yeah! Also, maybe we can alternatively make elf_check_module_arch() a
separate check entirely so that the powerpc implementation doesn't
have to include that extra elf_check_arch() call. Something like this maybe?

diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 9e09d11ffe5b..2f9ebd593b4f 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -39,6 +39,9 @@ bool module_init_section(const char *name);
   */
  bool module_exit_section(const char *name);

+/* Arch may override to do additional checking of ELF header architecture */
+int elf_check_module_arch(Elf_Ehdr *hdr);
+
  /*
   * Apply the given relocation to the (simplified) ELF.  Return -error
   * or 0.
diff --git a/kernel/module.c b/kernel/module.c
index fdd6047728df..9963a979ed54 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2923,6 +2923,11 @@ static int validate_section_offset(struct load_info *info, Elf_Shdr *shdr)
         return 0;
  }

+int __weak elf_check_module_arch(Elf_Ehdr *hdr)
+{
+       return 1;
+}
+
  /*
   * Sanity checks against invalid binaries, wrong arch, weird elf version.
   *
@@ -2941,6 +2946,7 @@ static int elf_validity_check(struct load_info *info)
         if (memcmp(info->hdr->e_ident, ELFMAG, SELFMAG) != 0
             || info->hdr->e_type != ET_REL
             || !elf_check_arch(info->hdr)
+           || !elf_check_module_arch(info->hdr)
             || info->hdr->e_shentsize != sizeof(Elf_Shdr))
                 return -ENOEXEC;
  


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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
@ 2021-06-15 12:17         ` Jessica Yu
  0 siblings, 0 replies; 33+ messages in thread
From: Jessica Yu @ 2021-06-15 12:17 UTC (permalink / raw)
  To: Nicholas Piggin; +Cc: Michal Suchánek, linuxppc-dev, linux-kernel

+++ Nicholas Piggin [15/06/21 12:05 +1000]:
>Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:
>> +++ Nicholas Piggin [11/06/21 19:39 +1000]:
>>>The elf_check_arch() function is used to test usermode binaries, but
>>>kernel modules may have more specific requirements. powerpc would like
>>>to test for ABI version compatibility.
>>>
>>>Add an arch-overridable function elf_check_module_arch() that defaults
>>>to elf_check_arch() and use it in elf_validity_check().
>>>
>>>Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>[np: split patch, added changelog]
>>>Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>---
>>> include/linux/moduleloader.h | 5 +++++
>>> kernel/module.c              | 2 +-
>>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>>
>>>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>>>index 9e09d11ffe5b..fdc042a84562 100644
>>>--- a/include/linux/moduleloader.h
>>>+++ b/include/linux/moduleloader.h
>>>@@ -13,6 +13,11 @@
>>>  * must be implemented by each architecture.
>>>  */
>>>
>>>+// Allow arch to optionally do additional checking of module ELF header
>>>+#ifndef elf_check_module_arch
>>>+#define elf_check_module_arch elf_check_arch
>>>+#endif
>>
>> Hi Nicholas,
>>
>> Why not make elf_check_module_arch() consistent with the other
>> arch-specific functions? Please see module_frob_arch_sections(),
>> module_{init,exit}_section(), etc in moduleloader.h. That is, they are
>> all __weak functions that are overridable by arches. We can maybe make
>> elf_check_module_arch() a weak symbol, available for arches to
>> override if they want to perform additional elf checks. Then we don't
>> have to have this one-off #define.
>
>
>Like this? I like it. Good idea.

Yeah! Also, maybe we can alternatively make elf_check_module_arch() a
separate check entirely so that the powerpc implementation doesn't
have to include that extra elf_check_arch() call. Something like this maybe?

diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 9e09d11ffe5b..2f9ebd593b4f 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -39,6 +39,9 @@ bool module_init_section(const char *name);
   */
  bool module_exit_section(const char *name);

+/* Arch may override to do additional checking of ELF header architecture */
+int elf_check_module_arch(Elf_Ehdr *hdr);
+
  /*
   * Apply the given relocation to the (simplified) ELF.  Return -error
   * or 0.
diff --git a/kernel/module.c b/kernel/module.c
index fdd6047728df..9963a979ed54 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2923,6 +2923,11 @@ static int validate_section_offset(struct load_info *info, Elf_Shdr *shdr)
         return 0;
  }

+int __weak elf_check_module_arch(Elf_Ehdr *hdr)
+{
+       return 1;
+}
+
  /*
   * Sanity checks against invalid binaries, wrong arch, weird elf version.
   *
@@ -2941,6 +2946,7 @@ static int elf_validity_check(struct load_info *info)
         if (memcmp(info->hdr->e_ident, ELFMAG, SELFMAG) != 0
             || info->hdr->e_type != ET_REL
             || !elf_check_arch(info->hdr)
+           || !elf_check_module_arch(info->hdr)
             || info->hdr->e_shentsize != sizeof(Elf_Shdr))
                 return -ENOEXEC;
  


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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
  2021-06-15 12:17         ` Jessica Yu
@ 2021-06-15 12:50           ` Segher Boessenkool
  -1 siblings, 0 replies; 33+ messages in thread
From: Segher Boessenkool @ 2021-06-15 12:50 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Nicholas Piggin, Michal Suchánek, linuxppc-dev, linux-kernel

On Tue, Jun 15, 2021 at 02:17:40PM +0200, Jessica Yu wrote:
> +int __weak elf_check_module_arch(Elf_Ehdr *hdr)
> +{
> +       return 1;
> +}

But is this a good idea?  It isn't useful to be able to attempt to load
a module not compiled for your architecture, and it increases the attack
surface tremendously.  These checks are one of the few things that can
*not* be weak symbols, imo.


Segher

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
@ 2021-06-15 12:50           ` Segher Boessenkool
  0 siblings, 0 replies; 33+ messages in thread
From: Segher Boessenkool @ 2021-06-15 12:50 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Michal Suchánek, linuxppc-dev, linux-kernel, Nicholas Piggin

On Tue, Jun 15, 2021 at 02:17:40PM +0200, Jessica Yu wrote:
> +int __weak elf_check_module_arch(Elf_Ehdr *hdr)
> +{
> +       return 1;
> +}

But is this a good idea?  It isn't useful to be able to attempt to load
a module not compiled for your architecture, and it increases the attack
surface tremendously.  These checks are one of the few things that can
*not* be weak symbols, imo.


Segher

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
  2021-06-15 12:50           ` Segher Boessenkool
@ 2021-06-15 13:41             ` Jessica Yu
  -1 siblings, 0 replies; 33+ messages in thread
From: Jessica Yu @ 2021-06-15 13:41 UTC (permalink / raw)
  To: Segher Boessenkool
  Cc: Nicholas Piggin, Michal Suchánek, linuxppc-dev, linux-kernel

+++ Segher Boessenkool [15/06/21 07:50 -0500]:
>On Tue, Jun 15, 2021 at 02:17:40PM +0200, Jessica Yu wrote:
>> +int __weak elf_check_module_arch(Elf_Ehdr *hdr)
>> +{
>> +       return 1;
>> +}
>
>But is this a good idea?  It isn't useful to be able to attempt to load
>a module not compiled for your architecture, and it increases the attack
>surface tremendously.  These checks are one of the few things that can
>*not* be weak symbols, imo.

Hm, could you please elaborate a bit more? This patchset is adding
extra Elf header checks specifically for powerpc, and the module
loader usually provides arch-specific hooks via weak symbols. We are
just providing an new hook here, which should act as a no-op if it
isn't used.

So if an architecture wants to provide extra header checks, it can do
so by overriding the new weak symbol. Otherwise, the weak function acts as
a noop. We also already have the existing elf_check_arch() check for each
arch and that is *not* a weak symbol.

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
@ 2021-06-15 13:41             ` Jessica Yu
  0 siblings, 0 replies; 33+ messages in thread
From: Jessica Yu @ 2021-06-15 13:41 UTC (permalink / raw)
  To: Segher Boessenkool
  Cc: Michal Suchánek, linuxppc-dev, linux-kernel, Nicholas Piggin

+++ Segher Boessenkool [15/06/21 07:50 -0500]:
>On Tue, Jun 15, 2021 at 02:17:40PM +0200, Jessica Yu wrote:
>> +int __weak elf_check_module_arch(Elf_Ehdr *hdr)
>> +{
>> +       return 1;
>> +}
>
>But is this a good idea?  It isn't useful to be able to attempt to load
>a module not compiled for your architecture, and it increases the attack
>surface tremendously.  These checks are one of the few things that can
>*not* be weak symbols, imo.

Hm, could you please elaborate a bit more? This patchset is adding
extra Elf header checks specifically for powerpc, and the module
loader usually provides arch-specific hooks via weak symbols. We are
just providing an new hook here, which should act as a no-op if it
isn't used.

So if an architecture wants to provide extra header checks, it can do
so by overriding the new weak symbol. Otherwise, the weak function acts as
a noop. We also already have the existing elf_check_arch() check for each
arch and that is *not* a weak symbol.

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
  2021-06-15 13:41             ` Jessica Yu
@ 2021-06-15 14:30               ` Segher Boessenkool
  -1 siblings, 0 replies; 33+ messages in thread
From: Segher Boessenkool @ 2021-06-15 14:30 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Nicholas Piggin, Michal Suchánek, linuxppc-dev, linux-kernel

On Tue, Jun 15, 2021 at 03:41:00PM +0200, Jessica Yu wrote:
> +++ Segher Boessenkool [15/06/21 07:50 -0500]:
> >On Tue, Jun 15, 2021 at 02:17:40PM +0200, Jessica Yu wrote:
> >>+int __weak elf_check_module_arch(Elf_Ehdr *hdr)
> >>+{
> >>+       return 1;
> >>+}
> >
> >But is this a good idea?  It isn't useful to be able to attempt to load
> >a module not compiled for your architecture, and it increases the attack
> >surface tremendously.  These checks are one of the few things that can
> >*not* be weak symbols, imo.
> 
> Hm, could you please elaborate a bit more? This patchset is adding
> extra Elf header checks specifically for powerpc, and the module
> loader usually provides arch-specific hooks via weak symbols. We are
> just providing an new hook here, which should act as a no-op if it
> isn't used.
> 
> So if an architecture wants to provide extra header checks, it can do
> so by overriding the new weak symbol. Otherwise, the weak function acts as
> a noop. We also already have the existing elf_check_arch() check for each
> arch and that is *not* a weak symbol.

The way I read your patch the default elf_check_module_arch does not
call elf_check_arch?  Is that clearly called elsewhere and I'm just
dumb again?  Sorry for the distraction in that case :-/


Segher

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
@ 2021-06-15 14:30               ` Segher Boessenkool
  0 siblings, 0 replies; 33+ messages in thread
From: Segher Boessenkool @ 2021-06-15 14:30 UTC (permalink / raw)
  To: Jessica Yu
  Cc: Michal Suchánek, linuxppc-dev, linux-kernel, Nicholas Piggin

On Tue, Jun 15, 2021 at 03:41:00PM +0200, Jessica Yu wrote:
> +++ Segher Boessenkool [15/06/21 07:50 -0500]:
> >On Tue, Jun 15, 2021 at 02:17:40PM +0200, Jessica Yu wrote:
> >>+int __weak elf_check_module_arch(Elf_Ehdr *hdr)
> >>+{
> >>+       return 1;
> >>+}
> >
> >But is this a good idea?  It isn't useful to be able to attempt to load
> >a module not compiled for your architecture, and it increases the attack
> >surface tremendously.  These checks are one of the few things that can
> >*not* be weak symbols, imo.
> 
> Hm, could you please elaborate a bit more? This patchset is adding
> extra Elf header checks specifically for powerpc, and the module
> loader usually provides arch-specific hooks via weak symbols. We are
> just providing an new hook here, which should act as a no-op if it
> isn't used.
> 
> So if an architecture wants to provide extra header checks, it can do
> so by overriding the new weak symbol. Otherwise, the weak function acts as
> a noop. We also already have the existing elf_check_arch() check for each
> arch and that is *not* a weak symbol.

The way I read your patch the default elf_check_module_arch does not
call elf_check_arch?  Is that clearly called elsewhere and I'm just
dumb again?  Sorry for the distraction in that case :-/


Segher

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
  2021-06-15 12:17         ` Jessica Yu
@ 2021-06-16  1:18           ` Nicholas Piggin
  -1 siblings, 0 replies; 33+ messages in thread
From: Nicholas Piggin @ 2021-06-16  1:18 UTC (permalink / raw)
  To: Jessica Yu
  Cc: linux-kernel, linuxppc-dev, Michael Ellerman, Michal Suchánek

Excerpts from Jessica Yu's message of June 15, 2021 10:17 pm:
> +++ Nicholas Piggin [15/06/21 12:05 +1000]:
>>Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:
>>> +++ Nicholas Piggin [11/06/21 19:39 +1000]:
>>>>The elf_check_arch() function is used to test usermode binaries, but
>>>>kernel modules may have more specific requirements. powerpc would like
>>>>to test for ABI version compatibility.
>>>>
>>>>Add an arch-overridable function elf_check_module_arch() that defaults
>>>>to elf_check_arch() and use it in elf_validity_check().
>>>>
>>>>Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>>[np: split patch, added changelog]
>>>>Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>---
>>>> include/linux/moduleloader.h | 5 +++++
>>>> kernel/module.c              | 2 +-
>>>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>>>>index 9e09d11ffe5b..fdc042a84562 100644
>>>>--- a/include/linux/moduleloader.h
>>>>+++ b/include/linux/moduleloader.h
>>>>@@ -13,6 +13,11 @@
>>>>  * must be implemented by each architecture.
>>>>  */
>>>>
>>>>+// Allow arch to optionally do additional checking of module ELF header
>>>>+#ifndef elf_check_module_arch
>>>>+#define elf_check_module_arch elf_check_arch
>>>>+#endif
>>>
>>> Hi Nicholas,
>>>
>>> Why not make elf_check_module_arch() consistent with the other
>>> arch-specific functions? Please see module_frob_arch_sections(),
>>> module_{init,exit}_section(), etc in moduleloader.h. That is, they are
>>> all __weak functions that are overridable by arches. We can maybe make
>>> elf_check_module_arch() a weak symbol, available for arches to
>>> override if they want to perform additional elf checks. Then we don't
>>> have to have this one-off #define.
>>
>>
>>Like this? I like it. Good idea.
> 
> Yeah! Also, maybe we can alternatively make elf_check_module_arch() a
> separate check entirely so that the powerpc implementation doesn't
> have to include that extra elf_check_arch() call. Something like this maybe?

Yeah we can do that. Would you be okay if it goes via powerpc tree? If 
yes, then we should get your Ack (or SOB because it seems to be entirely 
your patch now :D)

Thanks,
Nick

> 
> diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
> index 9e09d11ffe5b..2f9ebd593b4f 100644
> --- a/include/linux/moduleloader.h
> +++ b/include/linux/moduleloader.h
> @@ -39,6 +39,9 @@ bool module_init_section(const char *name);
>    */
>   bool module_exit_section(const char *name);
> 
> +/* Arch may override to do additional checking of ELF header architecture */
> +int elf_check_module_arch(Elf_Ehdr *hdr);
> +
>   /*
>    * Apply the given relocation to the (simplified) ELF.  Return -error
>    * or 0.
> diff --git a/kernel/module.c b/kernel/module.c
> index fdd6047728df..9963a979ed54 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2923,6 +2923,11 @@ static int validate_section_offset(struct load_info *info, Elf_Shdr *shdr)
>          return 0;
>   }
> 
> +int __weak elf_check_module_arch(Elf_Ehdr *hdr)
> +{
> +       return 1;
> +}
> +
>   /*
>    * Sanity checks against invalid binaries, wrong arch, weird elf version.
>    *
> @@ -2941,6 +2946,7 @@ static int elf_validity_check(struct load_info *info)
>          if (memcmp(info->hdr->e_ident, ELFMAG, SELFMAG) != 0
>              || info->hdr->e_type != ET_REL
>              || !elf_check_arch(info->hdr)
> +           || !elf_check_module_arch(info->hdr)
>              || info->hdr->e_shentsize != sizeof(Elf_Shdr))
>                  return -ENOEXEC;
>   
> 
> 

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
@ 2021-06-16  1:18           ` Nicholas Piggin
  0 siblings, 0 replies; 33+ messages in thread
From: Nicholas Piggin @ 2021-06-16  1:18 UTC (permalink / raw)
  To: Jessica Yu; +Cc: Michal Suchánek, linuxppc-dev, linux-kernel

Excerpts from Jessica Yu's message of June 15, 2021 10:17 pm:
> +++ Nicholas Piggin [15/06/21 12:05 +1000]:
>>Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:
>>> +++ Nicholas Piggin [11/06/21 19:39 +1000]:
>>>>The elf_check_arch() function is used to test usermode binaries, but
>>>>kernel modules may have more specific requirements. powerpc would like
>>>>to test for ABI version compatibility.
>>>>
>>>>Add an arch-overridable function elf_check_module_arch() that defaults
>>>>to elf_check_arch() and use it in elf_validity_check().
>>>>
>>>>Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>>[np: split patch, added changelog]
>>>>Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>---
>>>> include/linux/moduleloader.h | 5 +++++
>>>> kernel/module.c              | 2 +-
>>>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>>>>index 9e09d11ffe5b..fdc042a84562 100644
>>>>--- a/include/linux/moduleloader.h
>>>>+++ b/include/linux/moduleloader.h
>>>>@@ -13,6 +13,11 @@
>>>>  * must be implemented by each architecture.
>>>>  */
>>>>
>>>>+// Allow arch to optionally do additional checking of module ELF header
>>>>+#ifndef elf_check_module_arch
>>>>+#define elf_check_module_arch elf_check_arch
>>>>+#endif
>>>
>>> Hi Nicholas,
>>>
>>> Why not make elf_check_module_arch() consistent with the other
>>> arch-specific functions? Please see module_frob_arch_sections(),
>>> module_{init,exit}_section(), etc in moduleloader.h. That is, they are
>>> all __weak functions that are overridable by arches. We can maybe make
>>> elf_check_module_arch() a weak symbol, available for arches to
>>> override if they want to perform additional elf checks. Then we don't
>>> have to have this one-off #define.
>>
>>
>>Like this? I like it. Good idea.
> 
> Yeah! Also, maybe we can alternatively make elf_check_module_arch() a
> separate check entirely so that the powerpc implementation doesn't
> have to include that extra elf_check_arch() call. Something like this maybe?

Yeah we can do that. Would you be okay if it goes via powerpc tree? If 
yes, then we should get your Ack (or SOB because it seems to be entirely 
your patch now :D)

Thanks,
Nick

> 
> diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
> index 9e09d11ffe5b..2f9ebd593b4f 100644
> --- a/include/linux/moduleloader.h
> +++ b/include/linux/moduleloader.h
> @@ -39,6 +39,9 @@ bool module_init_section(const char *name);
>    */
>   bool module_exit_section(const char *name);
> 
> +/* Arch may override to do additional checking of ELF header architecture */
> +int elf_check_module_arch(Elf_Ehdr *hdr);
> +
>   /*
>    * Apply the given relocation to the (simplified) ELF.  Return -error
>    * or 0.
> diff --git a/kernel/module.c b/kernel/module.c
> index fdd6047728df..9963a979ed54 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2923,6 +2923,11 @@ static int validate_section_offset(struct load_info *info, Elf_Shdr *shdr)
>          return 0;
>   }
> 
> +int __weak elf_check_module_arch(Elf_Ehdr *hdr)
> +{
> +       return 1;
> +}
> +
>   /*
>    * Sanity checks against invalid binaries, wrong arch, weird elf version.
>    *
> @@ -2941,6 +2946,7 @@ static int elf_validity_check(struct load_info *info)
>          if (memcmp(info->hdr->e_ident, ELFMAG, SELFMAG) != 0
>              || info->hdr->e_type != ET_REL
>              || !elf_check_arch(info->hdr)
> +           || !elf_check_module_arch(info->hdr)
>              || info->hdr->e_shentsize != sizeof(Elf_Shdr))
>                  return -ENOEXEC;
>   
> 
> 

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
  2021-06-15 12:17         ` Jessica Yu
@ 2021-06-16  2:37           ` Michael Ellerman
  -1 siblings, 0 replies; 33+ messages in thread
From: Michael Ellerman @ 2021-06-16  2:37 UTC (permalink / raw)
  To: Jessica Yu, Nicholas Piggin
  Cc: linux-kernel, linuxppc-dev, Michal Suchánek

Jessica Yu <jeyu@kernel.org> writes:
> +++ Nicholas Piggin [15/06/21 12:05 +1000]:
>>Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:
>>> +++ Nicholas Piggin [11/06/21 19:39 +1000]:
>>>>The elf_check_arch() function is used to test usermode binaries, but
>>>>kernel modules may have more specific requirements. powerpc would like
>>>>to test for ABI version compatibility.
>>>>
>>>>Add an arch-overridable function elf_check_module_arch() that defaults
>>>>to elf_check_arch() and use it in elf_validity_check().
>>>>
>>>>Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>>[np: split patch, added changelog]
>>>>Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>---
>>>> include/linux/moduleloader.h | 5 +++++
>>>> kernel/module.c              | 2 +-
>>>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>>>>index 9e09d11ffe5b..fdc042a84562 100644
>>>>--- a/include/linux/moduleloader.h
>>>>+++ b/include/linux/moduleloader.h
>>>>@@ -13,6 +13,11 @@
>>>>  * must be implemented by each architecture.
>>>>  */
>>>>
>>>>+// Allow arch to optionally do additional checking of module ELF header
>>>>+#ifndef elf_check_module_arch
>>>>+#define elf_check_module_arch elf_check_arch
>>>>+#endif
>>>
>>> Hi Nicholas,
>>>
>>> Why not make elf_check_module_arch() consistent with the other
>>> arch-specific functions? Please see module_frob_arch_sections(),
>>> module_{init,exit}_section(), etc in moduleloader.h. That is, they are
>>> all __weak functions that are overridable by arches. We can maybe make
>>> elf_check_module_arch() a weak symbol, available for arches to
>>> override if they want to perform additional elf checks. Then we don't
>>> have to have this one-off #define.

>>Like this? I like it. Good idea.
>
> Yeah! Also, maybe we can alternatively make elf_check_module_arch() a
> separate check entirely so that the powerpc implementation doesn't
> have to include that extra elf_check_arch() call. Something like this maybe?

My thinking for making elf_check_module_arch() the only hook was that
conceivably you might not want/need to call elf_check_arch() from
elf_check_module_arch().

So having a single module specific hook allows arch code to decide
how to implement the check, which may or may not involve calling
elf_check_arch(), but that becomes an arch implementation detail.

It's also one arch hook instead of two (although elf_check_arch()
already exists).

But I don't feel that strongly either way, whatever you prefer.

cheers

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
@ 2021-06-16  2:37           ` Michael Ellerman
  0 siblings, 0 replies; 33+ messages in thread
From: Michael Ellerman @ 2021-06-16  2:37 UTC (permalink / raw)
  To: Jessica Yu, Nicholas Piggin
  Cc: Michal Suchánek, linuxppc-dev, linux-kernel

Jessica Yu <jeyu@kernel.org> writes:
> +++ Nicholas Piggin [15/06/21 12:05 +1000]:
>>Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:
>>> +++ Nicholas Piggin [11/06/21 19:39 +1000]:
>>>>The elf_check_arch() function is used to test usermode binaries, but
>>>>kernel modules may have more specific requirements. powerpc would like
>>>>to test for ABI version compatibility.
>>>>
>>>>Add an arch-overridable function elf_check_module_arch() that defaults
>>>>to elf_check_arch() and use it in elf_validity_check().
>>>>
>>>>Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>>[np: split patch, added changelog]
>>>>Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>---
>>>> include/linux/moduleloader.h | 5 +++++
>>>> kernel/module.c              | 2 +-
>>>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>>>>index 9e09d11ffe5b..fdc042a84562 100644
>>>>--- a/include/linux/moduleloader.h
>>>>+++ b/include/linux/moduleloader.h
>>>>@@ -13,6 +13,11 @@
>>>>  * must be implemented by each architecture.
>>>>  */
>>>>
>>>>+// Allow arch to optionally do additional checking of module ELF header
>>>>+#ifndef elf_check_module_arch
>>>>+#define elf_check_module_arch elf_check_arch
>>>>+#endif
>>>
>>> Hi Nicholas,
>>>
>>> Why not make elf_check_module_arch() consistent with the other
>>> arch-specific functions? Please see module_frob_arch_sections(),
>>> module_{init,exit}_section(), etc in moduleloader.h. That is, they are
>>> all __weak functions that are overridable by arches. We can maybe make
>>> elf_check_module_arch() a weak symbol, available for arches to
>>> override if they want to perform additional elf checks. Then we don't
>>> have to have this one-off #define.

>>Like this? I like it. Good idea.
>
> Yeah! Also, maybe we can alternatively make elf_check_module_arch() a
> separate check entirely so that the powerpc implementation doesn't
> have to include that extra elf_check_arch() call. Something like this maybe?

My thinking for making elf_check_module_arch() the only hook was that
conceivably you might not want/need to call elf_check_arch() from
elf_check_module_arch().

So having a single module specific hook allows arch code to decide
how to implement the check, which may or may not involve calling
elf_check_arch(), but that becomes an arch implementation detail.

It's also one arch hook instead of two (although elf_check_arch()
already exists).

But I don't feel that strongly either way, whatever you prefer.

cheers

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
  2021-06-15 14:30               ` Segher Boessenkool
@ 2021-06-16  2:39                 ` Michael Ellerman
  -1 siblings, 0 replies; 33+ messages in thread
From: Michael Ellerman @ 2021-06-16  2:39 UTC (permalink / raw)
  To: Segher Boessenkool, Jessica Yu
  Cc: Nicholas Piggin, Michal Suchánek, linuxppc-dev, linux-kernel

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Tue, Jun 15, 2021 at 03:41:00PM +0200, Jessica Yu wrote:
>> +++ Segher Boessenkool [15/06/21 07:50 -0500]:
>> >On Tue, Jun 15, 2021 at 02:17:40PM +0200, Jessica Yu wrote:
>> >>+int __weak elf_check_module_arch(Elf_Ehdr *hdr)
>> >>+{
>> >>+       return 1;
>> >>+}
>> >
>> >But is this a good idea?  It isn't useful to be able to attempt to load
>> >a module not compiled for your architecture, and it increases the attack
>> >surface tremendously.  These checks are one of the few things that can
>> >*not* be weak symbols, imo.
>> 
>> Hm, could you please elaborate a bit more? This patchset is adding
>> extra Elf header checks specifically for powerpc, and the module
>> loader usually provides arch-specific hooks via weak symbols. We are
>> just providing an new hook here, which should act as a no-op if it
>> isn't used.
>> 
>> So if an architecture wants to provide extra header checks, it can do
>> so by overriding the new weak symbol. Otherwise, the weak function acts as
>> a noop. We also already have the existing elf_check_arch() check for each
>> arch and that is *not* a weak symbol.
>
> The way I read your patch the default elf_check_module_arch does not
> call elf_check_arch?  Is that clearly called elsewhere and I'm just
> dumb again?  Sorry for the distraction in that case :-/

Yeah elf_check_arch() is already called from elf_validity_check(), and
that call would remain.

cheers

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
@ 2021-06-16  2:39                 ` Michael Ellerman
  0 siblings, 0 replies; 33+ messages in thread
From: Michael Ellerman @ 2021-06-16  2:39 UTC (permalink / raw)
  To: Segher Boessenkool, Jessica Yu
  Cc: Michal Suchánek, linuxppc-dev, linux-kernel, Nicholas Piggin

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Tue, Jun 15, 2021 at 03:41:00PM +0200, Jessica Yu wrote:
>> +++ Segher Boessenkool [15/06/21 07:50 -0500]:
>> >On Tue, Jun 15, 2021 at 02:17:40PM +0200, Jessica Yu wrote:
>> >>+int __weak elf_check_module_arch(Elf_Ehdr *hdr)
>> >>+{
>> >>+       return 1;
>> >>+}
>> >
>> >But is this a good idea?  It isn't useful to be able to attempt to load
>> >a module not compiled for your architecture, and it increases the attack
>> >surface tremendously.  These checks are one of the few things that can
>> >*not* be weak symbols, imo.
>> 
>> Hm, could you please elaborate a bit more? This patchset is adding
>> extra Elf header checks specifically for powerpc, and the module
>> loader usually provides arch-specific hooks via weak symbols. We are
>> just providing an new hook here, which should act as a no-op if it
>> isn't used.
>> 
>> So if an architecture wants to provide extra header checks, it can do
>> so by overriding the new weak symbol. Otherwise, the weak function acts as
>> a noop. We also already have the existing elf_check_arch() check for each
>> arch and that is *not* a weak symbol.
>
> The way I read your patch the default elf_check_module_arch does not
> call elf_check_arch?  Is that clearly called elsewhere and I'm just
> dumb again?  Sorry for the distraction in that case :-/

Yeah elf_check_arch() is already called from elf_validity_check(), and
that call would remain.

cheers

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
  2021-06-16  1:18           ` Nicholas Piggin
@ 2021-06-16 12:54             ` Jessica Yu
  -1 siblings, 0 replies; 33+ messages in thread
From: Jessica Yu @ 2021-06-16 12:54 UTC (permalink / raw)
  To: Nicholas Piggin
  Cc: linux-kernel, linuxppc-dev, Michael Ellerman, Michal Suchánek

+++ Nicholas Piggin [16/06/21 11:18 +1000]:
>Excerpts from Jessica Yu's message of June 15, 2021 10:17 pm:
>> +++ Nicholas Piggin [15/06/21 12:05 +1000]:
>>>Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:
>>>> +++ Nicholas Piggin [11/06/21 19:39 +1000]:
>>>>>The elf_check_arch() function is used to test usermode binaries, but
>>>>>kernel modules may have more specific requirements. powerpc would like
>>>>>to test for ABI version compatibility.
>>>>>
>>>>>Add an arch-overridable function elf_check_module_arch() that defaults
>>>>>to elf_check_arch() and use it in elf_validity_check().
>>>>>
>>>>>Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>>>[np: split patch, added changelog]
>>>>>Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>>---
>>>>> include/linux/moduleloader.h | 5 +++++
>>>>> kernel/module.c              | 2 +-
>>>>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>>>>>index 9e09d11ffe5b..fdc042a84562 100644
>>>>>--- a/include/linux/moduleloader.h
>>>>>+++ b/include/linux/moduleloader.h
>>>>>@@ -13,6 +13,11 @@
>>>>>  * must be implemented by each architecture.
>>>>>  */
>>>>>
>>>>>+// Allow arch to optionally do additional checking of module ELF header
>>>>>+#ifndef elf_check_module_arch
>>>>>+#define elf_check_module_arch elf_check_arch
>>>>>+#endif
>>>>
>>>> Hi Nicholas,
>>>>
>>>> Why not make elf_check_module_arch() consistent with the other
>>>> arch-specific functions? Please see module_frob_arch_sections(),
>>>> module_{init,exit}_section(), etc in moduleloader.h. That is, they are
>>>> all __weak functions that are overridable by arches. We can maybe make
>>>> elf_check_module_arch() a weak symbol, available for arches to
>>>> override if they want to perform additional elf checks. Then we don't
>>>> have to have this one-off #define.
>>>
>>>
>>>Like this? I like it. Good idea.
>>
>> Yeah! Also, maybe we can alternatively make elf_check_module_arch() a
>> separate check entirely so that the powerpc implementation doesn't
>> have to include that extra elf_check_arch() call. Something like this maybe?
>
>Yeah we can do that. Would you be okay if it goes via powerpc tree? If
>yes, then we should get your Ack (or SOB because it seems to be entirely
>your patch now :D)

This can go through the powerpc tree. Will you do another respin
of this patch? And yes, feel free to take my SOB for this one -

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

Thanks!

Jessica

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
@ 2021-06-16 12:54             ` Jessica Yu
  0 siblings, 0 replies; 33+ messages in thread
From: Jessica Yu @ 2021-06-16 12:54 UTC (permalink / raw)
  To: Nicholas Piggin; +Cc: Michal Suchánek, linuxppc-dev, linux-kernel

+++ Nicholas Piggin [16/06/21 11:18 +1000]:
>Excerpts from Jessica Yu's message of June 15, 2021 10:17 pm:
>> +++ Nicholas Piggin [15/06/21 12:05 +1000]:
>>>Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:
>>>> +++ Nicholas Piggin [11/06/21 19:39 +1000]:
>>>>>The elf_check_arch() function is used to test usermode binaries, but
>>>>>kernel modules may have more specific requirements. powerpc would like
>>>>>to test for ABI version compatibility.
>>>>>
>>>>>Add an arch-overridable function elf_check_module_arch() that defaults
>>>>>to elf_check_arch() and use it in elf_validity_check().
>>>>>
>>>>>Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>>>[np: split patch, added changelog]
>>>>>Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>>---
>>>>> include/linux/moduleloader.h | 5 +++++
>>>>> kernel/module.c              | 2 +-
>>>>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>>>>>index 9e09d11ffe5b..fdc042a84562 100644
>>>>>--- a/include/linux/moduleloader.h
>>>>>+++ b/include/linux/moduleloader.h
>>>>>@@ -13,6 +13,11 @@
>>>>>  * must be implemented by each architecture.
>>>>>  */
>>>>>
>>>>>+// Allow arch to optionally do additional checking of module ELF header
>>>>>+#ifndef elf_check_module_arch
>>>>>+#define elf_check_module_arch elf_check_arch
>>>>>+#endif
>>>>
>>>> Hi Nicholas,
>>>>
>>>> Why not make elf_check_module_arch() consistent with the other
>>>> arch-specific functions? Please see module_frob_arch_sections(),
>>>> module_{init,exit}_section(), etc in moduleloader.h. That is, they are
>>>> all __weak functions that are overridable by arches. We can maybe make
>>>> elf_check_module_arch() a weak symbol, available for arches to
>>>> override if they want to perform additional elf checks. Then we don't
>>>> have to have this one-off #define.
>>>
>>>
>>>Like this? I like it. Good idea.
>>
>> Yeah! Also, maybe we can alternatively make elf_check_module_arch() a
>> separate check entirely so that the powerpc implementation doesn't
>> have to include that extra elf_check_arch() call. Something like this maybe?
>
>Yeah we can do that. Would you be okay if it goes via powerpc tree? If
>yes, then we should get your Ack (or SOB because it seems to be entirely
>your patch now :D)

This can go through the powerpc tree. Will you do another respin
of this patch? And yes, feel free to take my SOB for this one -

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

Thanks!

Jessica

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
  2021-06-16  2:37           ` Michael Ellerman
@ 2021-06-16 13:49             ` Jessica Yu
  -1 siblings, 0 replies; 33+ messages in thread
From: Jessica Yu @ 2021-06-16 13:49 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Nicholas Piggin, linux-kernel, linuxppc-dev, Michal Suchánek

+++ Michael Ellerman [16/06/21 12:37 +1000]:
>Jessica Yu <jeyu@kernel.org> writes:
>> +++ Nicholas Piggin [15/06/21 12:05 +1000]:
>>>Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:
>>>> +++ Nicholas Piggin [11/06/21 19:39 +1000]:
>>>>>The elf_check_arch() function is used to test usermode binaries, but
>>>>>kernel modules may have more specific requirements. powerpc would like
>>>>>to test for ABI version compatibility.
>>>>>
>>>>>Add an arch-overridable function elf_check_module_arch() that defaults
>>>>>to elf_check_arch() and use it in elf_validity_check().
>>>>>
>>>>>Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>>>[np: split patch, added changelog]
>>>>>Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>>---
>>>>> include/linux/moduleloader.h | 5 +++++
>>>>> kernel/module.c              | 2 +-
>>>>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>>>>>index 9e09d11ffe5b..fdc042a84562 100644
>>>>>--- a/include/linux/moduleloader.h
>>>>>+++ b/include/linux/moduleloader.h
>>>>>@@ -13,6 +13,11 @@
>>>>>  * must be implemented by each architecture.
>>>>>  */
>>>>>
>>>>>+// Allow arch to optionally do additional checking of module ELF header
>>>>>+#ifndef elf_check_module_arch
>>>>>+#define elf_check_module_arch elf_check_arch
>>>>>+#endif
>>>>
>>>> Hi Nicholas,
>>>>
>>>> Why not make elf_check_module_arch() consistent with the other
>>>> arch-specific functions? Please see module_frob_arch_sections(),
>>>> module_{init,exit}_section(), etc in moduleloader.h. That is, they are
>>>> all __weak functions that are overridable by arches. We can maybe make
>>>> elf_check_module_arch() a weak symbol, available for arches to
>>>> override if they want to perform additional elf checks. Then we don't
>>>> have to have this one-off #define.
>
>>>Like this? I like it. Good idea.
>>
>> Yeah! Also, maybe we can alternatively make elf_check_module_arch() a
>> separate check entirely so that the powerpc implementation doesn't
>> have to include that extra elf_check_arch() call. Something like this maybe?
>
>My thinking for making elf_check_module_arch() the only hook was that
>conceivably you might not want/need to call elf_check_arch() from
>elf_check_module_arch().
>
>So having a single module specific hook allows arch code to decide
>how to implement the check, which may or may not involve calling
>elf_check_arch(), but that becomes an arch implementation detail.

Thanks for the feedback! Yeah, that's fair too. Well, I ended up doing
it this way mostly to create less churn/change of behavior, since in
its current state elf_check_arch() is already being called for each
arch. Additionally I wanted to save the powerpc implementation of
elf_check_module_arch() an extra elf_check_arch() call. In any case I
have a slight preference for having a second hook to allow arches add
any extra checks in addition to elf_check_arch(). Thanks!

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
@ 2021-06-16 13:49             ` Jessica Yu
  0 siblings, 0 replies; 33+ messages in thread
From: Jessica Yu @ 2021-06-16 13:49 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Michal Suchánek, linuxppc-dev, linux-kernel, Nicholas Piggin

+++ Michael Ellerman [16/06/21 12:37 +1000]:
>Jessica Yu <jeyu@kernel.org> writes:
>> +++ Nicholas Piggin [15/06/21 12:05 +1000]:
>>>Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:
>>>> +++ Nicholas Piggin [11/06/21 19:39 +1000]:
>>>>>The elf_check_arch() function is used to test usermode binaries, but
>>>>>kernel modules may have more specific requirements. powerpc would like
>>>>>to test for ABI version compatibility.
>>>>>
>>>>>Add an arch-overridable function elf_check_module_arch() that defaults
>>>>>to elf_check_arch() and use it in elf_validity_check().
>>>>>
>>>>>Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>>>[np: split patch, added changelog]
>>>>>Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>>---
>>>>> include/linux/moduleloader.h | 5 +++++
>>>>> kernel/module.c              | 2 +-
>>>>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>>>>>index 9e09d11ffe5b..fdc042a84562 100644
>>>>>--- a/include/linux/moduleloader.h
>>>>>+++ b/include/linux/moduleloader.h
>>>>>@@ -13,6 +13,11 @@
>>>>>  * must be implemented by each architecture.
>>>>>  */
>>>>>
>>>>>+// Allow arch to optionally do additional checking of module ELF header
>>>>>+#ifndef elf_check_module_arch
>>>>>+#define elf_check_module_arch elf_check_arch
>>>>>+#endif
>>>>
>>>> Hi Nicholas,
>>>>
>>>> Why not make elf_check_module_arch() consistent with the other
>>>> arch-specific functions? Please see module_frob_arch_sections(),
>>>> module_{init,exit}_section(), etc in moduleloader.h. That is, they are
>>>> all __weak functions that are overridable by arches. We can maybe make
>>>> elf_check_module_arch() a weak symbol, available for arches to
>>>> override if they want to perform additional elf checks. Then we don't
>>>> have to have this one-off #define.
>
>>>Like this? I like it. Good idea.
>>
>> Yeah! Also, maybe we can alternatively make elf_check_module_arch() a
>> separate check entirely so that the powerpc implementation doesn't
>> have to include that extra elf_check_arch() call. Something like this maybe?
>
>My thinking for making elf_check_module_arch() the only hook was that
>conceivably you might not want/need to call elf_check_arch() from
>elf_check_module_arch().
>
>So having a single module specific hook allows arch code to decide
>how to implement the check, which may or may not involve calling
>elf_check_arch(), but that becomes an arch implementation detail.

Thanks for the feedback! Yeah, that's fair too. Well, I ended up doing
it this way mostly to create less churn/change of behavior, since in
its current state elf_check_arch() is already being called for each
arch. Additionally I wanted to save the powerpc implementation of
elf_check_module_arch() an extra elf_check_arch() call. In any case I
have a slight preference for having a second hook to allow arches add
any extra checks in addition to elf_check_arch(). Thanks!

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
  2021-06-16 12:54             ` Jessica Yu
@ 2021-06-17  5:21               ` Nicholas Piggin
  -1 siblings, 0 replies; 33+ messages in thread
From: Nicholas Piggin @ 2021-06-17  5:21 UTC (permalink / raw)
  To: Jessica Yu
  Cc: linux-kernel, linuxppc-dev, Michael Ellerman, Michal Suchánek

Excerpts from Jessica Yu's message of June 16, 2021 10:54 pm:
> +++ Nicholas Piggin [16/06/21 11:18 +1000]:
>>Excerpts from Jessica Yu's message of June 15, 2021 10:17 pm:
>>> +++ Nicholas Piggin [15/06/21 12:05 +1000]:
>>>>Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:
>>>>> +++ Nicholas Piggin [11/06/21 19:39 +1000]:
>>>>>>The elf_check_arch() function is used to test usermode binaries, but
>>>>>>kernel modules may have more specific requirements. powerpc would like
>>>>>>to test for ABI version compatibility.
>>>>>>
>>>>>>Add an arch-overridable function elf_check_module_arch() that defaults
>>>>>>to elf_check_arch() and use it in elf_validity_check().
>>>>>>
>>>>>>Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>>>>[np: split patch, added changelog]
>>>>>>Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>>>---
>>>>>> include/linux/moduleloader.h | 5 +++++
>>>>>> kernel/module.c              | 2 +-
>>>>>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>>>>>
>>>>>>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>>>>>>index 9e09d11ffe5b..fdc042a84562 100644
>>>>>>--- a/include/linux/moduleloader.h
>>>>>>+++ b/include/linux/moduleloader.h
>>>>>>@@ -13,6 +13,11 @@
>>>>>>  * must be implemented by each architecture.
>>>>>>  */
>>>>>>
>>>>>>+// Allow arch to optionally do additional checking of module ELF header
>>>>>>+#ifndef elf_check_module_arch
>>>>>>+#define elf_check_module_arch elf_check_arch
>>>>>>+#endif
>>>>>
>>>>> Hi Nicholas,
>>>>>
>>>>> Why not make elf_check_module_arch() consistent with the other
>>>>> arch-specific functions? Please see module_frob_arch_sections(),
>>>>> module_{init,exit}_section(), etc in moduleloader.h. That is, they are
>>>>> all __weak functions that are overridable by arches. We can maybe make
>>>>> elf_check_module_arch() a weak symbol, available for arches to
>>>>> override if they want to perform additional elf checks. Then we don't
>>>>> have to have this one-off #define.
>>>>
>>>>
>>>>Like this? I like it. Good idea.
>>>
>>> Yeah! Also, maybe we can alternatively make elf_check_module_arch() a
>>> separate check entirely so that the powerpc implementation doesn't
>>> have to include that extra elf_check_arch() call. Something like this maybe?
>>
>>Yeah we can do that. Would you be okay if it goes via powerpc tree? If
>>yes, then we should get your Ack (or SOB because it seems to be entirely
>>your patch now :D)
> 
> This can go through the powerpc tree. Will you do another respin
> of this patch? And yes, feel free to take my SOB for this one -
> 
>      Signed-off-by: Jessica Yu <jeyu@kernel.org>

You're maintainer so let's go with your preference. We can always adjust 
the arch hooks later if a need comes up. And yes I'll re post with you 
cc'ed.

Thanks,
Nick

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

* Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks
@ 2021-06-17  5:21               ` Nicholas Piggin
  0 siblings, 0 replies; 33+ messages in thread
From: Nicholas Piggin @ 2021-06-17  5:21 UTC (permalink / raw)
  To: Jessica Yu; +Cc: Michal Suchánek, linuxppc-dev, linux-kernel

Excerpts from Jessica Yu's message of June 16, 2021 10:54 pm:
> +++ Nicholas Piggin [16/06/21 11:18 +1000]:
>>Excerpts from Jessica Yu's message of June 15, 2021 10:17 pm:
>>> +++ Nicholas Piggin [15/06/21 12:05 +1000]:
>>>>Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:
>>>>> +++ Nicholas Piggin [11/06/21 19:39 +1000]:
>>>>>>The elf_check_arch() function is used to test usermode binaries, but
>>>>>>kernel modules may have more specific requirements. powerpc would like
>>>>>>to test for ABI version compatibility.
>>>>>>
>>>>>>Add an arch-overridable function elf_check_module_arch() that defaults
>>>>>>to elf_check_arch() and use it in elf_validity_check().
>>>>>>
>>>>>>Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>>>>[np: split patch, added changelog]
>>>>>>Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>>>---
>>>>>> include/linux/moduleloader.h | 5 +++++
>>>>>> kernel/module.c              | 2 +-
>>>>>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>>>>>
>>>>>>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>>>>>>index 9e09d11ffe5b..fdc042a84562 100644
>>>>>>--- a/include/linux/moduleloader.h
>>>>>>+++ b/include/linux/moduleloader.h
>>>>>>@@ -13,6 +13,11 @@
>>>>>>  * must be implemented by each architecture.
>>>>>>  */
>>>>>>
>>>>>>+// Allow arch to optionally do additional checking of module ELF header
>>>>>>+#ifndef elf_check_module_arch
>>>>>>+#define elf_check_module_arch elf_check_arch
>>>>>>+#endif
>>>>>
>>>>> Hi Nicholas,
>>>>>
>>>>> Why not make elf_check_module_arch() consistent with the other
>>>>> arch-specific functions? Please see module_frob_arch_sections(),
>>>>> module_{init,exit}_section(), etc in moduleloader.h. That is, they are
>>>>> all __weak functions that are overridable by arches. We can maybe make
>>>>> elf_check_module_arch() a weak symbol, available for arches to
>>>>> override if they want to perform additional elf checks. Then we don't
>>>>> have to have this one-off #define.
>>>>
>>>>
>>>>Like this? I like it. Good idea.
>>>
>>> Yeah! Also, maybe we can alternatively make elf_check_module_arch() a
>>> separate check entirely so that the powerpc implementation doesn't
>>> have to include that extra elf_check_arch() call. Something like this maybe?
>>
>>Yeah we can do that. Would you be okay if it goes via powerpc tree? If
>>yes, then we should get your Ack (or SOB because it seems to be entirely
>>your patch now :D)
> 
> This can go through the powerpc tree. Will you do another respin
> of this patch? And yes, feel free to take my SOB for this one -
> 
>      Signed-off-by: Jessica Yu <jeyu@kernel.org>

You're maintainer so let's go with your preference. We can always adjust 
the arch hooks later if a need comes up. And yes I'll re post with you 
cc'ed.

Thanks,
Nick

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

end of thread, other threads:[~2021-06-17  5:22 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-11  9:39 [PATCH v4 0/2] powerpc/64: Option to use ELF V2 ABI for big-endian Nicholas Piggin
2021-06-11  9:39 ` Nicholas Piggin
2021-06-11  9:39 ` [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks Nicholas Piggin
2021-06-11  9:39   ` Nicholas Piggin
2021-06-14 12:06   ` Jessica Yu
2021-06-14 12:06     ` Jessica Yu
2021-06-15  2:05     ` Nicholas Piggin
2021-06-15  2:05       ` Nicholas Piggin
2021-06-15 12:17       ` Jessica Yu
2021-06-15 12:17         ` Jessica Yu
2021-06-15 12:50         ` Segher Boessenkool
2021-06-15 12:50           ` Segher Boessenkool
2021-06-15 13:41           ` Jessica Yu
2021-06-15 13:41             ` Jessica Yu
2021-06-15 14:30             ` Segher Boessenkool
2021-06-15 14:30               ` Segher Boessenkool
2021-06-16  2:39               ` Michael Ellerman
2021-06-16  2:39                 ` Michael Ellerman
2021-06-16  1:18         ` Nicholas Piggin
2021-06-16  1:18           ` Nicholas Piggin
2021-06-16 12:54           ` Jessica Yu
2021-06-16 12:54             ` Jessica Yu
2021-06-17  5:21             ` Nicholas Piggin
2021-06-17  5:21               ` Nicholas Piggin
2021-06-16  2:37         ` Michael Ellerman
2021-06-16  2:37           ` Michael Ellerman
2021-06-16 13:49           ` Jessica Yu
2021-06-16 13:49             ` Jessica Yu
2021-06-11  9:39 ` [PATCH v4 2/2] powerpc/64: Option to use ELF V2 ABI for big-endian kernels Nicholas Piggin
2021-06-11  9:39   ` Nicholas Piggin
2021-06-11  9:58   ` Michal Suchánek
2021-06-11  9:58     ` Michal Suchánek
2021-06-11 10:20     ` Michal Suchánek

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.