[PATCH v1 1/1] arm64: remove unnecessary ifdefs around is_compat_task()

[PATCH v1 1/1] arm64: remove unnecessary ifdefs around is_compat_task()

[Leonardo Bras]

Currently some parts of the codebase will test for CONFIG_COMPAT before
testing is_compat_task().

is_compat_task() is a inlined function only present on CONFIG_COMPAT.
On the other hand, for !CONFIG_COMPAT, we have in linux/compat.h:

#define is_compat_task() (0)

Since we have this define available in every usage of is_compat_task() for
!CONFIG_COMPAT, it's unnecessary to keep the ifdefs, since the compiler is
smart enough to optimize-out those snippets on CONFIG_COMPAT=n

Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
Changes since RFCv1:
- Removed unnecessary new inlined is_compat_task() for arm64
- Adjusted commit text and title
Link: https://lore.kernel.org/all/20240104192433.109983-2-leobras@redhat.com/

 arch/arm64/kernel/ptrace.c  | 6 ++----
 arch/arm64/kernel/syscall.c | 5 +----
 2 files changed, 3 insertions(+), 8 deletions(-)

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/1] arm64: remove unnecessary ifdefs around is_compat_task()

[Arnd Bergmann]

On Fri, Jan 5, 2024, at 05:15, Leonardo Bras wrote:
> Currently some parts of the codebase will test for CONFIG_COMPAT before
> testing is_compat_task().
>
> is_compat_task() is a inlined function only present on CONFIG_COMPAT.
> On the other hand, for !CONFIG_COMPAT, we have in linux/compat.h:
>
> #define is_compat_task() (0)
>
> Since we have this define available in every usage of is_compat_task() for
> !CONFIG_COMPAT, it's unnecessary to keep the ifdefs, since the compiler is
> smart enough to optimize-out those snippets on CONFIG_COMPAT=n
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/1] arm64: remove unnecessary ifdefs around is_compat_task()

[Mark Rutland]

On Fri, Jan 05, 2024 at 01:15:00AM -0300, Leonardo Bras wrote:
> Currently some parts of the codebase will test for CONFIG_COMPAT before
> testing is_compat_task().
> 
> is_compat_task() is a inlined function only present on CONFIG_COMPAT.
> On the other hand, for !CONFIG_COMPAT, we have in linux/compat.h:
> 
> #define is_compat_task() (0)
> 
> Since we have this define available in every usage of is_compat_task() for
> !CONFIG_COMPAT, it's unnecessary to keep the ifdefs, since the compiler is
> smart enough to optimize-out those snippets on CONFIG_COMPAT=n
> 
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

I tried this atop the arm64 for-next/core branch, using GCC 13.2.0; building
defconfig + CONFIG_COMPAT=n results in build errors:

[mark@lakrids:~/src/linux]% usekorg 13.2.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- Image
  CALL    scripts/checksyscalls.sh
  CC      arch/arm64/kernel/ptrace.o
arch/arm64/kernel/ptrace.c: In function 'task_user_regset_view':
arch/arm64/kernel/ptrace.c:2121:25: error: 'user_aarch32_view' undeclared (first use in this function); did you mean 'user_aarch64_view'?
 2121 |                 return &user_aarch32_view;
      |                         ^~~~~~~~~~~~~~~~~
      |                         user_aarch64_view
arch/arm64/kernel/ptrace.c:2121:25: note: each undeclared identifier is reported only once for each function it appears in
arch/arm64/kernel/ptrace.c:2123:25: error: 'user_aarch32_ptrace_view' undeclared (first use in this function)
 2123 |                 return &user_aarch32_ptrace_view;
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~
make[4]: *** [scripts/Makefile.build:243: arch/arm64/kernel/ptrace.o] Error 1
make[3]: *** [scripts/Makefile.build:480: arch/arm64/kernel] Error 2
make[2]: *** [scripts/Makefile.build:480: arch/arm64] Error 2
make[1]: *** [/home/mark/src/linux/Makefile:1911: .] Error 2
make: *** [Makefile:234: __sub-make] Error 2

... and looking at the code, user_aarch32_view and user_aarch32_ptrace_view are
both defined under ifdeffery for CONFIG_COMPAT, so that's obviously not going
to work...

That aside, removing ifdeffery is generally nice, so could you please try
building with CONFIG_COMPAT=n and see where you get to?

Thanks,
Mark.

> ---
> Changes since RFCv1:
> - Removed unnecessary new inlined is_compat_task() for arm64
> - Adjusted commit text and title
> Link: https://lore.kernel.org/all/20240104192433.109983-2-leobras@redhat.com/
> 
>  arch/arm64/kernel/ptrace.c  | 6 ++----
>  arch/arm64/kernel/syscall.c | 5 +----
>  2 files changed, 3 insertions(+), 8 deletions(-)
> 
> 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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/1] arm64: remove unnecessary ifdefs around is_compat_task()

[Arnd Bergmann]

On Fri, Jan 5, 2024, at 14:14, Mark Rutland wrote:
> On Fri, Jan 05, 2024 at 01:15:00AM -0300, Leonardo Bras wrote:
> arch/arm64/kernel/ptrace.c:2121:25: note: each undeclared identifier is 
> reported only once for each function it appears in
> arch/arm64/kernel/ptrace.c:2123:25: error: 'user_aarch32_ptrace_view' 
> undeclared (first use in this function)
>  2123 |                 return &user_aarch32_ptrace_view;
>       |                         ^~~~~~~~~~~~~~~~~~~~~~~~
> make[4]: *** [scripts/Makefile.build:243: arch/arm64/kernel/ptrace.o] 
> Error 1
> make[3]: *** [scripts/Makefile.build:480: arch/arm64/kernel] Error 2
> make[2]: *** [scripts/Makefile.build:480: arch/arm64] Error 2
> make[1]: *** [/home/mark/src/linux/Makefile:1911: .] Error 2
> make: *** [Makefile:234: __sub-make] Error 2
>
> ... and looking at the code, user_aarch32_view and user_aarch32_ptrace_view are
> both defined under ifdeffery for CONFIG_COMPAT, so that's obviously not going
> to work...

I suspect it's enough to remove all of the other
"#ifdef CONFIG_COMPAT" checks in this file and rely on
dead code elimination to remove the rest, but there might
be additional problems if some extern declarations are
hidden in an #ifdef as well.

    Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/1] arm64: remove unnecessary ifdefs around is_compat_task()

[kernel test robot]

Hi Leonardo,

kernel test robot noticed the following build errors:

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on linus/master v6.7-rc8 next-20240105]
[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/Leonardo-Bras/arm64-remove-unnecessary-ifdefs-around-is_compat_task/20240105-121957
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
patch link:    https://lore.kernel.org/r/20240105041458.126602-3-leobras%40redhat.com
patch subject: [PATCH v1 1/1] arm64: remove unnecessary ifdefs around is_compat_task()
config: arm64-randconfig-001-20240105 (https://download.01.org/0day-ci/archive/20240106/202401061219.Y2LD7LTx-lkp@intel.com/config)
compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project 7e186d366d6c7def0543acc255931f617e76dff0)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240106/202401061219.Y2LD7LTx-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/202401061219.Y2LD7LTx-lkp@intel.com/

All errors (new ones prefixed by >>):

>> arch/arm64/kernel/ptrace.c:2121:11: error: use of undeclared identifier 'user_aarch32_view'; did you mean 'user_aarch64_view'?
    2121 |                 return &user_aarch32_view;
         |                         ^~~~~~~~~~~~~~~~~
         |                         user_aarch64_view
   arch/arm64/kernel/ptrace.c:1591:38: note: 'user_aarch64_view' declared here
    1591 | static const struct user_regset_view user_aarch64_view = {
         |                                      ^
>> arch/arm64/kernel/ptrace.c:2123:11: error: use of undeclared identifier 'user_aarch32_ptrace_view'
    2123 |                 return &user_aarch32_ptrace_view;
         |                         ^
   2 errors generated.


vim +2121 arch/arm64/kernel/ptrace.c

478fcb2cdb2351 Will Deacon     2012-03-05  2111  
478fcb2cdb2351 Will Deacon     2012-03-05  2112  const struct user_regset_view *task_user_regset_view(struct task_struct *task)
478fcb2cdb2351 Will Deacon     2012-03-05  2113  {
5d220ff9420f8b Catalin Marinas 2015-07-14  2114  	/*
5d220ff9420f8b Catalin Marinas 2015-07-14  2115  	 * Core dumping of 32-bit tasks or compat ptrace requests must use the
5d220ff9420f8b Catalin Marinas 2015-07-14  2116  	 * user_aarch32_view compatible with arm32. Native ptrace requests on
5d220ff9420f8b Catalin Marinas 2015-07-14  2117  	 * 32-bit children use an extended user_aarch32_ptrace_view to allow
5d220ff9420f8b Catalin Marinas 2015-07-14  2118  	 * access to the TLS register.
5d220ff9420f8b Catalin Marinas 2015-07-14  2119  	 */
5d220ff9420f8b Catalin Marinas 2015-07-14  2120  	if (is_compat_task())
478fcb2cdb2351 Will Deacon     2012-03-05 @2121  		return &user_aarch32_view;
5d220ff9420f8b Catalin Marinas 2015-07-14  2122  	else if (is_compat_thread(task_thread_info(task)))
5d220ff9420f8b Catalin Marinas 2015-07-14 @2123  		return &user_aarch32_ptrace_view;
e802ab35101cdf Leonardo Bras   2024-01-05  2124  
478fcb2cdb2351 Will Deacon     2012-03-05  2125  	return &user_aarch64_view;
478fcb2cdb2351 Will Deacon     2012-03-05  2126  }
478fcb2cdb2351 Will Deacon     2012-03-05  2127  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/1] arm64: remove unnecessary ifdefs around is_compat_task()

[kernel test robot]

Hi Leonardo,

kernel test robot noticed the following build errors:

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on linus/master v6.7-rc8 next-20240105]
[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/Leonardo-Bras/arm64-remove-unnecessary-ifdefs-around-is_compat_task/20240105-121957
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
patch link:    https://lore.kernel.org/r/20240105041458.126602-3-leobras%40redhat.com
patch subject: [PATCH v1 1/1] arm64: remove unnecessary ifdefs around is_compat_task()
config: arm64-allnoconfig (https://download.01.org/0day-ci/archive/20240106/202401061355.h6lWuexc-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/20240106/202401061355.h6lWuexc-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/202401061355.h6lWuexc-lkp@intel.com/

All errors (new ones prefixed by >>):

   arch/arm64/kernel/ptrace.c: In function 'task_user_regset_view':
>> arch/arm64/kernel/ptrace.c:2121:25: error: 'user_aarch32_view' undeclared (first use in this function); did you mean 'user_aarch64_view'?
    2121 |                 return &user_aarch32_view;
         |                         ^~~~~~~~~~~~~~~~~
         |                         user_aarch64_view
   arch/arm64/kernel/ptrace.c:2121:25: note: each undeclared identifier is reported only once for each function it appears in
>> arch/arm64/kernel/ptrace.c:2123:25: error: 'user_aarch32_ptrace_view' undeclared (first use in this function)
    2123 |                 return &user_aarch32_ptrace_view;
         |                         ^~~~~~~~~~~~~~~~~~~~~~~~


vim +2121 arch/arm64/kernel/ptrace.c

478fcb2cdb2351 Will Deacon     2012-03-05  2111  
478fcb2cdb2351 Will Deacon     2012-03-05  2112  const struct user_regset_view *task_user_regset_view(struct task_struct *task)
478fcb2cdb2351 Will Deacon     2012-03-05  2113  {
5d220ff9420f8b Catalin Marinas 2015-07-14  2114  	/*
5d220ff9420f8b Catalin Marinas 2015-07-14  2115  	 * Core dumping of 32-bit tasks or compat ptrace requests must use the
5d220ff9420f8b Catalin Marinas 2015-07-14  2116  	 * user_aarch32_view compatible with arm32. Native ptrace requests on
5d220ff9420f8b Catalin Marinas 2015-07-14  2117  	 * 32-bit children use an extended user_aarch32_ptrace_view to allow
5d220ff9420f8b Catalin Marinas 2015-07-14  2118  	 * access to the TLS register.
5d220ff9420f8b Catalin Marinas 2015-07-14  2119  	 */
5d220ff9420f8b Catalin Marinas 2015-07-14  2120  	if (is_compat_task())
478fcb2cdb2351 Will Deacon     2012-03-05 @2121  		return &user_aarch32_view;
5d220ff9420f8b Catalin Marinas 2015-07-14  2122  	else if (is_compat_thread(task_thread_info(task)))
5d220ff9420f8b Catalin Marinas 2015-07-14 @2123  		return &user_aarch32_ptrace_view;
e802ab35101cdf Leonardo Bras   2024-01-05  2124  
478fcb2cdb2351 Will Deacon     2012-03-05  2125  	return &user_aarch64_view;
478fcb2cdb2351 Will Deacon     2012-03-05  2126  }
478fcb2cdb2351 Will Deacon     2012-03-05  2127  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/1] arm64: remove unnecessary ifdefs around is_compat_task()

[Leonardo Bras]

On Fri, Jan 05, 2024 at 03:38:05PM +0100, Arnd Bergmann wrote:
> On Fri, Jan 5, 2024, at 14:14, Mark Rutland wrote:
> > On Fri, Jan 05, 2024 at 01:15:00AM -0300, Leonardo Bras wrote:
> > arch/arm64/kernel/ptrace.c:2121:25: note: each undeclared identifier is 
> > reported only once for each function it appears in
> > arch/arm64/kernel/ptrace.c:2123:25: error: 'user_aarch32_ptrace_view' 
> > undeclared (first use in this function)
> >  2123 |                 return &user_aarch32_ptrace_view;
> >       |                         ^~~~~~~~~~~~~~~~~~~~~~~~
> > make[4]: *** [scripts/Makefile.build:243: arch/arm64/kernel/ptrace.o] 
> > Error 1
> > make[3]: *** [scripts/Makefile.build:480: arch/arm64/kernel] Error 2
> > make[2]: *** [scripts/Makefile.build:480: arch/arm64] Error 2
> > make[1]: *** [/home/mark/src/linux/Makefile:1911: .] Error 2
> > make: *** [Makefile:234: __sub-make] Error 2
> >
> > ... and looking at the code, user_aarch32_view and user_aarch32_ptrace_view are
> > both defined under ifdeffery for CONFIG_COMPAT, so that's obviously not going
> > to work...

Thanks for noticing, Mark!

> 
> I suspect it's enough to remove all of the other
> "#ifdef CONFIG_COMPAT" checks in this file and rely on
> dead code elimination to remove the rest, but there might
> be additional problems if some extern declarations are
> hidden in an #ifdef as well.
> 
>     Arnd

Sure, I sill send a v2 soon.

Thanks!
Leo

> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/1] arm64: remove unnecessary ifdefs around is_compat_task()

[Leonardo Bras]

On Mon, Jan 08, 2024 at 12:07:48PM -0300, Leonardo Bras wrote:
> On Fri, Jan 05, 2024 at 03:38:05PM +0100, Arnd Bergmann wrote:
> > On Fri, Jan 5, 2024, at 14:14, Mark Rutland wrote:
> > > On Fri, Jan 05, 2024 at 01:15:00AM -0300, Leonardo Bras wrote:
> > > arch/arm64/kernel/ptrace.c:2121:25: note: each undeclared identifier is 
> > > reported only once for each function it appears in
> > > arch/arm64/kernel/ptrace.c:2123:25: error: 'user_aarch32_ptrace_view' 
> > > undeclared (first use in this function)
> > >  2123 |                 return &user_aarch32_ptrace_view;
> > >       |                         ^~~~~~~~~~~~~~~~~~~~~~~~
> > > make[4]: *** [scripts/Makefile.build:243: arch/arm64/kernel/ptrace.o] 
> > > Error 1
> > > make[3]: *** [scripts/Makefile.build:480: arch/arm64/kernel] Error 2
> > > make[2]: *** [scripts/Makefile.build:480: arch/arm64] Error 2
> > > make[1]: *** [/home/mark/src/linux/Makefile:1911: .] Error 2
> > > make: *** [Makefile:234: __sub-make] Error 2
> > >
> > > ... and looking at the code, user_aarch32_view and user_aarch32_ptrace_view are
> > > both defined under ifdeffery for CONFIG_COMPAT, so that's obviously not going
> > > to work...
> 
> Thanks for noticing, Mark!
> 
> > 
> > I suspect it's enough to remove all of the other
> > "#ifdef CONFIG_COMPAT" checks in this file and rely on
> > dead code elimination to remove the rest, but there might
> > be additional problems if some extern declarations are
> > hidden in an #ifdef as well.

I could remove all CONFIG_COMPAT ifdefs from this file, and for compiling 
it required a few extra defines (in other files) to be moved outside of
their #ifdef CONFIG_COMPAT. Those being:

 #define VFP_STATE_SIZE         ((32 * 8) + 4)
 #define VFP_FPSCR_STAT_MASK    0xf800009f
 #define VFP_FPSCR_CTRL_MASK    0x07f79f00

 #define COMPAT_ELF_NGREG               18
 typedef unsigned int                   compat_elf_greg_t;
 typedef compat_elf_greg_t              compat_elf_gregset_t[COMPAT_ELF_NGREG];
 

OTOH, the size of the final arch/arm64/kernel/ptrace.o went from 44768 to 
56328 bytes, which I understand to be undesired.

A different (and simpler) solution is to have an empty struct in case of 
!CONFIG_COMPAT, that will be optimized out in compile-time:

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 9f8781f1fdfda..d2f275d8a3e6e 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -2107,6 +2107,9 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
 
        return ret;
 }
+#else
+static const struct user_regset_view user_aarch32_view = {};
+static const struct user_regset_view user_aarch32_ptrace_view = {};
 #endif /* CONFIG_COMPAT */
 
 const struct user_regset_view *task_user_regset_view(struct task_struct *task)

With this the patch will build successfully and arch/arm64/kernel/ptrace.o 
will be able to keep it's original size.

Arnd, is that ok?

Thanks!
Leo



> > 
> >     Arnd
> 
> Sure, I sill send a v2 soon.
> 
> Thanks!
> Leo
> 
> > 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/1] arm64: remove unnecessary ifdefs around is_compat_task()

[Arnd Bergmann]

On Mon, Jan 8, 2024, at 17:04, Leonardo Bras wrote:
> On Mon, Jan 08, 2024 at 12:07:48PM -0300, Leonardo Bras wrote:
>> On Fri, Jan 05, 2024 at 03:38:05PM +0100, Arnd Bergmann wrote:
>> > 
>> > I suspect it's enough to remove all of the other
>> > "#ifdef CONFIG_COMPAT" checks in this file and rely on
>> > dead code elimination to remove the rest, but there might
>> > be additional problems if some extern declarations are
>> > hidden in an #ifdef as well.
>
> I could remove all CONFIG_COMPAT ifdefs from this file, and for compiling 
> it required a few extra defines (in other files) to be moved outside of
> their #ifdef CONFIG_COMPAT. Those being:
>
>  #define VFP_STATE_SIZE         ((32 * 8) + 4)
>  #define VFP_FPSCR_STAT_MASK    0xf800009f
>  #define VFP_FPSCR_CTRL_MASK    0x07f79f00
>
>  #define COMPAT_ELF_NGREG               18
>  typedef unsigned int                   compat_elf_greg_t;
>  typedef compat_elf_greg_t              compat_elf_gregset_t[COMPAT_ELF_NGREG];
> 
>
> OTOH, the size of the final arch/arm64/kernel/ptrace.o went from 44768 to 
> 56328 bytes, which I understand to be undesired.

Right, unfortunately it seems that compat_arch_ptrace() is
globally visible and consequently not dropped by the compiler
in dead code elimination.

> A different (and simpler) solution is to have an empty struct in case of 
> !CONFIG_COMPAT, that will be optimized out in compile-time:
>
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 9f8781f1fdfda..d2f275d8a3e6e 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -2107,6 +2107,9 @@ long compat_arch_ptrace(struct task_struct 
> *child, compat_long_t request,
> 
>         return ret;
>  }
> +#else
> +static const struct user_regset_view user_aarch32_view = {};
> +static const struct user_regset_view user_aarch32_ptrace_view = {};
>  #endif /* CONFIG_COMPAT */
> 
>  const struct user_regset_view *task_user_regset_view(struct task_struct *task)
>
> With this the patch will build successfully and arch/arm64/kernel/ptrace.o 
> will be able to keep it's original size.
>
> Arnd, is that ok?

I don't see it being worth it if you add extra unused lines
in order to remove one more #ifdef. I would either leave the
task_user_regset_view() function unchanged here, or (if this
works) move the #ifdef down a few lines so the existing
user_regset_view structures can be shared:

@@ -1595,7 +1595,6 @@ static const struct user_regset_view user_aarch64_view = {
        .regsets = aarch64_regsets, .n = ARRAY_SIZE(aarch64_regsets)
 };
 
-#ifdef CONFIG_COMPAT
 enum compat_regset {
        REGSET_COMPAT_GPR,
        REGSET_COMPAT_VFP,
@@ -1852,6 +1851,7 @@ static const struct user_regset_view user_aarch32_ptrace_view = {
        .regsets = aarch32_ptrace_regsets, .n = ARRAY_SIZE(aarch32_ptrace_regsets)
 };
 
+#ifdef CONFIG_COMPAT
 static int compat_ptrace_read_user(struct task_struct *tsk, compat_ulong_t off,
                                   compat_ulong_t __user *ret)
 {


     Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/1] arm64: remove unnecessary ifdefs around is_compat_task()

[Leonardo Bras]

On Mon, Jan 08, 2024 at 05:26:26PM +0100, Arnd Bergmann wrote:
> On Mon, Jan 8, 2024, at 17:04, Leonardo Bras wrote:
> > On Mon, Jan 08, 2024 at 12:07:48PM -0300, Leonardo Bras wrote:
> >> On Fri, Jan 05, 2024 at 03:38:05PM +0100, Arnd Bergmann wrote:
> >> > 
> >> > I suspect it's enough to remove all of the other
> >> > "#ifdef CONFIG_COMPAT" checks in this file and rely on
> >> > dead code elimination to remove the rest, but there might
> >> > be additional problems if some extern declarations are
> >> > hidden in an #ifdef as well.
> >
> > I could remove all CONFIG_COMPAT ifdefs from this file, and for compiling 
> > it required a few extra defines (in other files) to be moved outside of
> > their #ifdef CONFIG_COMPAT. Those being:
> >
> >  #define VFP_STATE_SIZE         ((32 * 8) + 4)
> >  #define VFP_FPSCR_STAT_MASK    0xf800009f
> >  #define VFP_FPSCR_CTRL_MASK    0x07f79f00
> >
> >  #define COMPAT_ELF_NGREG               18
> >  typedef unsigned int                   compat_elf_greg_t;
> >  typedef compat_elf_greg_t              compat_elf_gregset_t[COMPAT_ELF_NGREG];
> > 
> >
> > OTOH, the size of the final arch/arm64/kernel/ptrace.o went from 44768 to 
> > 56328 bytes, which I understand to be undesired.
> 
> Right, unfortunately it seems that compat_arch_ptrace() is
> globally visible and consequently not dropped by the compiler
> in dead code elimination.
> 
> > A different (and simpler) solution is to have an empty struct in case of 
> > !CONFIG_COMPAT, that will be optimized out in compile-time:
> >
> > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> > index 9f8781f1fdfda..d2f275d8a3e6e 100644
> > --- a/arch/arm64/kernel/ptrace.c
> > +++ b/arch/arm64/kernel/ptrace.c
> > @@ -2107,6 +2107,9 @@ long compat_arch_ptrace(struct task_struct 
> > *child, compat_long_t request,
> > 
> >         return ret;
> >  }
> > +#else
> > +static const struct user_regset_view user_aarch32_view = {};
> > +static const struct user_regset_view user_aarch32_ptrace_view = {};
> >  #endif /* CONFIG_COMPAT */
> > 
> >  const struct user_regset_view *task_user_regset_view(struct task_struct *task)
> >
> > With this the patch will build successfully and arch/arm64/kernel/ptrace.o 
> > will be able to keep it's original size.
> >
> > Arnd, is that ok?
> 
> I don't see it being worth it if you add extra unused lines
> in order to remove one more #ifdef. I would either leave the
> task_user_regset_view() function unchanged here, or (if this
> works) move the #ifdef down a few lines so the existing
> user_regset_view structures can be shared:
> 
> @@ -1595,7 +1595,6 @@ static const struct user_regset_view user_aarch64_view = {
>         .regsets = aarch64_regsets, .n = ARRAY_SIZE(aarch64_regsets)
>  };
>  
> -#ifdef CONFIG_COMPAT
>  enum compat_regset {
>         REGSET_COMPAT_GPR,
>         REGSET_COMPAT_VFP,
> @@ -1852,6 +1851,7 @@ static const struct user_regset_view user_aarch32_ptrace_view = {
>         .regsets = aarch32_ptrace_regsets, .n = ARRAY_SIZE(aarch32_ptrace_regsets)
>  };
>  
> +#ifdef CONFIG_COMPAT
>  static int compat_ptrace_read_user(struct task_struct *tsk, compat_ulong_t off,
>                                    compat_ulong_t __user *ret)
>  {
> 

Ok, with the previous defines moved outside !CONFIG_COMPAT, this works as a 
charm and keeps arch/arm64/kernel/ptrace.o with the same size. 

I will send a v2 soon.

Thanks!
Leo

> 
>      Arnd
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel