[2/4] arm64/process: Make loading of 32bit processes depend on aarch32_enabled()

Message ID a40565807874c9ca82d60c118225ee65fe668fcd.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
  Major aspect of Aarch32 emulation is the ability to load 32bit
processes.
That's currently decided (among others) by compat_elf_check_arch().

Make the macro use aarch32_enabled() to decide if Aarch32 compat is
enabled before loading a 32bit process.

Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
---
 arch/arm64/kernel/process.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Comments

Mark Rutland Oct. 18, 2023, 12:52 p.m. UTC | #1
On Wed, Oct 18, 2023 at 01:13:20PM +0200, Andrea della Porta wrote:
> Major aspect of Aarch32 emulation is the ability to load 32bit
> processes.
> That's currently decided (among others) by compat_elf_check_arch().
> 
> Make the macro use aarch32_enabled() to decide if Aarch32 compat is
> enabled before loading a 32bit process.
> 
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>

Why can't you make system_supports_32bit_el0() take the option into account
instead?

Mark.

> ---
>  arch/arm64/kernel/process.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 657ea273c0f9..96832f1ec3ee 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -601,7 +601,7 @@ unsigned long arch_align_stack(unsigned long sp)
>  #ifdef CONFIG_COMPAT
>  int compat_elf_check_arch(const struct elf32_hdr *hdr)
>  {
> -	if (!system_supports_32bit_el0())
> +	if (!system_supports_32bit_el0() || !aarch32_enabled())
>  		return false;
>  
>  	if ((hdr)->e_machine != EM_ARM)
> -- 
> 2.35.3
>
  
Andrea della Porta Oct. 19, 2023, 12:38 p.m. UTC | #2
On 13:52 Wed 18 Oct     , Mark Rutland wrote:
> On Wed, Oct 18, 2023 at 01:13:20PM +0200, Andrea della Porta wrote:
> > Major aspect of Aarch32 emulation is the ability to load 32bit
> > processes.
> > That's currently decided (among others) by compat_elf_check_arch().
> > 
> > Make the macro use aarch32_enabled() to decide if Aarch32 compat is
> > enabled before loading a 32bit process.
> > 
> > Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> 
> Why can't you make system_supports_32bit_el0() take the option into account
> instead?
>

I may be wrong here, but it seems to me that system_supports_32bit_el0()
answers teh question "can this system supports compat execution?" rather than
"do I want this system to run any compat execution?". That's the point of
aarch32_enabled(), to state whether we want teh system to run A32 code or not,
regardless of the system supporting it (of course, if the system does not
support A32 in EL0, this is a no-no, but that's another story).

Andrea
  
Mark Rutland Oct. 19, 2023, 2:27 p.m. UTC | #3
On Thu, Oct 19, 2023 at 02:38:32PM +0200, Andrea della Porta wrote:
> On 13:52 Wed 18 Oct     , Mark Rutland wrote:
> > On Wed, Oct 18, 2023 at 01:13:20PM +0200, Andrea della Porta wrote:
> > > Major aspect of Aarch32 emulation is the ability to load 32bit
> > > processes.
> > > That's currently decided (among others) by compat_elf_check_arch().
> > > 
> > > Make the macro use aarch32_enabled() to decide if Aarch32 compat is
> > > enabled before loading a 32bit process.
> > > 
> > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> > 
> > Why can't you make system_supports_32bit_el0() take the option into account
> > instead?
> >
> 
> I may be wrong here, but it seems to me that system_supports_32bit_el0()
> answers teh question "can this system supports compat execution?" rather than
> "do I want this system to run any compat execution?". That's the point of
> aarch32_enabled(), to state whether we want teh system to run A32 code or not,
> regardless of the system supporting it (of course, if the system does not
> support A32 in EL0, this is a no-no, but that's another story).

That's what the implementation does today, but we're really using it as a "do
we intend for 32-bit EL0 to work?" predicate, and generally the
system_supports_${FEATURE}() helpers are affected by the combination of actual
HW support, kernel config options, *and* kernel command line options. For
example, system_supports_sve() is affected by both CONFIG_ARM64_SVE and the
"arm64.nosve" command line option.

Thanks,
Mark.
  
Andrea della Porta Oct. 19, 2023, 2:32 p.m. UTC | #4
On 15:27 Thu 19 Oct     , Mark Rutland wrote:
> On Thu, Oct 19, 2023 at 02:38:32PM +0200, Andrea della Porta wrote:
> > On 13:52 Wed 18 Oct     , Mark Rutland wrote:
> > > On Wed, Oct 18, 2023 at 01:13:20PM +0200, Andrea della Porta wrote:
> > > > Major aspect of Aarch32 emulation is the ability to load 32bit
> > > > processes.
> > > > That's currently decided (among others) by compat_elf_check_arch().
> > > > 
> > > > Make the macro use aarch32_enabled() to decide if Aarch32 compat is
> > > > enabled before loading a 32bit process.
> > > > 
> > > > Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> > > 
> > > Why can't you make system_supports_32bit_el0() take the option into account
> > > instead?
> > >
> > 
> > I may be wrong here, but it seems to me that system_supports_32bit_el0()
> > answers teh question "can this system supports compat execution?" rather than
> > "do I want this system to run any compat execution?". That's the point of
> > aarch32_enabled(), to state whether we want teh system to run A32 code or not,
> > regardless of the system supporting it (of course, if the system does not
> > support A32 in EL0, this is a no-no, but that's another story).
> 
> That's what the implementation does today, but we're really using it as a "do
> we intend for 32-bit EL0 to work?" predicate, and generally the
> system_supports_${FEATURE}() helpers are affected by the combination of actual
> HW support, kernel config options, *and* kernel command line options. For
> example, system_supports_sve() is affected by both CONFIG_ARM64_SVE and the
> "arm64.nosve" command line option.
> 
> Thanks,
> Mark.

Many thanks for the explanation, then inserting aach32_enabled() in
system_supports_32bit_el0() is the way to go.

Andrea
  

Patch

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 657ea273c0f9..96832f1ec3ee 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -601,7 +601,7 @@  unsigned long arch_align_stack(unsigned long sp)
 #ifdef CONFIG_COMPAT
 int compat_elf_check_arch(const struct elf32_hdr *hdr)
 {
-	if (!system_supports_32bit_el0())
+	if (!system_supports_32bit_el0() || !aarch32_enabled())
 		return false;
 
 	if ((hdr)->e_machine != EM_ARM)