2019-07-03 00:31:30

by Alistair Francis

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

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 <[email protected]>
---
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


2019-07-03 08:41:52

by Arnd Bergmann

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

On Wed, Jul 3, 2019 at 2:21 AM Alistair Francis
<[email protected]> 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

2019-07-03 18:47:00

by Alistair Francis

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

On Wed, Jul 3, 2019 at 1:41 AM Arnd Bergmann <[email protected]> wrote:
>
> On Wed, Jul 3, 2019 at 2:21 AM Alistair Francis
> <[email protected]> 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

2019-07-03 19:49:08

by Arnd Bergmann

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

On Wed, Jul 3, 2019 at 8:45 PM Alistair Francis <[email protected]> wrote:
>
> On Wed, Jul 3, 2019 at 1:41 AM Arnd Bergmann <[email protected]> wrote:
> >
> > On Wed, Jul 3, 2019 at 2:21 AM Alistair Francis
> > <[email protected]> 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

2019-07-03 22:19:33

by Alistair Francis

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

On Wed, Jul 3, 2019 at 12:47 PM Arnd Bergmann <[email protected]> wrote:
>
> On Wed, Jul 3, 2019 at 8:45 PM Alistair Francis <[email protected]> wrote:
> >
> > On Wed, Jul 3, 2019 at 1:41 AM Arnd Bergmann <[email protected]> wrote:
> > >
> > > On Wed, Jul 3, 2019 at 2:21 AM Alistair Francis
> > > <[email protected]> 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

2019-07-04 08:35:48

by Arnd Bergmann

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

On Thu, Jul 4, 2019 at 12:18 AM Alistair Francis <[email protected]> wrote:
> On Wed, Jul 3, 2019 at 12:47 PM Arnd Bergmann <[email protected]> wrote:
> > On Wed, Jul 3, 2019 at 8:45 PM Alistair Francis <[email protected]> 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

2019-07-09 23:20:54

by Alistair Francis

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

On Thu, Jul 4, 2019 at 1:34 AM Arnd Bergmann <[email protected]> wrote:
>
> On Thu, Jul 4, 2019 at 12:18 AM Alistair Francis <[email protected]> wrote:
> > On Wed, Jul 3, 2019 at 12:47 PM Arnd Bergmann <[email protected]> wrote:
> > > On Wed, Jul 3, 2019 at 8:45 PM Alistair Francis <[email protected]> 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