broken __kernel_mode_t affecting some big endian archs

broken __kernel_mode_t affecting some big endian archs

[Rich Felker]

It's been semi-known for a long time (I say semi-, because nobody's
had the setup to test most of them well, or at least nobody I'm
communicating with regularly) that some archs are failing libc-test
sysvipc tests. I think I've tracked down the root cause.

Linux defined __kernel_mode_t as short on some old archs, and used it
in place of mode_t in the ipc_perm structure. The field is padded out
to 32 bits, so on little endian archs it's no problem for us to just
(as we do) ignore the incorrect type and declare the structure with
mode_t, as POSIX requires. However on big-endian archs, the padding is
on the wrong side and this trick doesn't work.

On MIPS we fixed a similar issue in struct stat, where dev_t was
incorrectly padded, with a fixup in syscall_arch.h. However this time
a large number of archs are affected, and patching them all up
individually seems nasty.

My leaning is to have syscall_arch.h expose a macro indicating the
bug, and have msgctl, semctl, and shmctl each do the fixup if it's
set.

FWIW the affected archs seem to be (only in big endian variants):
- ARM
- M68k (in-progress port)
- Microblaze
- SH
- Sparc (future port)

Thoughts?

Rich

Re: broken __kernel_mode_t affecting some big endian archs

[Natanael Copa]

On Wed, 13 Jun 2018 20:54:42 -0400
Rich Felker <dalias@libc.org> wrote:

> It's been semi-known for a long time (I say semi-, because nobody's
> had the setup to test most of them well, or at least nobody I'm
> communicating with regularly) that some archs are failing libc-test
> sysvipc tests. I think I've tracked down the root cause.
> 
> Linux defined __kernel_mode_t as short on some old archs, and used it
> in place of mode_t in the ipc_perm structure. The field is padded out
> to 32 bits, so on little endian archs it's no problem for us to just
> (as we do) ignore the incorrect type and declare the structure with
> mode_t, as POSIX requires. However on big-endian archs, the padding is
> on the wrong side and this trick doesn't work.
> 
> On MIPS we fixed a similar issue in struct stat, where dev_t was
> incorrectly padded, with a fixup in syscall_arch.h. However this time
> a large number of archs are affected, and patching them all up
> individually seems nasty.
> 
> My leaning is to have syscall_arch.h expose a macro indicating the
> bug, and have msgctl, semctl, and shmctl each do the fixup if it's
> set.
> 
> FWIW the affected archs seem to be (only in big endian variants):
> - ARM
> - M68k (in-progress port)
> - Microblaze
> - SH
> - Sparc (future port)

Is s390s affected too?

-nc
 
> Thoughts?
> 
> Rich

Re: broken __kernel_mode_t affecting some big endian archs

[Rich Felker]

On Thu, Jun 14, 2018 at 01:19:31PM +0200, Natanael Copa wrote:
> On Wed, 13 Jun 2018 20:54:42 -0400
> Rich Felker <dalias@libc.org> wrote:
> 
> > It's been semi-known for a long time (I say semi-, because nobody's
> > had the setup to test most of them well, or at least nobody I'm
> > communicating with regularly) that some archs are failing libc-test
> > sysvipc tests. I think I've tracked down the root cause.
> > 
> > Linux defined __kernel_mode_t as short on some old archs, and used it
> > in place of mode_t in the ipc_perm structure. The field is padded out
> > to 32 bits, so on little endian archs it's no problem for us to just
> > (as we do) ignore the incorrect type and declare the structure with
> > mode_t, as POSIX requires. However on big-endian archs, the padding is
> > on the wrong side and this trick doesn't work.
> > 
> > On MIPS we fixed a similar issue in struct stat, where dev_t was
> > incorrectly padded, with a fixup in syscall_arch.h. However this time
> > a large number of archs are affected, and patching them all up
> > individually seems nasty.
> > 
> > My leaning is to have syscall_arch.h expose a macro indicating the
> > bug, and have msgctl, semctl, and shmctl each do the fixup if it's
> > set.
> > 
> > FWIW the affected archs seem to be (only in big endian variants):
> > - ARM
> > - M68k (in-progress port)
> > - Microblaze
> > - SH
> > - Sparc (future port)
> 
> Is s390s affected too?

Presumably you mean s390x, and it's not affected -- the bad (short)
definition of __kernel_mode_t is contingent on 32-bit s390 in the s390
version of asm/posix_types.h. This seems to be a pattern across
several archs with 32/64 bit variants, and there doesn't seem to be
any explanation for why it should be except possibly that, when adding
the 64-bit variant, somebody noticed the awful legacy __kernel_mode_t
definition and thought to fix it...

Rich

Re: broken __kernel_mode_t affecting some big endian archs

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 1396 bytes --]

On Wed, Jun 13, 2018 at 08:54:42PM -0400, Rich Felker wrote:
> It's been semi-known for a long time (I say semi-, because nobody's
> had the setup to test most of them well, or at least nobody I'm
> communicating with regularly) that some archs are failing libc-test
> sysvipc tests. I think I've tracked down the root cause.
> 
> Linux defined __kernel_mode_t as short on some old archs, and used it
> in place of mode_t in the ipc_perm structure. The field is padded out
> to 32 bits, so on little endian archs it's no problem for us to just
> (as we do) ignore the incorrect type and declare the structure with
> mode_t, as POSIX requires. However on big-endian archs, the padding is
> on the wrong side and this trick doesn't work.
> 
> On MIPS we fixed a similar issue in struct stat, where dev_t was
> incorrectly padded, with a fixup in syscall_arch.h. However this time
> a large number of archs are affected, and patching them all up
> individually seems nasty.
> 
> My leaning is to have syscall_arch.h expose a macro indicating the
> bug, and have msgctl, semctl, and shmctl each do the fixup if it's
> set.
> 
> FWIW the affected archs seem to be (only in big endian variants):
> - ARM
> - M68k (in-progress port)
> - Microblaze
> - SH
> - Sparc (future port)
> 
> Thoughts?

Here's a draft of the fix, just for shmctl and not including the
SHM_STAT or SHM_STAT_ALL operations.

Rich

[-- Attachment #2: shmfix.diff --]
[-- Type: text/plain, Size: 791 bytes --]

diff --git a/src/ipc/shmctl.c b/src/ipc/shmctl.c
index e2879f2..3659807 100644
--- a/src/ipc/shmctl.c
+++ b/src/ipc/shmctl.c
@@ -4,9 +4,24 @@
 
 int shmctl(int id, int cmd, struct shmid_ds *buf)
 {
+#ifdef SYSCALL_IPC_BROKEN_MODE
+	struct shmid_ds tmp;
+	if (cmd == IPC_SET) {
+		tmp = *buf;
+		tmp.shm_perm.mode *= 0x10000U;
+		buf = &tmp;
+	}
+#endif
 #ifdef SYS_shmctl
-	return syscall(SYS_shmctl, id, cmd | IPC_64, buf);
+	int r = __syscall(SYS_shmctl, id, cmd | IPC_64, buf);
 #else
-	return syscall(SYS_ipc, IPCOP_shmctl, id, cmd | IPC_64, 0, buf, 0);
+	int r = __syscall(SYS_ipc, IPCOP_shmctl, id, cmd | IPC_64, 0, buf, 0);
+#endif
+#ifdef SYSCALL_IPC_BROKEN_MODE
+	if (r < 0) return __syscall_ret(r);
+	if (cmd == IPC_STAT) {
+		buf->shm_perm.mode >>= 16;
+	}
+	return r;
 #endif
 }

Re: broken __kernel_mode_t affecting some big endian archs

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 1594 bytes --]

On Thu, Jun 14, 2018 at 10:00:23PM -0400, Rich Felker wrote:
> On Wed, Jun 13, 2018 at 08:54:42PM -0400, Rich Felker wrote:
> > It's been semi-known for a long time (I say semi-, because nobody's
> > had the setup to test most of them well, or at least nobody I'm
> > communicating with regularly) that some archs are failing libc-test
> > sysvipc tests. I think I've tracked down the root cause.
> > 
> > Linux defined __kernel_mode_t as short on some old archs, and used it
> > in place of mode_t in the ipc_perm structure. The field is padded out
> > to 32 bits, so on little endian archs it's no problem for us to just
> > (as we do) ignore the incorrect type and declare the structure with
> > mode_t, as POSIX requires. However on big-endian archs, the padding is
> > on the wrong side and this trick doesn't work.
> > 
> > On MIPS we fixed a similar issue in struct stat, where dev_t was
> > incorrectly padded, with a fixup in syscall_arch.h. However this time
> > a large number of archs are affected, and patching them all up
> > individually seems nasty.
> > 
> > My leaning is to have syscall_arch.h expose a macro indicating the
> > bug, and have msgctl, semctl, and shmctl each do the fixup if it's
> > set.
> > 
> > FWIW the affected archs seem to be (only in big endian variants):
> > - ARM
> > - M68k (in-progress port)
> > - Microblaze
> > - SH
> > - Sparc (future port)
> > 
> > Thoughts?
> 
> Here's a draft of the fix, just for shmctl and not including the
> SHM_STAT or SHM_STAT_ALL operations.

And again with a fix so it doesn't break build on non-broken archs...

Rich

[-- Attachment #2: shmfix.diff --]
[-- Type: text/plain, Size: 778 bytes --]

diff --git a/src/ipc/shmctl.c b/src/ipc/shmctl.c
index e2879f2..9d723d9 100644
--- a/src/ipc/shmctl.c
+++ b/src/ipc/shmctl.c
@@ -4,9 +4,23 @@
 
 int shmctl(int id, int cmd, struct shmid_ds *buf)
 {
+#ifdef SYSCALL_IPC_BROKEN_MODE
+	struct shmid_ds tmp;
+	if (cmd == IPC_SET) {
+		tmp = *buf;
+		tmp.shm_perm.mode *= 0x10000U;
+		buf = &tmp;
+	}
+#endif
 #ifdef SYS_shmctl
-	return syscall(SYS_shmctl, id, cmd | IPC_64, buf);
+	int r = __syscall(SYS_shmctl, id, cmd | IPC_64, buf);
 #else
-	return syscall(SYS_ipc, IPCOP_shmctl, id, cmd | IPC_64, 0, buf, 0);
+	int r = __syscall(SYS_ipc, IPCOP_shmctl, id, cmd | IPC_64, 0, buf, 0);
+#endif
+#ifdef SYSCALL_IPC_BROKEN_MODE
+	if (r >= 0 && cmd == IPC_STAT) {
+		buf->shm_perm.mode >>= 16;
+	}
 #endif
+	return __syscall_ret(r);
 }

Re: broken __kernel_mode_t affecting some big endian archs

[Szabolcs Nagy]

* Rich Felker <dalias@libc.org> [2018-06-14 22:01:54 -0400]:
> diff --git a/src/ipc/shmctl.c b/src/ipc/shmctl.c
> index e2879f2..9d723d9 100644
> --- a/src/ipc/shmctl.c
> +++ b/src/ipc/shmctl.c
> @@ -4,9 +4,23 @@
>  
>  int shmctl(int id, int cmd, struct shmid_ds *buf)
>  {
> +#ifdef SYSCALL_IPC_BROKEN_MODE
> +	struct shmid_ds tmp;
> +	if (cmd == IPC_SET) {
> +		tmp = *buf;
> +		tmp.shm_perm.mode *= 0x10000U;
> +		buf = &tmp;
> +	}
> +#endif
>  #ifdef SYS_shmctl
> -	return syscall(SYS_shmctl, id, cmd | IPC_64, buf);
> +	int r = __syscall(SYS_shmctl, id, cmd | IPC_64, buf);
>  #else
> -	return syscall(SYS_ipc, IPCOP_shmctl, id, cmd | IPC_64, 0, buf, 0);
> +	int r = __syscall(SYS_ipc, IPCOP_shmctl, id, cmd | IPC_64, 0, buf, 0);
> +#endif
> +#ifdef SYSCALL_IPC_BROKEN_MODE
> +	if (r >= 0 && cmd == IPC_STAT) {
> +		buf->shm_perm.mode >>= 16;
> +	}
>  #endif
> +	return __syscall_ret(r);
>  }

ugly but looks ok to me.
i don't think there is a clean solution on the broken targets.