diff mbox series

[3/4] arm64/entry-common: Make Aarch32 syscalls' availability depend on aarch32_enabled()

Message ID 88bdea628a13747bff32c0c3055d6d6ef7264d96.1697614386.git.andrea.porta@suse.com
State New
Headers
Series arm64: Make Aarch32 compatibility enablement optional at boot |

Commit Message

Andrea della Porta Oct. 18, 2023, 11:13 a.m. UTC 
  Another major aspect of supporting running of 32bit processes is the
ability to access 32bit syscalls. Such syscalls can be invoked by
using the svc instruction.

If Aarch32 emulation is disabled ensure that calling svc results
in the same behavior as if CONFIG_COMPAT has not been enabled (i.e.
a kernel panic).

Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
---
 arch/arm64/kernel/entry-common.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

Comments

Mark Rutland Oct. 18, 2023, 12:57 p.m. UTC | #1 
On Wed, Oct 18, 2023 at 01:13:21PM +0200, Andrea della Porta wrote:
> Another major aspect of supporting running of 32bit processes is the
> ability to access 32bit syscalls. Such syscalls can be invoked by
> using the svc instruction.
> 
> If Aarch32 emulation is disabled ensure that calling svc results
> in the same behavior as if CONFIG_COMPAT has not been enabled (i.e.
> a kernel panic).

It's not "emulation" it's directly supported by the hardware.

> 
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> ---
>  arch/arm64/kernel/entry-common.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 69ff9b8c0bde..32761760d9dd 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -802,6 +802,11 @@ asmlinkage void noinstr el0t_64_error_handler(struct pt_regs *regs)
>  }
>  
>  #ifdef CONFIG_COMPAT
> +UNHANDLED(el0t, 32, sync_ni)
> +UNHANDLED(el0t, 32, irq_ni)
> +UNHANDLED(el0t, 32, fiq_ni)
> +UNHANDLED(el0t, 32, error_ni)

IRQ, FIQ, and SError are not syscalls, so the commit title is bad.

> +
>  static void noinstr el0_cp15(struct pt_regs *regs, unsigned long esr)
>  {
>  	enter_from_user_mode(regs);
> @@ -821,6 +826,11 @@ static void noinstr el0_svc_compat(struct pt_regs *regs)
>  
>  asmlinkage void noinstr el0t_32_sync_handler(struct pt_regs *regs)
>  {
> +	if (!aarch32_enabled()) {
> +		el0t_32_sync_ni_handler(regs);
> +		return;
> +	}

Why do we have to do this at all?

If we don't have AArch32 tasks, these paths are unreachable. Why do we need to
check that they aren't called?

Mark.

> +
>  	unsigned long esr = read_sysreg(esr_el1);
>  
>  	switch (ESR_ELx_EC(esr)) {
> @@ -865,17 +875,26 @@ asmlinkage void noinstr el0t_32_sync_handler(struct pt_regs *regs)
>  
>  asmlinkage void noinstr el0t_32_irq_handler(struct pt_regs *regs)
>  {
> -	__el0_irq_handler_common(regs);
> +	if (!aarch32_enabled())
> +		el0t_32_irq_ni_handler(regs);
> +	else
> +		__el0_irq_handler_common(regs);
>  }
>  
>  asmlinkage void noinstr el0t_32_fiq_handler(struct pt_regs *regs)
>  {
> -	__el0_fiq_handler_common(regs);
> +	if (!aarch32_enabled())
> +		el0t_32_fiq_ni_handler(regs);
> +	else
> +		__el0_fiq_handler_common(regs);
>  }
>  
>  asmlinkage void noinstr el0t_32_error_handler(struct pt_regs *regs)
>  {
> -	__el0_error_handler_common(regs);
> +	if (!aarch32_enabled())
> +		el0t_32_error_ni_handler(regs);
> +	else
> +		__el0_error_handler_common(regs);
>  }
>  
>  bool __aarch32_enabled __ro_after_init = true;
> -- 
> 2.35.3
>
Andrea della Porta Oct. 19, 2023, 12:48 p.m. UTC | #2 
On 13:57 Wed 18 Oct     , Mark Rutland wrote:
> On Wed, Oct 18, 2023 at 01:13:21PM +0200, Andrea della Porta wrote:
> > Another major aspect of supporting running of 32bit processes is the
> > ability to access 32bit syscalls. Such syscalls can be invoked by
> > using the svc instruction.
> > 
> > If Aarch32 emulation is disabled ensure that calling svc results
> > in the same behavior as if CONFIG_COMPAT has not been enabled (i.e.
> > a kernel panic).
> 
> It's not "emulation" it's directly supported by the hardware.

You're right. I also struggled to use this label but I just reused the same
name from Nikolai's patchset for x86, in the hope that the option would be
more recognizable (something like 'ARCH_emulation' that could be used maybe
for other platforms as well), but I agree with you that this is highly
misleading. I will change it to something more straightforward.

> 
> > 
> > Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> > ---
> >  arch/arm64/kernel/entry-common.c | 25 ++++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> > index 69ff9b8c0bde..32761760d9dd 100644
> > --- a/arch/arm64/kernel/entry-common.c
> > +++ b/arch/arm64/kernel/entry-common.c
> > @@ -802,6 +802,11 @@ asmlinkage void noinstr el0t_64_error_handler(struct pt_regs *regs)
> >  }
> >  
> >  #ifdef CONFIG_COMPAT
> > +UNHANDLED(el0t, 32, sync_ni)
> > +UNHANDLED(el0t, 32, irq_ni)
> > +UNHANDLED(el0t, 32, fiq_ni)
> > +UNHANDLED(el0t, 32, error_ni)
> 
> IRQ, FIQ, and SError are not syscalls, so the commit title is bad.

Agreed. I'll call them exceptions.

> 
> > +
> >  static void noinstr el0_cp15(struct pt_regs *regs, unsigned long esr)
> >  {
> >  	enter_from_user_mode(regs);
> > @@ -821,6 +826,11 @@ static void noinstr el0_svc_compat(struct pt_regs *regs)
> >  
> >  asmlinkage void noinstr el0t_32_sync_handler(struct pt_regs *regs)
> >  {
> > +	if (!aarch32_enabled()) {
> > +		el0t_32_sync_ni_handler(regs);
> > +		return;
> > +	}
> 
> Why do we have to do this at all?
> 
> If we don't have AArch32 tasks, these paths are unreachable. Why do we need to
> check that they aren't called?
> 
> Mark.

Agreed. Please see also my previous comments here:
https://lore.kernel.org/lkml/ZTEKabxNdegsbxyv@apocalypse/
https://lore.kernel.org/lkml/ZTD0DAes-J-YQ2eu@apocalypse/

but again, that's only speculative as of now, so we can ignore that part.

Andrea
kernel test robot Oct. 22, 2023, 8:30 p.m. UTC | #3 
Hi Andrea,

kernel test robot noticed the following build warnings:

[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on arm-perf/for-next/perf arm/for-next arm/fixes kvmarm/next soc/for-next linus/master v6.6-rc6 next-20231020]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andrea-della-Porta/arm64-Introduce-aarch32_enabled/20231018-191517
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
patch link:    https://lore.kernel.org/r/88bdea628a13747bff32c0c3055d6d6ef7264d96.1697614386.git.andrea.porta%40suse.com
patch subject: [PATCH 3/4] arm64/entry-common: Make Aarch32 syscalls' availability depend on aarch32_enabled()
config: arm64-randconfig-003-20231023 (https://download.01.org/0day-ci/archive/20231023/202310230423.r2U4Lqr8-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231023/202310230423.r2U4Lqr8-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310230423.r2U4Lqr8-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> arch/arm64/kernel/entry-common.c:805:11: warning: no previous prototype for 'el0t_32_sync_ni_handler' [-Wmissing-prototypes]
     805 | UNHANDLED(el0t, 32, sync_ni)
         |           ^~~~
   arch/arm64/kernel/entry-common.c:302:25: note: in definition of macro 'UNHANDLED'
     302 | asmlinkage void noinstr el##_##regsize##_##vector##_handler(struct pt_regs *regs)       \
         |                         ^~
>> arch/arm64/kernel/entry-common.c:806:11: warning: no previous prototype for 'el0t_32_irq_ni_handler' [-Wmissing-prototypes]
     806 | UNHANDLED(el0t, 32, irq_ni)
         |           ^~~~
   arch/arm64/kernel/entry-common.c:302:25: note: in definition of macro 'UNHANDLED'
     302 | asmlinkage void noinstr el##_##regsize##_##vector##_handler(struct pt_regs *regs)       \
         |                         ^~
>> arch/arm64/kernel/entry-common.c:807:11: warning: no previous prototype for 'el0t_32_fiq_ni_handler' [-Wmissing-prototypes]
     807 | UNHANDLED(el0t, 32, fiq_ni)
         |           ^~~~
   arch/arm64/kernel/entry-common.c:302:25: note: in definition of macro 'UNHANDLED'
     302 | asmlinkage void noinstr el##_##regsize##_##vector##_handler(struct pt_regs *regs)       \
         |                         ^~
>> arch/arm64/kernel/entry-common.c:808:11: warning: no previous prototype for 'el0t_32_error_ni_handler' [-Wmissing-prototypes]
     808 | UNHANDLED(el0t, 32, error_ni)
         |           ^~~~
   arch/arm64/kernel/entry-common.c:302:25: note: in definition of macro 'UNHANDLED'
     302 | asmlinkage void noinstr el##_##regsize##_##vector##_handler(struct pt_regs *regs)       \
         |                         ^~


vim +/el0t_32_sync_ni_handler +805 arch/arm64/kernel/entry-common.c

   803	
   804	#ifdef CONFIG_COMPAT
 > 805	UNHANDLED(el0t, 32, sync_ni)
 > 806	UNHANDLED(el0t, 32, irq_ni)
 > 807	UNHANDLED(el0t, 32, fiq_ni)
 > 808	UNHANDLED(el0t, 32, error_ni)
   809
diff mbox series

Patch

diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 69ff9b8c0bde..32761760d9dd 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -802,6 +802,11 @@  asmlinkage void noinstr el0t_64_error_handler(struct pt_regs *regs)
 }
 
 #ifdef CONFIG_COMPAT
+UNHANDLED(el0t, 32, sync_ni)
+UNHANDLED(el0t, 32, irq_ni)
+UNHANDLED(el0t, 32, fiq_ni)
+UNHANDLED(el0t, 32, error_ni)
+
 static void noinstr el0_cp15(struct pt_regs *regs, unsigned long esr)
 {
 	enter_from_user_mode(regs);
@@ -821,6 +826,11 @@  static void noinstr el0_svc_compat(struct pt_regs *regs)
 
 asmlinkage void noinstr el0t_32_sync_handler(struct pt_regs *regs)
 {
+	if (!aarch32_enabled()) {
+		el0t_32_sync_ni_handler(regs);
+		return;
+	}
+
 	unsigned long esr = read_sysreg(esr_el1);
 
 	switch (ESR_ELx_EC(esr)) {
@@ -865,17 +875,26 @@  asmlinkage void noinstr el0t_32_sync_handler(struct pt_regs *regs)
 
 asmlinkage void noinstr el0t_32_irq_handler(struct pt_regs *regs)
 {
-	__el0_irq_handler_common(regs);
+	if (!aarch32_enabled())
+		el0t_32_irq_ni_handler(regs);
+	else
+		__el0_irq_handler_common(regs);
 }
 
 asmlinkage void noinstr el0t_32_fiq_handler(struct pt_regs *regs)
 {
-	__el0_fiq_handler_common(regs);
+	if (!aarch32_enabled())
+		el0t_32_fiq_ni_handler(regs);
+	else
+		__el0_fiq_handler_common(regs);
 }
 
 asmlinkage void noinstr el0t_32_error_handler(struct pt_regs *regs)
 {
-	__el0_error_handler_common(regs);
+	if (!aarch32_enabled())
+		el0t_32_error_ni_handler(regs);
+	else
+		__el0_error_handler_common(regs);
 }
 
 bool __aarch32_enabled __ro_after_init = true;