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
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
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
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
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
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
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