[PATCH 0/2] RISC-V: Handle the siginfo_t offset problem

[PATCH 0/2] RISC-V: Handle the siginfo_t offset problem

[Alistair Francis]

In the RISC-V 32-bit glibc port [1] the siginfo_t struct in the kernel
doesn't line up with the struct in glibc. In glibc world the _sifields
union is 8 byte alligned (although I can't figure out why) while in the
kernel wordl the _sifields union is 4 bytes alligned. This results in
information being lost in the waitid syscall.

This doesn't seem to be a great fix, but it is somewhat similar to what
x32 does (which has 64-bit time_t like RV32) and I can't figure out why
the two allignments are different.

1: https://github.com/alistair23/glibc/commits/alistair/rv32.next

Alistair Francis (2):
  uapi/asm-generic: Allow defining a custom __SIGINFO struct
  riscv/include/uapi: Define a custom __SIGINFO struct for RV32

 arch/riscv/include/uapi/asm/siginfo.h | 32 +++++++++++++++++++++++++++
 include/uapi/asm-generic/siginfo.h    | 32 ++++++++++++++-------------
 2 files changed, 49 insertions(+), 15 deletions(-)
 create mode 100644 arch/riscv/include/uapi/asm/siginfo.h

-- 
2.22.0

[PATCH 1/2] uapi/asm-generic: Allow defining a custom __SIGINFO struct

[Alistair Francis]

Allow defining a custom __SIGINFO struct. This allows architectures to
apply their own padding and allignment requirements to the struct. This
is similar to the __ARCH_SI_ATTRIBUTES #define that already exists, but
applies to the __SIGINFO struct instead of the siginfo_t struct.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 include/uapi/asm-generic/siginfo.h | 32 ++++++++++++++++--------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
index cb3d6c267181..09b0a1abac14 100644
--- a/include/uapi/asm-generic/siginfo.h
+++ b/include/uapi/asm-generic/siginfo.h
@@ -108,23 +108,25 @@ union __sifields {
 	} _sigsys;
 };
 
-#ifndef __ARCH_HAS_SWAPPED_SIGINFO
-#define __SIGINFO 			\
-struct {				\
-	int si_signo;			\
-	int si_errno;			\
-	int si_code;			\
-	union __sifields _sifields;	\
+#ifndef __SIGINFO
+# ifndef __ARCH_HAS_SWAPPED_SIGINFO
+# define __SIGINFO 						\
+struct {							\
+	int si_signo;						\
+	int si_errno;						\
+	int si_code;						\
+	union __sifields _sifields __ARCH_SI_ATTRIBUTES;	\
 }
-#else
-#define __SIGINFO 			\
-struct {				\
-	int si_signo;			\
-	int si_code;			\
-	int si_errno;			\
-	union __sifields _sifields;	\
+# else
+# define __SIGINFO 						\
+struct {							\
+	int si_signo;						\
+	int si_code;						\
+	int si_errno;						\
+	union __sifields _sifields __ARCH_SI_ATTRIBUTES;	\
 }
-#endif /* __ARCH_HAS_SWAPPED_SIGINFO */
+# endif /* __ARCH_HAS_SWAPPED_SIGINFO */
+#endif /* __SIGINFO */
 
 typedef struct siginfo {
 	union {
-- 
2.22.0

[PATCH 2/2] riscv/include/uapi: Define a custom __SIGINFO struct for RV32

[Alistair Francis]

The glibc implementation of siginfo_t results in an allignment of 8 bytes
for the union _sifields on RV32. The kernel has an allignment of 4 bytes
for the _sifields union. This results in information being lost when
glibc parses the siginfo_t struct.

To fix the issue add a pad variable to the struct to avoid allignment
mismatches.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 arch/riscv/include/uapi/asm/siginfo.h | 32 +++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 arch/riscv/include/uapi/asm/siginfo.h

diff --git a/arch/riscv/include/uapi/asm/siginfo.h b/arch/riscv/include/uapi/asm/siginfo.h
new file mode 100644
index 000000000000..0854ad97bf44
--- /dev/null
+++ b/arch/riscv/include/uapi/asm/siginfo.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _ASM_RISCV_SIGINFO_H
+#define _ASM_RISCV_SIGINFO_H
+
+/* Add a pad element for RISC-V 32-bit. We need this as the
+ * _sifields union is 8 byte allgined in usperace.
+ */
+#if __riscv_xlen == 32
+#ifndef __ARCH_HAS_SWAPPED_SIGINFO
+#define __SIGINFO 			\
+struct {				\
+	int si_signo;			\
+	int si_errno;			\
+	int si_code;			\
+	int pad;			\
+	union __sifields _sifields;	\
+}
+#else
+#define __SIGINFO 			\
+struct {				\
+	int si_signo;			\
+	int si_code;			\
+	int si_errno;			\
+	int pad;			\
+	union __sifields _sifields;	\
+}
+#endif /* __ARCH_HAS_SWAPPED_SIGINFO */
+#endif
+
+#include <asm-generic/siginfo.h>
+
+#endif /* _ASM_RISCV_SIGINFO_H */
-- 
2.22.0

Re: [PATCH 1/2] uapi/asm-generic: Allow defining a custom __SIGINFO struct

[Arnd Bergmann]

On Wed, Jul 3, 2019 at 2:21 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> Allow defining a custom __SIGINFO struct. This allows architectures to
> apply their own padding and allignment requirements to the struct. This
> is similar to the __ARCH_SI_ATTRIBUTES #define that already exists, but
> applies to the __SIGINFO struct instead of the siginfo_t struct.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

I don't think there should be another special case for rv32 here,
we already have too many exceptions for architectures that
were different for no good reason.

Please keep the same definition that everything else has.

       Arnd

Re: [PATCH 2/2] riscv/include/uapi: Define a custom __SIGINFO struct for RV32

[Arnd Bergmann]

On Wed, Jul 3, 2019 at 2:21 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> The glibc implementation of siginfo_t results in an allignment of 8 bytes
> for the union _sifields on RV32. The kernel has an allignment of 4 bytes
> for the _sifields union. This results in information being lost when
> glibc parses the siginfo_t struct.

I think the problem is that you incorrectly defined clock_t to 64-bit,
while it is 32 bit in the kernel. You should fix the clock_t definition
instead, it would otherwise cause additional problems.

        Arnd

Re: [PATCH 2/2] riscv/include/uapi: Define a custom __SIGINFO struct for RV32

[Alistair Francis]

On Wed, Jul 3, 2019 at 1:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Jul 3, 2019 at 2:21 AM Alistair Francis
> <alistair.francis@wdc.com> wrote:
> >
> > The glibc implementation of siginfo_t results in an allignment of 8 bytes
> > for the union _sifields on RV32. The kernel has an allignment of 4 bytes
> > for the _sifields union. This results in information being lost when
> > glibc parses the siginfo_t struct.
>
> I think the problem is that you incorrectly defined clock_t to 64-bit,
> while it is 32 bit in the kernel. You should fix the clock_t definition
> instead, it would otherwise cause additional problems.

That is the problem. I assume we want to change the kernel to use a
64-bit clock_t.

What I don't understand though is how that impacted this struct, it
doesn't use clock_t at all, everything in the struct is an int or
void*.

Alistair

>
>         Arnd

Re: [PATCH 2/2] riscv/include/uapi: Define a custom __SIGINFO struct for RV32

[Arnd Bergmann]

On Wed, Jul 3, 2019 at 8:45 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Jul 3, 2019 at 1:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Wed, Jul 3, 2019 at 2:21 AM Alistair Francis
> > <alistair.francis@wdc.com> wrote:
> > >
> > > The glibc implementation of siginfo_t results in an allignment of 8 bytes
> > > for the union _sifields on RV32. The kernel has an allignment of 4 bytes
> > > for the _sifields union. This results in information being lost when
> > > glibc parses the siginfo_t struct.
> >
> > I think the problem is that you incorrectly defined clock_t to 64-bit,
> > while it is 32 bit in the kernel. You should fix the clock_t definition
> > instead, it would otherwise cause additional problems.
>
> That is the problem. I assume we want to change the kernel to use a
> 64-bit clock_t.

Certainly not!

Doing so would likely involve deprecating all system calls that
take a clock_t (anything passing a struct siginfo or struct tms) and
replacements based on clock64_t.

> What I don't understand though is how that impacted this struct, it
> doesn't use clock_t at all, everything in the struct is an int or
> void*.

si_utime/si_stime in siginfo are clock_t.

      Arnd

Re: [PATCH 2/2] riscv/include/uapi: Define a custom __SIGINFO struct for RV32

[Alistair Francis]

On Wed, Jul 3, 2019 at 12:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Jul 3, 2019 at 8:45 PM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Jul 3, 2019 at 1:41 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > >
> > > On Wed, Jul 3, 2019 at 2:21 AM Alistair Francis
> > > <alistair.francis@wdc.com> wrote:
> > > >
> > > > The glibc implementation of siginfo_t results in an allignment of 8 bytes
> > > > for the union _sifields on RV32. The kernel has an allignment of 4 bytes
> > > > for the _sifields union. This results in information being lost when
> > > > glibc parses the siginfo_t struct.
> > >
> > > I think the problem is that you incorrectly defined clock_t to 64-bit,
> > > while it is 32 bit in the kernel. You should fix the clock_t definition
> > > instead, it would otherwise cause additional problems.
> >
> > That is the problem. I assume we want to change the kernel to use a
> > 64-bit clock_t.
>
> Certainly not!
>
> Doing so would likely involve deprecating all system calls that
> take a clock_t (anything passing a struct siginfo or struct tms) and
> replacements based on clock64_t.

Ah, that's really easy to fix then.

>
> > What I don't understand though is how that impacted this struct, it
> > doesn't use clock_t at all, everything in the struct is an int or
> > void*.
>
> si_utime/si_stime in siginfo are clock_t.

But they are further down the struct. I just assumed that GCC would
align those as required, I guess it aligns the start of the struct to
match some 64-bit members which seems strange.

Alistair

>
>       Arnd

Re: [PATCH 2/2] riscv/include/uapi: Define a custom __SIGINFO struct for RV32

[Arnd Bergmann]

On Thu, Jul 4, 2019 at 12:18 AM Alistair Francis <alistair23@gmail.com> wrote:
> On Wed, Jul 3, 2019 at 12:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wed, Jul 3, 2019 at 8:45 PM Alistair Francis <alistair23@gmail.com> wrote:
> > > What I don't understand though is how that impacted this struct, it
> > > doesn't use clock_t at all, everything in the struct is an int or
> > > void*.
> >
> > si_utime/si_stime in siginfo are clock_t.
>
> But they are further down the struct. I just assumed that GCC would
> align those as required, I guess it aligns the start of the struct to
> match some 64-bit members which seems strange.

These are the regular struct alignment rules. Essentially you would
get something like

struct s {
    int a;
    int b;
    int c;
    union {
         int d;
         long long e;
   };
   int f;
};

Since 'e' has 8 byte alignment, the same is true for the union,
and putting the union in a struct also requires the same alignment
for the struct itself, so now you get padding after 'c' and 'f'.

       Arnd

Re: [PATCH 2/2] riscv/include/uapi: Define a custom __SIGINFO struct for RV32

[Alistair Francis]

On Thu, Jul 4, 2019 at 1:34 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Jul 4, 2019 at 12:18 AM Alistair Francis <alistair23@gmail.com> wrote:
> > On Wed, Jul 3, 2019 at 12:47 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Wed, Jul 3, 2019 at 8:45 PM Alistair Francis <alistair23@gmail.com> wrote:
> > > > What I don't understand though is how that impacted this struct, it
> > > > doesn't use clock_t at all, everything in the struct is an int or
> > > > void*.
> > >
> > > si_utime/si_stime in siginfo are clock_t.
> >
> > But they are further down the struct. I just assumed that GCC would
> > align those as required, I guess it aligns the start of the struct to
> > match some 64-bit members which seems strange.
>
> These are the regular struct alignment rules. Essentially you would
> get something like
>
> struct s {
>     int a;
>     int b;
>     int c;
>     union {
>          int d;
>          long long e;
>    };
>    int f;
> };
>
> Since 'e' has 8 byte alignment, the same is true for the union,
> and putting the union in a struct also requires the same alignment
> for the struct itself, so now you get padding after 'c' and 'f'.

Now that I think about it more it does make sense. Thanks for the help
with this and all the glibc stuff.

I have a new patch set that seems to work on RV32 and RV64. I'm now
hitting issues with syscalls that glibc doesn't use but other projects
do like io_getevents in OpenSSL.

Alistair

>
>        Arnd