Currently some parts of the codebase will test for CONFIG_COMPAT before
testing is_compat_task(), probably in order to avoid a run-time test into
the task structure, while other parts of codebase will just test even when
the option is not compiled in.
Since is_compat_task() is an inlined function, it would be helpful to add a
!CONFIG_COMPAT version of the helper, allowing compile-time optimization.
With this, the compiler is able to understand in build-time that
is_compat_task() will always return 0, and optimize-out some of the extra
code introduced by the option.
This allows optimizing-out code when the option is not selected, and
otherwise removing a lot #ifdefs that were introduced, making the code
more clean.
Signed-off-by: Leonardo Bras <[email protected]>
---
arch/arm64/include/asm/compat.h | 5 +++++
arch/arm64/kernel/ptrace.c | 6 ++----
arch/arm64/kernel/syscall.c | 5 +----
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h
index ae904a1ad5293..3cc61cbbb9062 100644
--- a/arch/arm64/include/asm/compat.h
+++ b/arch/arm64/include/asm/compat.h
@@ -100,6 +100,11 @@ long compat_arm_syscall(struct pt_regs *regs, int scno);
#else /* !CONFIG_COMPAT */
+static inline int is_compat_task(void)
+{
+ return 0;
+}
+
static inline int is_compat_thread(struct thread_info *thread)
{
return 0;
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 20d7ef82de90a..9f8781f1fdfda 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -173,7 +173,6 @@ static void ptrace_hbptriggered(struct perf_event *bp,
struct arch_hw_breakpoint *bkpt = counter_arch_bp(bp);
const char *desc = "Hardware breakpoint trap (ptrace)";
-#ifdef CONFIG_COMPAT
if (is_compat_task()) {
int si_errno = 0;
int i;
@@ -195,7 +194,7 @@ static void ptrace_hbptriggered(struct perf_event *bp,
desc);
return;
}
-#endif
+
arm64_force_sig_fault(SIGTRAP, TRAP_HWBKPT, bkpt->trigger, desc);
}
@@ -2112,7 +2111,6 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
const struct user_regset_view *task_user_regset_view(struct task_struct *task)
{
-#ifdef CONFIG_COMPAT
/*
* Core dumping of 32-bit tasks or compat ptrace requests must use the
* user_aarch32_view compatible with arm32. Native ptrace requests on
@@ -2123,7 +2121,7 @@ const struct user_regset_view *task_user_regset_view(struct task_struct *task)
return &user_aarch32_view;
else if (is_compat_thread(task_thread_info(task)))
return &user_aarch32_ptrace_view;
-#endif
+
return &user_aarch64_view;
}
diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
index 9a70d9746b661..ad198262b9817 100644
--- a/arch/arm64/kernel/syscall.c
+++ b/arch/arm64/kernel/syscall.c
@@ -20,14 +20,11 @@ long sys_ni_syscall(void);
static long do_ni_syscall(struct pt_regs *regs, int scno)
{
-#ifdef CONFIG_COMPAT
- long ret;
if (is_compat_task()) {
- ret = compat_arm_syscall(regs, scno);
+ long ret = compat_arm_syscall(regs, scno);
if (ret != -ENOSYS)
return ret;
}
-#endif
return sys_ni_syscall();
}
--
2.43.0
On Thu, Jan 4, 2024, at 20:24, Leonardo Bras wrote:
> Currently some parts of the codebase will test for CONFIG_COMPAT before
> testing is_compat_task(), probably in order to avoid a run-time test into
> the task structure, while other parts of codebase will just test even when
> the option is not compiled in.
>
> Since is_compat_task() is an inlined function, it would be helpful to add a
> !CONFIG_COMPAT version of the helper, allowing compile-time optimization.
>
> With this, the compiler is able to understand in build-time that
> is_compat_task() will always return 0, and optimize-out some of the extra
> code introduced by the option.
>
> This allows optimizing-out code when the option is not selected, and
> otherwise removing a lot #ifdefs that were introduced, making the code
> more clean.
>
> Signed-off-by: Leonardo Bras <[email protected]>
This looks like a useful cleanup to me, with one change:
> ---
> arch/arm64/include/asm/compat.h | 5 +++++
> arch/arm64/kernel/ptrace.c | 6 ++----
> arch/arm64/kernel/syscall.c | 5 +----
> 3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h
> index ae904a1ad5293..3cc61cbbb9062 100644
> --- a/arch/arm64/include/asm/compat.h
> +++ b/arch/arm64/include/asm/compat.h
> @@ -100,6 +100,11 @@ long compat_arm_syscall(struct pt_regs *regs, int scno);
>
> #else /* !CONFIG_COMPAT */
>
> +static inline int is_compat_task(void)
> +{
> + return 0;
> +}
> +
I think this bit is not even needed as long as users
include linux/compat.h rather than asm/compat.h, as there
is already a macro definition in the common file:
#define is_compat_task() (0)
Arnd
On Thu, Jan 04, 2024 at 09:43:56PM +0100, Arnd Bergmann wrote:
> On Thu, Jan 4, 2024, at 20:24, Leonardo Bras wrote:
> > Currently some parts of the codebase will test for CONFIG_COMPAT before
> > testing is_compat_task(), probably in order to avoid a run-time test into
> > the task structure, while other parts of codebase will just test even when
> > the option is not compiled in.
> >
> > Since is_compat_task() is an inlined function, it would be helpful to add a
> > !CONFIG_COMPAT version of the helper, allowing compile-time optimization.
> >
> > With this, the compiler is able to understand in build-time that
> > is_compat_task() will always return 0, and optimize-out some of the extra
> > code introduced by the option.
> >
> > This allows optimizing-out code when the option is not selected, and
> > otherwise removing a lot #ifdefs that were introduced, making the code
> > more clean.
> >
> > Signed-off-by: Leonardo Bras <[email protected]>
>
> This looks like a useful cleanup to me,
Hello Arnd, thanks for reviewing!
> with one change:
>
> > ---
> > arch/arm64/include/asm/compat.h | 5 +++++
> > arch/arm64/kernel/ptrace.c | 6 ++----
> > arch/arm64/kernel/syscall.c | 5 +----
> > 3 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h
> > index ae904a1ad5293..3cc61cbbb9062 100644
> > --- a/arch/arm64/include/asm/compat.h
> > +++ b/arch/arm64/include/asm/compat.h
> > @@ -100,6 +100,11 @@ long compat_arm_syscall(struct pt_regs *regs, int scno);
> >
> > #else /* !CONFIG_COMPAT */
> >
> > +static inline int is_compat_task(void)
> > +{
> > + return 0;
> > +}
> > +
>
> I think this bit is not even needed as long as users
> include linux/compat.h rather than asm/compat.h, as there
> is already a macro definition in the common file:
>
> #define is_compat_task() (0)
>
Oh, I was unaware of this macro. Thanks for pointing it out!
I just checked every use of is_compat_task() in the codebase for (arch ==
arm64 && non-arch code), and it seems like the file will either include
linux/compat.h or another header which includes linux/compat.h.
So it's safe to assume the macro will be available for every user.
I will send a v1 soon.
Thanks!
Leo