[v13,1/8] parisc: Drop parisc special case for __sighandler_t

Commit Message

Peter Collingbourne Nov. 3, 2020, 4:09 a.m. UTC

From: Helge Deller <[e-mail]>

I believe we can and *should* drop this parisc-specific typedef for
__sighandler_t when compiling a 64-bit kernel. The reasons:

1. We don't have a 64-bit userspace yet, so nothing (on userspace side)
can break.

2. Inside the Linux kernel, this is only used in kernel/signal.c, in
function kernel_sigaction() where the signal handler is compared against
SIG_IGN.  SIG_IGN is defined as (__sighandler_t)1), so only the pointers
are compared.

3. Even when a 64-bit userspace gets added at some point, I think
__sighandler_t should be defined what it is: a function pointer struct.

I compiled kernel/signal.c with and without the patch, and the produced code
is identical in both cases.

Signed-off-by: Helge Deller <[e-mail]>
Reviewed-by: Peter Collingbourne <[e-mail]>
Link: https://linux-review.googlesource.com/id/I21c43f21b264f339e3aa395626af838646f62d97
---
 arch/parisc/include/uapi/asm/signal.h | 8 --------
 1 file changed, 8 deletions(-)

Comments

Eric W. Biederman Nov. 4, 2020, 4:54 p.m. UTC | #1

Peter Collingbourne <[e-mail]> writes:

> From: Helge Deller <[e-mail]>
>
> I believe we can and *should* drop this parisc-specific typedef for
> __sighandler_t when compiling a 64-bit kernel. The reasons:
>
> 1. We don't have a 64-bit userspace yet, so nothing (on userspace side)
> can break.
>
> 2. Inside the Linux kernel, this is only used in kernel/signal.c, in
> function kernel_sigaction() where the signal handler is compared against
> SIG_IGN.  SIG_IGN is defined as (__sighandler_t)1), so only the pointers
> are compared.
>
> 3. Even when a 64-bit userspace gets added at some point, I think
> __sighandler_t should be defined what it is: a function pointer struct.
>
> I compiled kernel/signal.c with and without the patch, and the produced code
> is identical in both cases.
>
> Signed-off-by: Helge Deller <[e-mail]>
> Reviewed-by: Peter Collingbourne <[e-mail]>
> Link:
> https://linux-review.googlesource.com/id/I21c43f21b264f339e3aa395626af838646f62d97

Peter as you have sent this, this also needs your Signed-off-by.

Otherwise this looks reasonable to me.
Acked-by: "Eric W. Biederman" <[e-mail]>

While the final bits look like they are still under discussion it looks
like the preceding cleanups are pretty solid at this point.

Any chance we can get the cleanups into a tree in linux-next so that
the discussion can focus on the core parts of this work?

Perhaps I should pick up the clenaups?

Eric



> ---
>  arch/parisc/include/uapi/asm/signal.h | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/arch/parisc/include/uapi/asm/signal.h b/arch/parisc/include/uapi/asm/signal.h
> index e605197b462c..d9c51769851a 100644
> --- a/arch/parisc/include/uapi/asm/signal.h
> +++ b/arch/parisc/include/uapi/asm/signal.h
> @@ -85,16 +85,8 @@
>  struct siginfo;
>  
>  /* Type of a signal handler.  */
> -#if defined(__LP64__)
> -/* function pointers on 64-bit parisc are pointers to little structs and the
> - * compiler doesn't support code which changes or tests the address of
> - * the function in the little struct.  This is really ugly -PB
> - */
> -typedef char __user *__sighandler_t;
> -#else
>  typedef void __signalfn_t(int);
>  typedef __signalfn_t __user *__sighandler_t;
> -#endif
>  
>  typedef struct sigaltstack {
>  	void __user *ss_sp;

Catalin Marinas Nov. 4, 2020, 5:24 p.m. UTC | #2

On Wed, Nov 04, 2020 at 10:54:34AM -0600, Eric W. Biederman wrote:
> Peter Collingbourne <[e-mail]> writes:
> > From: Helge Deller <[e-mail]>
> >
> > I believe we can and *should* drop this parisc-specific typedef for
> > __sighandler_t when compiling a 64-bit kernel. The reasons:
> >
> > 1. We don't have a 64-bit userspace yet, so nothing (on userspace side)
> > can break.
> >
> > 2. Inside the Linux kernel, this is only used in kernel/signal.c, in
> > function kernel_sigaction() where the signal handler is compared against
> > SIG_IGN.  SIG_IGN is defined as (__sighandler_t)1), so only the pointers
> > are compared.
> >
> > 3. Even when a 64-bit userspace gets added at some point, I think
> > __sighandler_t should be defined what it is: a function pointer struct.
> >
> > I compiled kernel/signal.c with and without the patch, and the produced code
> > is identical in both cases.
> >
> > Signed-off-by: Helge Deller <[e-mail]>
> > Reviewed-by: Peter Collingbourne <[e-mail]>
> > Link:
> > https://linux-review.googlesource.com/id/I21c43f21b264f339e3aa395626af838646f62d97
> 
> Peter as you have sent this, this also needs your Signed-off-by.
> 
> Otherwise this looks reasonable to me.
> Acked-by: "Eric W. Biederman" <[e-mail]>
> 
> While the final bits look like they are still under discussion it looks
> like the preceding cleanups are pretty solid at this point.

Minor nits, unless you nak the whole approach of SA_FAULTFLAGS and
SA_UNSUPPORTED ;) (it looks a bit complicated to me but I don't have a
better idea for a generic implementation).

> Any chance we can get the cleanups into a tree in linux-next so that
> the discussion can focus on the core parts of this work?
> 
> Perhaps I should pick up the clenaups?

However you prefer (I usually start queuing patches at -rc3). If you
pick them up, please provide a stable branch somewhere so that we can
add the others on top.

Thanks.

Eric W. Biederman Nov. 4, 2020, 5:36 p.m. UTC | #3

Catalin Marinas <[e-mail]> writes:

> On Wed, Nov 04, 2020 at 10:54:34AM -0600, Eric W. Biederman wrote:
>> Peter Collingbourne <[e-mail]> writes:
>> > From: Helge Deller <[e-mail]>
>> >
>> > I believe we can and *should* drop this parisc-specific typedef for
>> > __sighandler_t when compiling a 64-bit kernel. The reasons:
>> >
>> > 1. We don't have a 64-bit userspace yet, so nothing (on userspace side)
>> > can break.
>> >
>> > 2. Inside the Linux kernel, this is only used in kernel/signal.c, in
>> > function kernel_sigaction() where the signal handler is compared against
>> > SIG_IGN.  SIG_IGN is defined as (__sighandler_t)1), so only the pointers
>> > are compared.
>> >
>> > 3. Even when a 64-bit userspace gets added at some point, I think
>> > __sighandler_t should be defined what it is: a function pointer struct.
>> >
>> > I compiled kernel/signal.c with and without the patch, and the produced code
>> > is identical in both cases.
>> >
>> > Signed-off-by: Helge Deller <[e-mail]>
>> > Reviewed-by: Peter Collingbourne <[e-mail]>
>> > Link:
>> > https://linux-review.googlesource.com/id/I21c43f21b264f339e3aa395626af838646f62d97
>> 
>> Peter as you have sent this, this also needs your Signed-off-by.
>> 
>> Otherwise this looks reasonable to me.
>> Acked-by: "Eric W. Biederman" <[e-mail]>
>> 
>> While the final bits look like they are still under discussion it looks
>> like the preceding cleanups are pretty solid at this point.
>
> Minor nits, unless you nak the whole approach of SA_FAULTFLAGS and
> SA_UNSUPPORTED ;) (it looks a bit complicated to me but I don't have a
> better idea for a generic implementation).
>
>> Any chance we can get the cleanups into a tree in linux-next so that
>> the discussion can focus on the core parts of this work?
>> 
>> Perhaps I should pick up the clenaups?
>
> However you prefer (I usually start queuing patches at -rc3). If you
> pick them up, please provide a stable branch somewhere so that we can
> add the others on top.

I just want to make certain the cleanups don't get lost in the shuffle.

If we are almost there then I will focus my energy on reviewing the
patches and make certain there isn't something important that has been
overlooked.  I don't expect there is.


Eric

Dave Martin Nov. 4, 2020, 6 p.m. UTC | #4

On Wed, Nov 04, 2020 at 05:24:48PM +0000, Catalin Marinas wrote:
> On Wed, Nov 04, 2020 at 10:54:34AM -0600, Eric W. Biederman wrote:
> > Peter Collingbourne <[e-mail]> writes:
> > > From: Helge Deller <[e-mail]>
> > >
> > > I believe we can and *should* drop this parisc-specific typedef for
> > > __sighandler_t when compiling a 64-bit kernel. The reasons:
> > >
> > > 1. We don't have a 64-bit userspace yet, so nothing (on userspace side)
> > > can break.
> > >
> > > 2. Inside the Linux kernel, this is only used in kernel/signal.c, in
> > > function kernel_sigaction() where the signal handler is compared against
> > > SIG_IGN.  SIG_IGN is defined as (__sighandler_t)1), so only the pointers
> > > are compared.
> > >
> > > 3. Even when a 64-bit userspace gets added at some point, I think
> > > __sighandler_t should be defined what it is: a function pointer struct.
> > >
> > > I compiled kernel/signal.c with and without the patch, and the produced code
> > > is identical in both cases.
> > >
> > > Signed-off-by: Helge Deller <[e-mail]>
> > > Reviewed-by: Peter Collingbourne <[e-mail]>
> > > Link:
> > > https://linux-review.googlesource.com/id/I21c43f21b264f339e3aa395626af838646f62d97
> > 
> > Peter as you have sent this, this also needs your Signed-off-by.
> > 
> > Otherwise this looks reasonable to me.
> > Acked-by: "Eric W. Biederman" <[e-mail]>
> > 
> > While the final bits look like they are still under discussion it looks
> > like the preceding cleanups are pretty solid at this point.
> 
> Minor nits, unless you nak the whole approach of SA_FAULTFLAGS and
> SA_UNSUPPORTED ;) (it looks a bit complicated to me but I don't have a
> better idea for a generic implementation).

It is a bit complicated, but we didn't come up with anything better so
far that can cope with the various historical quirks in the signal API.

A bigger overhaul of the whole interface might be a good idea at some
point, but it would probably be a mistake to rush that.


It may be possible to make the SA_UNSUPPORTED stuff a bit more
digestible via libc.  I'll try to get a discussion started on that.

[...]

Cheers
---Dave

Peter Collingbourne Nov. 4, 2020, 8:46 p.m. UTC | #5

On Wed, Nov 4, 2020 at 9:24 AM Catalin Marinas <[e-mail]> wrote:
>
> On Wed, Nov 04, 2020 at 10:54:34AM -0600, Eric W. Biederman wrote:
> > Peter Collingbourne <[e-mail]> writes:
> > > From: Helge Deller <[e-mail]>
> > >
> > > I believe we can and *should* drop this parisc-specific typedef for
> > > __sighandler_t when compiling a 64-bit kernel. The reasons:
> > >
> > > 1. We don't have a 64-bit userspace yet, so nothing (on userspace side)
> > > can break.
> > >
> > > 2. Inside the Linux kernel, this is only used in kernel/signal.c, in
> > > function kernel_sigaction() where the signal handler is compared against
> > > SIG_IGN.  SIG_IGN is defined as (__sighandler_t)1), so only the pointers
> > > are compared.
> > >
> > > 3. Even when a 64-bit userspace gets added at some point, I think
> > > __sighandler_t should be defined what it is: a function pointer struct.
> > >
> > > I compiled kernel/signal.c with and without the patch, and the produced code
> > > is identical in both cases.
> > >
> > > Signed-off-by: Helge Deller <[e-mail]>
> > > Reviewed-by: Peter Collingbourne <[e-mail]>
> > > Link:
> > > https://linux-review.googlesource.com/id/I21c43f21b264f339e3aa395626af838646f62d97
> >
> > Peter as you have sent this, this also needs your Signed-off-by.
> >
> > Otherwise this looks reasonable to me.
> > Acked-by: "Eric W. Biederman" <[e-mail]>

Thanks, likewise for the other patches that you acked.

> > While the final bits look like they are still under discussion it looks
> > like the preceding cleanups are pretty solid at this point.
>
> Minor nits, unless you nak the whole approach of SA_FAULTFLAGS and
> SA_UNSUPPORTED ;) (it looks a bit complicated to me but I don't have a
> better idea for a generic implementation).
>
> > Any chance we can get the cleanups into a tree in linux-next so that
> > the discussion can focus on the core parts of this work?
> >
> > Perhaps I should pick up the clenaups?
>
> However you prefer (I usually start queuing patches at -rc3). If you
> pick them up, please provide a stable branch somewhere so that we can
> add the others on top.

Picking up the cleanups first sounds good to me and I don't mind which
tree they go via. To make it easier to pick up just the cleanups I
will reorder the patches a bit. I will move patch 6 to patch 4 so that
1-4 are the non-uapi-affecting cleanups and 5-8 implement the
substantive changes.

Peter

Patch

diff --git a/arch/parisc/include/uapi/asm/signal.h b/arch/parisc/include/uapi/asm/signal.h
index e605197b462c..d9c51769851a 100644
--- a/arch/parisc/include/uapi/asm/signal.h
+++ b/arch/parisc/include/uapi/asm/signal.h
@@ -85,16 +85,8 @@ 
 struct siginfo;
 
 /* Type of a signal handler.  */
-#if defined(__LP64__)
-/* function pointers on 64-bit parisc are pointers to little structs and the
- * compiler doesn't support code which changes or tests the address of
- * the function in the little struct.  This is really ugly -PB
- */
-typedef char __user *__sighandler_t;
-#else
 typedef void __signalfn_t(int);
 typedef __signalfn_t __user *__sighandler_t;
-#endif
 
 typedef struct sigaltstack {
 	void __user *ss_sp;