2012-05-17 22:14:01

by H.J. Lu

[permalink] [raw]
Subject: [RFC PATCH 00/10] Use __kernel_[u]long_t for x32 user space compatibility

From: H.J. Lu <[email protected]>

This patch set changes a number of places where the kernel headers are
exported to user space and currently use explicit "long" or "unsigned
long" to use __kernel_[u]long_t in order to be compatible with the x32
user space ABI. These location are places where x32 uses the x86-64
ABI.

It is quite possible that some, or even all, of these locations should
really use dedicated types, but in the meantime this gives the correct
results which the current headers do not.


2012-05-17 22:14:12

by H.J. Lu

[permalink] [raw]
Subject: [PATCH 04/10] Use __kernel_long_t in struct msgbuf

From: "H.J. Lu" <[email protected]>

Replace long with __kernel_long_t in struct msgbuf for user space.

Signed-off-by: H.J. Lu <[email protected]>
---
include/linux/msg.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/msg.h b/include/linux/msg.h
index 56abf15..c5d21c1 100644
--- a/include/linux/msg.h
+++ b/include/linux/msg.h
@@ -33,8 +33,8 @@ struct msqid_ds {

/* message buffer for msgsnd and msgrcv calls */
struct msgbuf {
- long mtype; /* type of message */
- char mtext[1]; /* message text */
+ __kernel_long_t mtype; /* type of message */
+ char mtext[1]; /* message text */
};

/* buffer for msgctl calls IPC_INFO, MSG_INFO */
--
1.7.6.5

2012-05-17 22:14:09

by H.J. Lu

[permalink] [raw]
Subject: [PATCH 05/10] Use __kernel_long_t in struct mq_attr

From: "H.J. Lu" <[email protected]>

Replace long with __kernel_long_t in struct mq_attr for user space.

Signed-off-by: H.J. Lu <[email protected]>
---
include/linux/mqueue.h | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/mqueue.h b/include/linux/mqueue.h
index 8b5a796..d89a735 100644
--- a/include/linux/mqueue.h
+++ b/include/linux/mqueue.h
@@ -23,11 +23,11 @@
#define MQ_BYTES_MAX 819200

struct mq_attr {
- long mq_flags; /* message queue flags */
- long mq_maxmsg; /* maximum number of messages */
- long mq_msgsize; /* maximum message size */
- long mq_curmsgs; /* number of messages currently queued */
- long __reserved[4]; /* ignored for input, zeroed for output */
+ __kernel_long_t mq_flags; /* message queue flags */
+ __kernel_long_t mq_maxmsg; /* maximum number of messages */
+ __kernel_long_t mq_msgsize; /* maximum message size */
+ __kernel_long_t mq_curmsgs; /* number of messages currently queued */
+ __kernel_long_t __reserved[4]; /* ignored for input, zeroed for output */
};

/*
--
1.7.6.5

2012-05-17 22:14:08

by H.J. Lu

[permalink] [raw]
Subject: [PATCH 03/10] Use __kernel_[u]long_t in linux/resource.h

From: "H.J. Lu" <[email protected]>

Replace long with __kernel_long_t in struct rusage and replace
unsigned long with __kernel_ulong_t in struct rlimit for usr space.

Signed-off-by: H.J. Lu <[email protected]>
---
include/linux/resource.h | 32 ++++++++++++++++----------------
1 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/include/linux/resource.h b/include/linux/resource.h
index d01c96c..38bbb6a 100644
--- a/include/linux/resource.h
+++ b/include/linux/resource.h
@@ -23,25 +23,25 @@
struct rusage {
struct timeval ru_utime; /* user time used */
struct timeval ru_stime; /* system time used */
- long ru_maxrss; /* maximum resident set size */
- long ru_ixrss; /* integral shared memory size */
- long ru_idrss; /* integral unshared data size */
- long ru_isrss; /* integral unshared stack size */
- long ru_minflt; /* page reclaims */
- long ru_majflt; /* page faults */
- long ru_nswap; /* swaps */
- long ru_inblock; /* block input operations */
- long ru_oublock; /* block output operations */
- long ru_msgsnd; /* messages sent */
- long ru_msgrcv; /* messages received */
- long ru_nsignals; /* signals received */
- long ru_nvcsw; /* voluntary context switches */
- long ru_nivcsw; /* involuntary " */
+ __kernel_long_t ru_maxrss; /* maximum resident set size */
+ __kernel_long_t ru_ixrss; /* integral shared memory size */
+ __kernel_long_t ru_idrss; /* integral unshared data size */
+ __kernel_long_t ru_isrss; /* integral unshared stack size */
+ __kernel_long_t ru_minflt; /* page reclaims */
+ __kernel_long_t ru_majflt; /* page faults */
+ __kernel_long_t ru_nswap; /* swaps */
+ __kernel_long_t ru_inblock; /* block input operations */
+ __kernel_long_t ru_oublock; /* block output operations */
+ __kernel_long_t ru_msgsnd; /* messages sent */
+ __kernel_long_t ru_msgrcv; /* messages received */
+ __kernel_long_t ru_nsignals; /* signals received */
+ __kernel_long_t ru_nvcsw; /* voluntary context switches */
+ __kernel_long_t ru_nivcsw; /* involuntary " */
};

struct rlimit {
- unsigned long rlim_cur;
- unsigned long rlim_max;
+ __kernel_ulong_t rlim_cur;
+ __kernel_ulong_t rlim_max;
};

#define RLIM64_INFINITY (~0ULL)
--
1.7.6.5

2012-05-17 22:14:06

by H.J. Lu

[permalink] [raw]
Subject: [PATCH 02/10] Use __kernel_ulong_t in struct shm_info

From: "H.J. Lu" <[email protected]>

Replace unsigned long with __kernel_ulong_t in struct shm_info for
user space.

Signed-off-by: H.J. Lu <[email protected]>
---
include/linux/shm.h | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/shm.h b/include/linux/shm.h
index 92808b8..6cd9510 100644
--- a/include/linux/shm.h
+++ b/include/linux/shm.h
@@ -75,11 +75,11 @@ struct shminfo {

struct shm_info {
int used_ids;
- unsigned long shm_tot; /* total allocated shm */
- unsigned long shm_rss; /* total resident shm */
- unsigned long shm_swp; /* total swapped shm */
- unsigned long swap_attempts;
- unsigned long swap_successes;
+ __kernel_ulong_t shm_tot; /* total allocated shm */
+ __kernel_ulong_t shm_rss; /* total resident shm */
+ __kernel_ulong_t shm_swp; /* total swapped shm */
+ __kernel_ulong_t swap_attempts;
+ __kernel_ulong_t swap_successes;
};

#ifdef __KERNEL__
--
1.7.6.5

2012-05-17 22:15:06

by H.J. Lu

[permalink] [raw]
Subject: [PATCH 09/10] Use __kernel_ulong_t in struct ipc64_perm

From: "H.J. Lu" <[email protected]>

Replace unsigned long with __kernel_ulong_t in struct ipc64_perm for
user space.

Signed-off-by: H.J. Lu <[email protected]>
---
include/asm-generic/ipcbuf.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/ipcbuf.h b/include/asm-generic/ipcbuf.h
index 76982b2..3dbcc1e 100644
--- a/include/asm-generic/ipcbuf.h
+++ b/include/asm-generic/ipcbuf.h
@@ -27,8 +27,8 @@ struct ipc64_perm {
unsigned char __pad1[4 - sizeof(__kernel_mode_t)];
unsigned short seq;
unsigned short __pad2;
- unsigned long __unused1;
- unsigned long __unused2;
+ __kernel_ulong_t __unused1;
+ __kernel_ulong_t __unused2;
};

#endif /* __ASM_GENERIC_IPCBUF_H */
--
1.7.6.5

2012-05-17 22:15:03

by H.J. Lu

[permalink] [raw]
Subject: [PATCH 10/10] Use __kernel_[u]long_t in x86-64 struct stat

From: "H.J. Lu" <[email protected]>

Include <linux/types.h>. Replace unsigned long/long with
__kernel_ulong_t/__kernel_long_t in x86-64 struct stat for user space.

Signed-off-by: H.J. Lu <[email protected]>
---
arch/x86/include/asm/stat.h | 42 ++++++++++++++++++++++--------------------
1 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/arch/x86/include/asm/stat.h b/arch/x86/include/asm/stat.h
index e0b1d9b..6d99f66 100644
--- a/arch/x86/include/asm/stat.h
+++ b/arch/x86/include/asm/stat.h
@@ -3,6 +3,8 @@

#define STAT_HAVE_NSEC 1

+#include <linux/types.h>
+
#ifdef __i386__
struct stat {
unsigned long st_dev;
@@ -66,26 +68,26 @@ struct stat64 {
#else /* __i386__ */

struct stat {
- unsigned long st_dev;
- unsigned long st_ino;
- unsigned long st_nlink;
-
- unsigned int st_mode;
- unsigned int st_uid;
- unsigned int st_gid;
- unsigned int __pad0;
- unsigned long st_rdev;
- long st_size;
- long st_blksize;
- long st_blocks; /* Number 512-byte blocks allocated. */
-
- unsigned long st_atime;
- unsigned long st_atime_nsec;
- unsigned long st_mtime;
- unsigned long st_mtime_nsec;
- unsigned long st_ctime;
- unsigned long st_ctime_nsec;
- long __unused[3];
+ __kernel_ulong_t st_dev;
+ __kernel_ulong_t st_ino;
+ __kernel_ulong_t st_nlink;
+
+ unsigned int st_mode;
+ unsigned int st_uid;
+ unsigned int st_gid;
+ unsigned int __pad0;
+ __kernel_ulong_t st_rdev;
+ __kernel_long_t st_size;
+ __kernel_long_t st_blksize;
+ __kernel_long_t st_blocks; /* Number 512-byte blocks allocated. */
+
+ __kernel_ulong_t st_atime;
+ __kernel_ulong_t st_atime_nsec;
+ __kernel_ulong_t st_mtime;
+ __kernel_ulong_t st_mtime_nsec;
+ __kernel_ulong_t st_ctime;
+ __kernel_ulong_t st_ctime_nsec;
+ __kernel_long_t __unused[3];
};
#endif

--
1.7.6.5

2012-05-17 22:15:58

by H.J. Lu

[permalink] [raw]
Subject: [PATCH 07/10] Use __kernel_ulong_t in struct shmid64_ds/shminfo64

From: "H.J. Lu" <[email protected]>

Replace unsigned long with __kernel_ulong_t in struct shmid64_ds and
struct shminfo64 for user space.

Signed-off-by: H.J. Lu <[email protected]>
---
include/asm-generic/shmbuf.h | 24 ++++++++++++------------
1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/asm-generic/shmbuf.h b/include/asm-generic/shmbuf.h
index 5768fa6..7e9fb2f 100644
--- a/include/asm-generic/shmbuf.h
+++ b/include/asm-generic/shmbuf.h
@@ -39,21 +39,21 @@ struct shmid64_ds {
#endif
__kernel_pid_t shm_cpid; /* pid of creator */
__kernel_pid_t shm_lpid; /* pid of last operator */
- unsigned long shm_nattch; /* no. of current attaches */
- unsigned long __unused4;
- unsigned long __unused5;
+ __kernel_ulong_t shm_nattch; /* no. of current attaches */
+ __kernel_ulong_t __unused4;
+ __kernel_ulong_t __unused5;
};

struct shminfo64 {
- unsigned long shmmax;
- unsigned long shmmin;
- unsigned long shmmni;
- unsigned long shmseg;
- unsigned long shmall;
- unsigned long __unused1;
- unsigned long __unused2;
- unsigned long __unused3;
- unsigned long __unused4;
+ __kernel_ulong_t shmmax;
+ __kernel_ulong_t shmmin;
+ __kernel_ulong_t shmmni;
+ __kernel_ulong_t shmseg;
+ __kernel_ulong_t shmall;
+ __kernel_ulong_t __unused1;
+ __kernel_ulong_t __unused2;
+ __kernel_ulong_t __unused3;
+ __kernel_ulong_t __unused4;
};

#endif /* __ASM_GENERIC_SHMBUF_H */
--
1.7.6.5

2012-05-17 22:15:57

by H.J. Lu

[permalink] [raw]
Subject: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

From: "H.J. Lu" <[email protected]>

Replace unsigned long with __kernel_ulong_t in struct msqid64_ds for
user space. Don't change unsigned long when __BITS_PER_LONG != 64
since __kernel_ulong_t == unsigned long in this case.

Signed-off-by: H.J. Lu <[email protected]>
---
include/asm-generic/msgbuf.h | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/asm-generic/msgbuf.h b/include/asm-generic/msgbuf.h
index aec850d..f55ecc4 100644
--- a/include/asm-generic/msgbuf.h
+++ b/include/asm-generic/msgbuf.h
@@ -35,13 +35,13 @@ struct msqid64_ds {
#if __BITS_PER_LONG != 64
unsigned long __unused3;
#endif
- unsigned long msg_cbytes; /* current number of bytes on queue */
- unsigned long msg_qnum; /* number of messages in queue */
- unsigned long msg_qbytes; /* max number of bytes on queue */
+ __kernel_ulong_t msg_cbytes; /* current number of bytes on queue */
+ __kernel_ulong_t msg_qnum; /* number of messages in queue */
+ __kernel_ulong_t msg_qbytes; /* max number of bytes on queue */
__kernel_pid_t msg_lspid; /* pid of last msgsnd */
__kernel_pid_t msg_lrpid; /* last receive pid */
- unsigned long __unused4;
- unsigned long __unused5;
+ __kernel_ulong_t __unused4;
+ __kernel_ulong_t __unused5;
};

#endif /* __ASM_GENERIC_MSGBUF_H */
--
1.7.6.5

2012-05-17 22:16:36

by H.J. Lu

[permalink] [raw]
Subject: [PATCH 01/10] Use __kernel_long_t in struct timex

From: "H.J. Lu" <[email protected]>

Replace long with __kernel_long_t in struct timex for user space.

Signed-off-by: H.J. Lu <[email protected]>
---
include/linux/timex.h | 34 +++++++++++++++++-----------------
1 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/include/linux/timex.h b/include/linux/timex.h
index 99bc88b..22c57ac 100644
--- a/include/linux/timex.h
+++ b/include/linux/timex.h
@@ -63,27 +63,27 @@
*/
struct timex {
unsigned int modes; /* mode selector */
- long offset; /* time offset (usec) */
- long freq; /* frequency offset (scaled ppm) */
- long maxerror; /* maximum error (usec) */
- long esterror; /* estimated error (usec) */
+ __kernel_long_t offset; /* time offset (usec) */
+ __kernel_long_t freq; /* frequency offset (scaled ppm) */
+ __kernel_long_t maxerror; /* maximum error (usec) */
+ __kernel_long_t esterror; /* estimated error (usec) */
int status; /* clock command/status */
- long constant; /* pll time constant */
- long precision; /* clock precision (usec) (read only) */
- long tolerance; /* clock frequency tolerance (ppm)
- * (read only)
- */
+ __kernel_long_t constant; /* pll time constant */
+ __kernel_long_t precision; /* clock precision (usec) (read only) */
+ __kernel_long_t tolerance; /* clock frequency tolerance (ppm)
+ * (read only)
+ */
struct timeval time; /* (read only, except for ADJ_SETOFFSET) */
- long tick; /* (modified) usecs between clock ticks */
+ __kernel_long_t tick; /* (modified) usecs between clock ticks */

- long ppsfreq; /* pps frequency (scaled ppm) (ro) */
- long jitter; /* pps jitter (us) (ro) */
+ __kernel_long_t ppsfreq; /* pps frequency (scaled ppm) (ro) */
+ __kernel_long_t jitter; /* pps jitter (us) (ro) */
int shift; /* interval duration (s) (shift) (ro) */
- long stabil; /* pps stability (scaled ppm) (ro) */
- long jitcnt; /* jitter limit exceeded (ro) */
- long calcnt; /* calibration intervals (ro) */
- long errcnt; /* calibration errors (ro) */
- long stbcnt; /* stability limit exceeded (ro) */
+ __kernel_long_t stabil; /* pps stability (scaled ppm) (ro) */
+ __kernel_long_t jitcnt; /* jitter limit exceeded (ro) */
+ __kernel_long_t calcnt; /* calibration intervals (ro) */
+ __kernel_long_t errcnt; /* calibration errors (ro) */
+ __kernel_long_t stbcnt; /* stability limit exceeded (ro) */

int tai; /* TAI offset (ro) */

--
1.7.6.5

2012-05-17 22:16:34

by H.J. Lu

[permalink] [raw]
Subject: [PATCH 06/10] Use __kernel_ulong_t in x86 struct semid64_ds

From: "H.J. Lu" <[email protected]>

Replace unsigned long with __kernel_ulong_t in x86 struct semid64_ds
for user space.

Signed-off-by: H.J. Lu <[email protected]>
---
arch/x86/include/asm/sembuf.h | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/sembuf.h b/arch/x86/include/asm/sembuf.h
index ee50c80..cc2d6a3 100644
--- a/arch/x86/include/asm/sembuf.h
+++ b/arch/x86/include/asm/sembuf.h
@@ -13,12 +13,12 @@
struct semid64_ds {
struct ipc64_perm sem_perm; /* permissions .. see ipc.h */
__kernel_time_t sem_otime; /* last semop time */
- unsigned long __unused1;
+ __kernel_ulong_t __unused1;
__kernel_time_t sem_ctime; /* last change time */
- unsigned long __unused2;
- unsigned long sem_nsems; /* no. of semaphores in array */
- unsigned long __unused3;
- unsigned long __unused4;
+ __kernel_ulong_t __unused2;
+ __kernel_ulong_t sem_nsems; /* no. of semaphores in array */
+ __kernel_ulong_t __unused3;
+ __kernel_ulong_t __unused4;
};

#endif /* _ASM_X86_SEMBUF_H */
--
1.7.6.5

2012-05-17 22:32:24

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH 01/10] Use __kernel_long_t in struct timex

On Thu, May 17, 2012 at 3:13 PM, H.J. Lu <[email protected]> wrote:
>
> Replace long with __kernel_long_t in struct timex for user space.

I absolutely detest these types.

I realize that we already have a few users, but just looking at these
diffs *hurts*. It's disgusting.

The whole __kernel_ prefix was a mistake, but it at least makes sense
for certain things where it is really about some random kernel choice
(ie __kernel_pid_t). Even there I despise it, because it's not really
about "kernel choice", it's about just the real native type for uid_t
- the fact that user-mode then occasionally screwed up because glibc
has chosen crazy extended types is really not a "kernel" issue at all.
So the naming in general is painful.

When it comes to the x32 thing I think it's *doubly* wrong, because
this isn't even about a "kernel choice". It's damn well the native
machine word-size. The fact that a limited user-mode ABI then limits
"long" to 32-bit is not the kernels problem.

So I'd really like to see some discussion about naming. What does this
have to do with "kernel"? Nothing. It's the native word-size of the
machine, for crying out loud. The fact that some user interfaces may
limit themselves is not a "user mode vs kernel" thing.

Linus

2012-05-17 22:42:12

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 01/10] Use __kernel_long_t in struct timex

On 05/17/2012 03:32 PM, Linus Torvalds wrote:
>
> The whole __kernel_ prefix was a mistake, but it at least makes sense
> for certain things where it is really about some random kernel choice
> (ie __kernel_pid_t). Even there I despise it, because it's not really
> about "kernel choice", it's about just the real native type for uid_t
> - the fact that user-mode then occasionally screwed up because glibc
> has chosen crazy extended types is really not a "kernel" issue at all.
> So the naming in general is painful.
>
> When it comes to the x32 thing I think it's *doubly* wrong, because
> this isn't even about a "kernel choice". It's damn well the native
> machine word-size. The fact that a limited user-mode ABI then limits
> "long" to 32-bit is not the kernels problem.
>
> So I'd really like to see some discussion about naming. What does this
> have to do with "kernel"? Nothing. It's the native word-size of the
> machine, for crying out loud. The fact that some user interfaces may
> limit themselves is not a "user mode vs kernel" thing.

It also puts a clear line between the kernel and user space namespaces,
which has been an ongoing problem (we *still* haven't cleaned out some
namespace pollution in the i386 <asm/signal.h> for example.)

That being said, this is a lot like the __u* and __s* types which we use
instead of <stdint.h> for similar reasons. I don't know if
__ulong/__slong or __uword/__sword would be better here?

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-05-17 22:50:28

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 01/10] Use __kernel_long_t in struct timex

On 05/17/2012 03:41 PM, H. Peter Anvin wrote:
>
> That being said, this is a lot like the __u* and __s* types which we use
> instead of <stdint.h> for similar reasons. I don't know if
> __ulong/__slong or __uword/__sword would be better here?
>

Anyway - Linus, if you have a specific preference let me know and can
fix up these patches as well as the existing users in the kernel.

-hpa

2012-05-17 22:51:18

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH 01/10] Use __kernel_long_t in struct timex

On Thu, May 17, 2012 at 3:41 PM, H. Peter Anvin <[email protected]> wrote:
>
> It also puts a clear line between the kernel and user space namespaces,
> which has been an ongoing problem (we *still* haven't cleaned out some
> namespace pollution in the i386 <asm/signal.h> for example.)
>
> That being said, this is a lot like the __u* and __s* types which we use
> instead of <stdint.h> for similar reasons. ?I don't know if
> __ulong/__slong or __uword/__sword would be better here?

Yes, I do think this is closer to the "__u32" kind of usage, and in
general I tend to think that's true of most of the __kernel_ prefix
things. There is very little "kernely" things about it.

Yes, we have to have the double underscore (or single+capitalized),
but I think that at least personally, I'd be happier with just
"__long" and "__ulong".

I think __word would be good too, *except* for the fact that
especially in x86 land, I think there's the legacy confusion with
"word" being 16-bit. Ugh.

So I don't know. I just do know that I don't see the point in that
"__kernel_" prefix.

Linus

2012-05-17 22:55:31

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 01/10] Use __kernel_long_t in struct timex

On 05/17/2012 03:50 PM, Linus Torvalds wrote:
> Yes, I do think this is closer to the "__u32" kind of usage, and in
> general I tend to think that's true of most of the __kernel_ prefix
> things. There is very little "kernely" things about it.

The only think "kernely" about it is that it describes the kernel ABI.

> Yes, we have to have the double underscore (or single+capitalized),
> but I think that at least personally, I'd be happier with just
> "__long" and "__ulong".

I would suggest __slong and __ulong then, to keep with the __[su]*
namespace, or does the extra "s" look too much like crap?

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-05-17 22:56:44

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH 01/10] Use __kernel_long_t in struct timex

On Thu, May 17, 2012 at 3:50 PM, Linus Torvalds
<[email protected]> wrote:
>
> I think __word would be good too, *except* for the fact that
> especially in x86 land, I think there's the legacy confusion with
> "word" being 16-bit. Ugh.

Looking at the x32 case, I have to say that "long" in general looks
horrible. Especially when we have things like

typedef long long __kernel_long_t;

(and __long really wouldn't look any nicer). Any sane person would go
"Eww" at looking at that - we're using 'long long' to typedef a type
that is named 'long'.

It would make much more sense to use "__word" for reasons like that.
But I really don't think that works well in a x86 context.

Other ideas? Maybe "__wordsize" would be less associated with x86 16-bit words?

Linus

2012-05-17 22:57:54

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 01/10] Use __kernel_long_t in struct timex

On 05/17/2012 03:56 PM, Linus Torvalds wrote:
>
> It would make much more sense to use "__word" for reasons like that.
> But I really don't think that works well in a x86 context.
>
> Other ideas? Maybe "__wordsize" would be less associated with x86 16-bit words?
>

__[su]native maybe?

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-05-17 22:58:50

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH 01/10] Use __kernel_long_t in struct timex

On Thu, May 17, 2012 at 3:55 PM, H. Peter Anvin <[email protected]> wrote:
>
> I would suggest __slong and __ulong then, to keep with the __[su]*
> namespace, or does the extra "s" look too much like crap?

I do think I could live with it. Maybe we can make x32 do

typedef __s64 __slong;
typedef __u64 __ulong;

and at least not make the "we're declaring a 'long' as a 'long long'"?

Linus

2012-05-17 23:07:50

by David Daney

[permalink] [raw]
Subject: Re: [RFC PATCH 00/10] Use __kernel_[u]long_t for x32 user space compatibility

On 05/17/2012 03:13 PM, H.J. Lu wrote:
> From: H.J. Lu<[email protected]>
>
> This patch set changes a number of places where the kernel headers are
> exported to user space and currently use explicit "long" or "unsigned
> long" to use __kernel_[u]long_t in order to be compatible with the x32
> user space ABI. These location are places where x32 uses the x86-64
> ABI.
>

Has anybody checked how this affects MIPS n32 userspace?

I think it totally breaks it.

In addition, 109a1f32 (sysinfo: Use explicit types in <linux/sysinfo.h>)
is probably bad. I think it may need to be reverted, or somebody should
fix all the __kernel_{,u}long_t definitions for the ABI that may have
been broken by the change.

> It is quite possible that some, or even all, of these locations should
> really use dedicated types, but in the meantime this gives the correct
> results which the current headers do not.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

2012-05-17 23:12:08

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [RFC PATCH 00/10] Use __kernel_[u]long_t for x32 user space compatibility

On 05/17/2012 04:07 PM, David Daney wrote:
>
> Has anybody checked how this affects MIPS n32 userspace?
>
> I think it totally breaks it.
>

Do you have any basis whatsoever for that statement? This should have
zero effect on any non-x32 platforms.

> In addition, 109a1f32 (sysinfo: Use explicit types in <linux/sysinfo.h>)
> is probably bad. I think it may need to be reverted, or somebody should
> fix all the __kernel_{,u}long_t definitions for the ABI that may have
> been broken by the change.

You realize __kernel_[u]long_t didn't even exist until the 3.4 kernel,
right?

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-05-17 23:26:13

by David Daney

[permalink] [raw]
Subject: Re: [RFC PATCH 00/10] Use __kernel_[u]long_t for x32 user space compatibility

On 05/17/2012 04:11 PM, H. Peter Anvin wrote:
> On 05/17/2012 04:07 PM, David Daney wrote:
>>
>> Has anybody checked how this affects MIPS n32 userspace?
>>
>> I think it totally breaks it.
>>
>
> Do you have any basis whatsoever for that statement?

You should have asked for a 'solid basis'.

My basis is that the name '__kernel_ulong_t' implies, in my mind, that
it would have the width of a kernel unsigned long.

Really it should be called something like __abi_alternate_ulong_t.

> This should have
> zero effect on any non-x32 platforms.

After further reflection on this, you are probably right.

Sorry for raising the alarm (or would that be crying Wolf?).

>
>> In addition, 109a1f32 (sysinfo: Use explicit types in<linux/sysinfo.h>)
>> is probably bad. I think it may need to be reverted, or somebody should
>> fix all the __kernel_{,u}long_t definitions for the ABI that may have
>> been broken by the change.
>
> You realize __kernel_[u]long_t didn't even exist until the 3.4 kernel,
> right?

Yes.

David Daney

2012-05-17 23:32:09

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [RFC PATCH 00/10] Use __kernel_[u]long_t for x32 user space compatibility

On 05/17/2012 04:25 PM, David Daney wrote:
>>
>> Do you have any basis whatsoever for that statement?
>
> You should have asked for a 'solid basis'.
>
> My basis is that the name '__kernel_ulong_t' implies, in my mind, that
> it would have the width of a kernel unsigned long.

The namespace __kernel_* doesn't mean "as used in the kernel" but rather
"exported by the kernel". But yes, see also Linus' criticism.

-hpa


--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-05-17 23:51:22

by David Daney

[permalink] [raw]
Subject: Re: [PATCH 01/10] Use __kernel_long_t in struct timex

On 05/17/2012 03:56 PM, Linus Torvalds wrote:
> On Thu, May 17, 2012 at 3:50 PM, Linus Torvalds
> <[email protected]> wrote:
>>
>> I think __word would be good too, *except* for the fact that
>> especially in x86 land, I think there's the legacy confusion with
>> "word" being 16-bit. Ugh.
>
> Looking at the x32 case, I have to say that "long" in general looks
> horrible. Especially when we have things like
>
> typedef long long __kernel_long_t;
>
> (and __long really wouldn't look any nicer). Any sane person would go
> "Eww" at looking at that - we're using 'long long' to typedef a type
> that is named 'long'.
>
> It would make much more sense to use "__word" for reasons like that.
> But I really don't think that works well in a x86 context.
>
> Other ideas? Maybe "__wordsize" would be less associated with x86 16-bit words?
>

FWIW: "__abi_wordsize" to indicate that it is not really a property of
the machine itself, but rather the ABI in use.

David Daney

2012-05-17 23:51:42

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On 05/17/2012 03:13 PM, H.J. Lu wrote:
> From: "H.J. Lu" <[email protected]>
>
> Replace unsigned long with __kernel_ulong_t in struct msqid64_ds for
> user space. Don't change unsigned long when __BITS_PER_LONG != 64
> since __kernel_ulong_t == unsigned long in this case.
>
> Signed-off-by: H.J. Lu <[email protected]>

This patch and the one before it seems to have another problem: we
currently define __BITS_PER_LONG as:

#ifdef __x86_64__
# define __BITS_PER_LONG 64
#else
# define __BITS_PER_LONG 32
#endif

... which means __BITS_PER_LONG == 64 on x86-64. This seems wrong. The
result would seem to be that the padding members around __kernel_time_t
in struct shmid64_ds (not visible in patch 07/10 but existing in the
same structure) work by accident (I also wonder how the heck they're
currently supposed to work on bigendian architectures!!)

However, whereas struct shmid64_ds seems to work okay, this patch would
now seem to be wrong.

The sane thing would seem to be to change __BITS_PER_LONG to 32 on x32
and fix the padding hacks in struct shmid64_ds; H.J., would you agree?

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-05-18 00:08:07

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Thu, May 17, 2012 at 4:51 PM, H. Peter Anvin <[email protected]> wrote:
>
> The sane thing would seem to be to change __BITS_PER_LONG to 32 on x32
> and fix the padding hacks in struct shmid64_ds; H.J., would you agree?

Ugh. That looks like a disaster.

The padding hacks that depend on __BITS_PER_LONG seem pretty damn broken anyway.

They only work if the kernel agrees with the value (which is against
the whole point of making __BITS_PER_LONG be about some user-level ABI
thing) or for little-endian machines.

IOW, all the __BITS_PER_LONG games look totally broken to me. I can't
see how they could possibly even be fixed.


Linus

2012-05-18 00:15:01

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On 05/17/2012 05:07 PM, Linus Torvalds wrote:
> On Thu, May 17, 2012 at 4:51 PM, H. Peter Anvin <[email protected]> wrote:
>>
>> The sane thing would seem to be to change __BITS_PER_LONG to 32 on x32
>> and fix the padding hacks in struct shmid64_ds; H.J., would you agree?
>
> Ugh. That looks like a disaster.
>
> The padding hacks that depend on __BITS_PER_LONG seem pretty damn broken anyway.
>
> They only work if the kernel agrees with the value (which is against
> the whole point of making __BITS_PER_LONG be about some user-level ABI
> thing) or for little-endian machines.
>
> IOW, all the __BITS_PER_LONG games look totally broken to me. I can't
> see how they could possibly even be fixed.
>

Well, on existing compat (e.g. i386) __BITS_PER_LONG is definitely not
the same as kernel. And yes, I don't see how the heck this was ever
correct on bigendian machines or even for compat in any form (if the
kernel tries to interpret the extra bits and user space didn't
initialize them we're lost.)

The "logical" thing to do here seems to just use __s64, but I have no
idea if that would suddenly break bigendian architectures...

David, Ralf, do you have any idea what e.g. MIPS does here?

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-05-18 00:19:00

by Mike Frysinger

[permalink] [raw]
Subject: Re: [RFC PATCH 00/10] Use __kernel_[u]long_t for x32 user space compatibility

On Thursday 17 May 2012 18:13:26 H.J. Lu wrote:
> From: H.J. Lu <[email protected]>
>
> This patch set changes a number of places where the kernel headers are
> exported to user space and currently use explicit "long" or "unsigned
> long" to use __kernel_[u]long_t in order to be compatible with the x32
> user space ABI. These location are places where x32 uses the x86-64
> ABI.
>
> It is quite possible that some, or even all, of these locations should
> really use dedicated types, but in the meantime this gives the correct
> results which the current headers do not.

tangentially related, but what happened to the x86 asm/ptrace.h patch i sent
that changed all the registers from unsigned long to u64 ? e.g.
struct pt_regs {
- unsigned long r15;
+ __u64 r15;
-mike


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part.

2012-05-18 00:21:52

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [RFC PATCH 00/10] Use __kernel_[u]long_t for x32 user space compatibility

On 05/17/2012 05:19 PM, Mike Frysinger wrote:
> On Thursday 17 May 2012 18:13:26 H.J. Lu wrote:
>> From: H.J. Lu <[email protected]>
>>
>> This patch set changes a number of places where the kernel
>> headers are exported to user space and currently use explicit
>> "long" or "unsigned long" to use __kernel_[u]long_t in order to
>> be compatible with the x32 user space ABI. These location are
>> places where x32 uses the x86-64 ABI.
>>
>> It is quite possible that some, or even all, of these locations
>> should really use dedicated types, but in the meantime this gives
>> the correct results which the current headers do not.
>
> tangentially related, but what happened to the x86 asm/ptrace.h
> patch i sent that changed all the registers from unsigned long to
> u64 ? e.g. struct pt_regs { - unsigned long r15; + __u64 r15;
> -mike

Link please?

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-05-18 00:22:49

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Thu, May 17, 2012 at 5:14 PM, H. Peter Anvin <[email protected]> wrote:
>
> The "logical" thing to do here seems to just use __s64, but I have no
> idea if that would suddenly break bigendian architectures...

Well, it would break the case of a 32-bit kernel and 32-bit user land.

So the thing is, that the __BITS_PER_LONG games there currently work
for two cases:

- when the kernel and user land agree on either 32-bit or 64-bit
(this is probably the common MIPS/PPC case)

- when the kernel is 64-bit, user-land is 32-bit, and we're little-endian

Using __s64 would actually break old 32-bit big-endian cases, because
the 32-bit binaries would continue to read the first 32 bits, while a
32-bit kernel would consider the first 32 bits to be the *high* bits
of the __s64 value.

That's why I think it's unfixable. It started out broken, and I
presume that 32-bit user land on a 64-bit MIPS/PPC thing either do not
work, or there's some compat crap (like special user-land headers)
fixing things up. Or they just don't use that buggered msqid64_ds
thing at all.

Linus

2012-05-18 00:28:09

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On 05/17/2012 05:22 PM, Linus Torvalds wrote:
>
> That's why I think it's unfixable. It started out broken, and I
> presume that 32-bit user land on a 64-bit MIPS/PPC thing either do not
> work, or there's some compat crap (like special user-land headers)
> fixing things up. Or they just don't use that buggered msqid64_ds
> thing at all.
>

I guess we need to find out what mips, ppc, sparc etc. do and if they
have something which works in userland make sure that it gets exported
accordingly, if they don't, then well... nothing we can do :-/

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-05-18 00:29:43

by David Daney

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On 05/17/2012 05:14 PM, H. Peter Anvin wrote:
> On 05/17/2012 05:07 PM, Linus Torvalds wrote:
>> On Thu, May 17, 2012 at 4:51 PM, H. Peter Anvin<[email protected]> wrote:
>>>
>>> The sane thing would seem to be to change __BITS_PER_LONG to 32 on x32
>>> and fix the padding hacks in struct shmid64_ds; H.J., would you agree?
>>
>> Ugh. That looks like a disaster.
>>
>> The padding hacks that depend on __BITS_PER_LONG seem pretty damn broken anyway.
>>
>> They only work if the kernel agrees with the value (which is against
>> the whole point of making __BITS_PER_LONG be about some user-level ABI
>> thing) or for little-endian machines.
>>
>> IOW, all the __BITS_PER_LONG games look totally broken to me. I can't
>> see how they could possibly even be fixed.
>>
>
> Well, on existing compat (e.g. i386) __BITS_PER_LONG is definitely not
> the same as kernel. And yes, I don't see how the heck this was ever
> correct on bigendian machines or even for compat in any form (if the
> kernel tries to interpret the extra bits and user space didn't
> initialize them we're lost.)
>
> The "logical" thing to do here seems to just use __s64, but I have no
> idea if that would suddenly break bigendian architectures...
>
> David, Ralf, do you have any idea what e.g. MIPS does here?

At the top of arch/mips/include/asm/types.h we have:

#ifdef __KERNEL__
# include <asm-generic/int-ll64.h>
#else
# if _MIPS_SZLONG == 64
# include <asm-generic/int-l64.h>
# else
# include <asm-generic/int-ll64.h>
# endif
#endif

In this case the userspace gcc will define _MIPS_SZLONG according to the
selected ABI.

in arch/mips/include/asm/bitsperlong.h we have:

#define __BITS_PER_LONG _MIPS_SZLONG

Again, the proper value for the userspace ABI.

This is either 32 or 64 depending which of the three userspace ABIs are
selected.

I don't know if that answers your question though.

My preference would be that any type that has a width that varies by
userspace ABI, not include "64" or "32" within its name.

David Daney

2012-05-18 00:32:39

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On 05/17/2012 05:29 PM, David Daney wrote:
>
> I don't know if that answers your question though.
>

Not in the slightest. The question is how on Earth struct msqid64_ds
isn't botched on MIPS.

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-05-18 00:37:15

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

It looks like MIPS has a private definition of struct msqid64_ds, as do
most other architectures; the MIPS one is completely broken for user
space usages as it uses CONFIG_* macros:

#if defined(CONFIG_32BIT) && !defined(CONFIG_CPU_LITTLE_ENDIAN)
unsigned long __unused2;
#endif
__kernel_time_t msg_rtime; /* last msgrcv time */
#if defined(CONFIG_32BIT) && defined(CONFIG_CPU_LITTLE_ENDIAN)
unsigned long __unused2;
#endif

It looks like the only users of asm-generic here are:

arch/microblaze/include/asm/msgbuf.h:#include <asm-generic/msgbuf.h>
arch/score/include/asm/msgbuf.h:#include <asm-generic/msgbuf.h>
arch/sh/include/asm/msgbuf.h:#include <asm-generic/msgbuf.h>
arch/x86/include/asm/msgbuf.h:#include <asm-generic/msgbuf.h>

... and unless I'm mistaken, x86 is the only one of those which isn't 32
bits only, which explains the reason it "works"...

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-05-18 00:38:21

by Mike Frysinger

[permalink] [raw]
Subject: Re: [RFC PATCH 00/10] Use __kernel_[u]long_t for x32 user space compatibility

On Thursday 17 May 2012 20:21:34 H. Peter Anvin wrote:
> On 05/17/2012 05:19 PM, Mike Frysinger wrote:
> > On Thursday 17 May 2012 18:13:26 H.J. Lu wrote:
> >> From: H.J. Lu <[email protected]>
> >>
> >> This patch set changes a number of places where the kernel
> >> headers are exported to user space and currently use explicit
> >> "long" or "unsigned long" to use __kernel_[u]long_t in order to
> >> be compatible with the x32 user space ABI. These location are
> >> places where x32 uses the x86-64 ABI.
> >>
> >> It is quite possible that some, or even all, of these locations
> >> should really use dedicated types, but in the meantime this gives
> >> the correct results which the current headers do not.
> >
> > tangentially related, but what happened to the x86 asm/ptrace.h
> > patch i sent that changed all the registers from unsigned long to
> > u64 ? e.g. struct pt_regs { - unsigned long r15; + __u64 r15;
>
> Link please?

https://groups.google.com/group/x32-abi/msg/b0b5be30d7aab1f8
-mike


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part.

2012-05-18 00:42:07

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Thu, May 17, 2012 at 5:22 PM, Linus Torvalds
<[email protected]> wrote:
>
> That's why I think it's unfixable. It started out broken, and I
> presume that 32-bit user land on a 64-bit MIPS/PPC thing either do not
> work, or there's some compat crap (like special user-land headers)
> fixing things up. Or they just don't use that buggered msqid64_ds
> thing at all.

Btw, even if it's unfixable, that doesn't necessarily mean that we
can't make it *prettier*.

For example, instead of this horrible crap:

__kernel_time_t msg_stime; /* last msgsnd time */
#if __BITS_PER_LONG != 64
unsigned long __unused1;
#endif

which is just nasty, we *could* have something much cleaner like this:

#define align_64_entry(type,name) \
union { type name; __u64 __align_##name; }

and then just use

align_64_entry(__kernel_time_t msg_stime);

without any preprocessor #if/#ifdef crap anywhere.

It would keep the current state for the (apparently broken) case of
64-bit kernel and 32-bit user space with big-endian architectures, but
it would *also* just magically work if __kernel_time_t is 64-bit
despite "long" being 32-bit.

So it would fix the x32 case, as far as I can tell.

Note: totally untested. Maybe there's some reason why my anonymous
union trick wouldn't work.

Linus

2012-05-18 00:45:37

by David Daney

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On 05/17/2012 05:31 PM, H. Peter Anvin wrote:
> On 05/17/2012 05:29 PM, David Daney wrote:
>>
>> I don't know if that answers your question though.
>>
>
> Not in the slightest. The question is how on Earth struct msqid64_ds
> isn't botched on MIPS.

It is botched.

#if defined(CONFIG_32BIT) && defined(CONFIG_CPU_LITTLE_ENDIAN) come out
in the userspace header file.

On mips it would appear that asm/msgbuf.h is not unique in this manner:

# grep CONFIG *.h
fcntl.h:#ifdef CONFIG_32BIT
fcntl.h:#endif /* CONFIG_32BIT */
ioctls.h:#define TIOCSERCONFIG 0x5488
msgbuf.h:#if defined(CONFIG_32BIT) && !defined(CONFIG_CPU_LITTLE_ENDIAN)
msgbuf.h:#if defined(CONFIG_32BIT) && defined(CONFIG_CPU_LITTLE_ENDIAN)
msgbuf.h:#if defined(CONFIG_32BIT) && !defined(CONFIG_CPU_LITTLE_ENDIAN)
msgbuf.h:#if defined(CONFIG_32BIT) && defined(CONFIG_CPU_LITTLE_ENDIAN)
msgbuf.h:#if defined(CONFIG_32BIT) && !defined(CONFIG_CPU_LITTLE_ENDIAN)
msgbuf.h:#if defined(CONFIG_32BIT) && defined(CONFIG_CPU_LITTLE_ENDIAN)
ptrace.h:#ifdef CONFIG_32BIT
ptrace.h:#ifdef CONFIG_CPU_HAS_SMARTMIPS
ptrace.h:#ifdef CONFIG_MIPS_MT_SMTC
ptrace.h:#endif /* CONFIG_MIPS_MT_SMTC */
ptrace.h:#ifdef CONFIG_CPU_CAVIUM_OCTEON
resource.h:#ifdef CONFIG_32BIT
siginfo.h:#ifdef CONFIG_32BIT
siginfo.h:#ifdef CONFIG_64BIT
swab.h:#ifdef CONFIG_CPU_MIPSR2
swab.h: * Having already checked for CONFIG_CPU_MIPSR2, enable the
swab.h:#ifdef CONFIG_64BIT
swab.h:#endif /* CONFIG_64BIT */
swab.h:#endif /* CONFIG_CPU_MIPSR2 */

David Daney

2012-05-18 03:22:16

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On 05/17/2012 04:51 PM, H. Peter Anvin wrote:
>
> This patch and the one before it seems to have another problem: we
> currently define __BITS_PER_LONG as:
>
> #ifdef __x86_64__
> # define __BITS_PER_LONG 64
> #else
> # define __BITS_PER_LONG 32
> #endif
>

H.J., do you see any problem *other* than this wretched struct
msqid64_ds with changing the above from __x86_64__ to

#if defined(__x86_64__) && !defined(__ILP32__)

... in the above?

As far as struct msqid64_ds, I think we can fix it simply because x86
is the only compat-aware architecture which has to deal with it.

(Incidentally, if sh is ever expanded to 64 bits, it will have a problem
in the bigendian configuration...)

-hpa

2012-05-18 03:39:09

by H.J. Lu

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Thu, May 17, 2012 at 8:21 PM, H. Peter Anvin <[email protected]> wrote:
> On 05/17/2012 04:51 PM, H. Peter Anvin wrote:
>>
>> This patch and the one before it seems to have another problem: we
>> currently define __BITS_PER_LONG as:
>>
>> #ifdef __x86_64__
>> # define __BITS_PER_LONG 64
>> #else
>> # define __BITS_PER_LONG 32
>> #endif
>>
>
> H.J., do you see any problem *other* than this wretched struct
> msqid64_ds with changing the above from __x86_64__ to
>
> #if defined(__x86_64__) && !defined(__ILP32__)
>
> ... in the above?
>
> As far as struct msqid64_ds, ?I think we can fix it simply because x86
> is the only compat-aware architecture which has to deal with it.
>
> (Incidentally, if sh is ever expanded to 64 bits, it will have a problem
> in the bigendian configuration...)

That will be wrong. __BITS_PER_LONG defines # bits of long
as seen by kernel. We don't use it in user space. Remember
x32 uses the identical interface as x86-64. So

#ifdef __x86_64__
# define __BITS_PER_LONG 64
#else
# define __BITS_PER_LONG 32
#endif

struct msqid64_ds {
struct ipc64_perm msg_perm;
__kernel_time_t msg_stime; /* last msgsnd time */
#if __BITS_PER_LONG != 64
unsigned long __unused1;
#endif
__kernel_time_t msg_rtime; /* last msgrcv time */
#if __BITS_PER_LONG != 64
unsigned long __unused2;
#endif
__kernel_time_t msg_ctime; /* last change time */
#if __BITS_PER_LONG != 64
unsigned long __unused3;
#endif

are absolutely correct for x32. You can think

#if __BITS_PER_LONG != 64

as

#ifndef __x86_64__

which is used in glibc.


--
H.J.

2012-05-18 03:43:16

by H.J. Lu

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Thu, May 17, 2012 at 8:39 PM, H.J. Lu <[email protected]> wrote:
> On Thu, May 17, 2012 at 8:21 PM, H. Peter Anvin <[email protected]> wrote:
>> On 05/17/2012 04:51 PM, H. Peter Anvin wrote:
>>>
>>> This patch and the one before it seems to have another problem: we
>>> currently define __BITS_PER_LONG as:
>>>
>>> #ifdef __x86_64__
>>> # define __BITS_PER_LONG 64
>>> #else
>>> # define __BITS_PER_LONG 32
>>> #endif
>>>
>>
>> H.J., do you see any problem *other* than this wretched struct
>> msqid64_ds with changing the above from __x86_64__ to
>>
>> #if defined(__x86_64__) && !defined(__ILP32__)
>>
>> ... in the above?
>>
>> As far as struct msqid64_ds, ?I think we can fix it simply because x86
>> is the only compat-aware architecture which has to deal with it.
>>
>> (Incidentally, if sh is ever expanded to 64 bits, it will have a problem
>> in the bigendian configuration...)
>
> That will be wrong. ? __BITS_PER_LONG defines # bits of long
> as seen by kernel. ?We don't use it in user space. ?Remember
> x32 uses the identical interface as x86-64. ?So
>
> #ifdef __x86_64__
> # define __BITS_PER_LONG 64
> #else
> # define __BITS_PER_LONG 32
> #endif
>
> struct msqid64_ds {
> ? ? ? ?struct ipc64_perm msg_perm;
> ? ? ? ?__kernel_time_t msg_stime; ? ? ?/* last msgsnd time */
> #if __BITS_PER_LONG != 64
> ? ? ? ?unsigned long ? __unused1;
> #endif
> ? ? ? ?__kernel_time_t msg_rtime; ? ? ?/* last msgrcv time */
> #if __BITS_PER_LONG != 64
> ? ? ? ?unsigned long ? __unused2;
> #endif
> ? ? ? ?__kernel_time_t msg_ctime; ? ? ?/* last change time */
> #if __BITS_PER_LONG != 64
> ? ? ? ?unsigned long ? __unused3;
> #endif
>
> are absolutely correct for x32. ?You can think
>
> #if __BITS_PER_LONG != 64
>
> as
>
> #ifndef __x86_64__
>
> which is used in glibc.
>

Now I remembered. Here is another patch. Here

#if __BITS_PER_LONG == 64

means

#ifdef __x86_64__

Change __BITS_PER_LONG to __BITS_PER_LONG_AS_SEEN_BY_KERNEL
may avoid this confusion.

--
H.J.
---
commit eadbf4f44bd12ec379f43f9359849a5012403c50
Author: H.J. Lu <[email protected]>
Date: Fri Apr 27 09:58:59 2012 -0700

Define __statfs_word as __kernel_long_t

When __BITS_PER_LONG is 64, define __statfs_word as __kernel_long_t
instead of long for user space.

Signed-off-by: H.J. Lu <[email protected]>

diff --git a/include/asm-generic/statfs.h b/include/asm-generic/statfs.h
index c749af9..2043d51 100644
--- a/include/asm-generic/statfs.h
+++ b/include/asm-generic/statfs.h
@@ -16,7 +16,7 @@ typedef __kernel_fsid_t fsid_t;
*/
#ifndef __statfs_word
#if __BITS_PER_LONG == 64
-#define __statfs_word long
+#define __statfs_word __kernel_long_t
#else
#define __statfs_word __u32
#endif

2012-05-18 03:47:40

by H.J. Lu

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Thu, May 17, 2012 at 8:43 PM, H.J. Lu <[email protected]> wrote:
> On Thu, May 17, 2012 at 8:39 PM, H.J. Lu <[email protected]> wrote:
>> On Thu, May 17, 2012 at 8:21 PM, H. Peter Anvin <[email protected]> wrote:
>>> On 05/17/2012 04:51 PM, H. Peter Anvin wrote:
>>>>
>>>> This patch and the one before it seems to have another problem: we
>>>> currently define __BITS_PER_LONG as:
>>>>
>>>> #ifdef __x86_64__
>>>> # define __BITS_PER_LONG 64
>>>> #else
>>>> # define __BITS_PER_LONG 32
>>>> #endif
>>>>
>>>
>>> H.J., do you see any problem *other* than this wretched struct
>>> msqid64_ds with changing the above from __x86_64__ to
>>>
>>> #if defined(__x86_64__) && !defined(__ILP32__)
>>>
>>> ... in the above?
>>>
>>> As far as struct msqid64_ds, ?I think we can fix it simply because x86
>>> is the only compat-aware architecture which has to deal with it.
>>>
>>> (Incidentally, if sh is ever expanded to 64 bits, it will have a problem
>>> in the bigendian configuration...)
>>
>> That will be wrong. ? __BITS_PER_LONG defines # bits of long
>> as seen by kernel. ?We don't use it in user space. ?Remember
>> x32 uses the identical interface as x86-64. ?So
>>
>> #ifdef __x86_64__
>> # define __BITS_PER_LONG 64
>> #else
>> # define __BITS_PER_LONG 32
>> #endif
>>
>> struct msqid64_ds {
>> ? ? ? ?struct ipc64_perm msg_perm;
>> ? ? ? ?__kernel_time_t msg_stime; ? ? ?/* last msgsnd time */
>> #if __BITS_PER_LONG != 64
>> ? ? ? ?unsigned long ? __unused1;
>> #endif
>> ? ? ? ?__kernel_time_t msg_rtime; ? ? ?/* last msgrcv time */
>> #if __BITS_PER_LONG != 64
>> ? ? ? ?unsigned long ? __unused2;
>> #endif
>> ? ? ? ?__kernel_time_t msg_ctime; ? ? ?/* last change time */
>> #if __BITS_PER_LONG != 64
>> ? ? ? ?unsigned long ? __unused3;
>> #endif
>>
>> are absolutely correct for x32. ?You can think
>>
>> #if __BITS_PER_LONG != 64
>>
>> as
>>
>> #ifndef __x86_64__
>>
>> which is used in glibc.
>>
>
> Now I remembered. ?Here is another patch. ?Here
>
> #if __BITS_PER_LONG == 64
>
> means
>
> #ifdef __x86_64__
>
> Change ?__BITS_PER_LONG to __BITS_PER_LONG_AS_SEEN_BY_KERNEL
> may avoid this confusion.
>

Or __BITS_PER_NATURAL_WORD.

--
H.J.

2012-05-18 03:49:51

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Thu, May 17, 2012 at 8:39 PM, H.J. Lu <[email protected]> wrote:
>
> That will be wrong. ? __BITS_PER_LONG defines # bits of long
> as seen by kernel. ?We don't use it in user space.

Yes you do. Exactly in that structure that Peter points to. *Exactly*
because that structure uses "long" instead of some fixed size. Which
will be different in user mode than in kernel mode.

And if user mode doesn't use these headers at all, then we should stop
playing the insane games.

Linus

2012-05-18 03:56:15

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On 05/17/2012 08:49 PM, Linus Torvalds wrote:
> On Thu, May 17, 2012 at 8:39 PM, H.J. Lu <[email protected]> wrote:
>>
>> That will be wrong. __BITS_PER_LONG defines # bits of long
>> as seen by kernel. We don't use it in user space.
>
> Yes you do. Exactly in that structure that Peter points to. *Exactly*
> because that structure uses "long" instead of some fixed size. Which
> will be different in user mode than in kernel mode.
>
> And if user mode doesn't use these headers at all, then we should stop
> playing the insane games.
>

User mode can, and should, be able to use the exported headers. David
Howells have been doing even more work to distill out the actual
exported ABIs from the kernel and remove remaining chaff.

That being said it seems kind of loopy to expect something called
__BITS_PER_LONG to be anything other than (CHAR_BIT*sizeof(long)),
especially since one of the main uses of it seems to be sizing
bitvectors (which has its own issues on bigendian machines because I
think we do littleendian bit numbering even on bigendian iron).

-hpa

2012-05-18 03:56:42

by H.J. Lu

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Thu, May 17, 2012 at 8:49 PM, Linus Torvalds
<[email protected]> wrote:
> On Thu, May 17, 2012 at 8:39 PM, H.J. Lu <[email protected]> wrote:
>>
>> That will be wrong. ? __BITS_PER_LONG defines # bits of long
>> as seen by kernel. ?We don't use it in user space.
>
> Yes you do. Exactly in that structure that Peter points to. *Exactly*
> because that structure uses "long" instead of some fixed size. Which
> will be different in user mode than in kernel mode.
>
> And if user mode doesn't use these headers at all, then we should stop
> playing the insane games.
>
> ? ? ? ? ? ? ? ? ? ? Linus

We need kernel headers for constants used in system calls. But I don't
mind if kernel stops exporting data structures to user space.


--
H.J.

2012-05-18 03:59:13

by H.J. Lu

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Thu, May 17, 2012 at 8:55 PM, H. Peter Anvin <[email protected]> wrote:
> On 05/17/2012 08:49 PM, Linus Torvalds wrote:
>> On Thu, May 17, 2012 at 8:39 PM, H.J. Lu <[email protected]> wrote:
>>>
>>> That will be wrong. ? __BITS_PER_LONG defines # bits of long
>>> as seen by kernel. ?We don't use it in user space.
>>
>> Yes you do. Exactly in that structure that Peter points to. *Exactly*
>> because that structure uses "long" instead of some fixed size. Which
>> will be different in user mode than in kernel mode.
>>
>> And if user mode doesn't use these headers at all, then we should stop
>> playing the insane games.
>>
>
> User mode can, and should, be able to use the exported headers. ?David
> Howells have been doing even more work to distill out the actual
> exported ABIs from the kernel and remove remaining chaff.
>
> That being said it seems kind of loopy to expect something called
> __BITS_PER_LONG to be anything other than (CHAR_BIT*sizeof(long)),
> especially since one of the main uses of it seems to be sizing
> bitvectors (which has its own issues on bigendian machines because I
> think we do littleendian bit numbering even on bigendian iron).
>

But __BITS_PER_LONG used in kernel header files really
means "long" as seen by kernel, not by user space. 64-bit
kernel can have 32-bit and 64-bit longs in use space.
__BITS_PER_LONG is a bad name for user space.


--
H.J.

2012-05-18 04:05:48

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Thu, May 17, 2012 at 8:59 PM, H.J. Lu <[email protected]> wrote:
>
> But __BITS_PER_LONG used in kernel header files really
> means "long" as seen by kernel, not by user space.

No, hjl, no it does NOT.

That might be what we'd all *wish* it did, but it's not at all what it does.

What it actually means is "how many bits in a long as it is being
compiled right now".

User code headers have absolutely *no* idea what the size "long" the
kernel was compiled with. You'd have to do an "uname()" system call to
figure that out, and even then you shouldn't be sure if the kernel is
just really good at compatibility mode.

Just look at the code. It literally sets __BITS_PER_LONG depending on
*compiler* flags like __s390x__ etc. Which depend on things like
"-m32" etc as done in user space.

When those same headers are compiled *as*part*of*the*kernel*, then -
and only then - does __BITS_PER_LONG hopefully match what the kernel
"long" is. But that is kind of irrelevant, because if this was purely
a kernel issue, we would use the non-underscore version (ie just
"BITS_PER_LONG") which is *only* for the kernel, and not exported to
user space at all.

Linus

2012-05-18 04:14:01

by H.J. Lu

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Thu, May 17, 2012 at 9:05 PM, Linus Torvalds
<[email protected]> wrote:
> On Thu, May 17, 2012 at 8:59 PM, H.J. Lu <[email protected]> wrote:
>>
>> But __BITS_PER_LONG used in kernel header files really
>> means "long" as seen by kernel, not by user space.
>
> No, hjl, no it does NOT.
>
> That might be what we'd all *wish* it did, but it's not at all what it does.
>
> What it actually means is "how many bits in a long as it is being
> compiled right now".
>
> User code headers have absolutely *no* idea what the size "long" the
> kernel was compiled with. You'd have to do an "uname()" system call to
> figure that out, and even then you shouldn't be sure if the kernel is
> just really good at compatibility mode.
>
> Just look at the code. It literally sets __BITS_PER_LONG depending on
> *compiler* flags like __s390x__ etc. Which depend on things like
> "-m32" etc as done in user space.
>
> When those same headers are compiled *as*part*of*the*kernel*, then -
> and only then - does __BITS_PER_LONG hopefully match what the kernel
> "long" is. But that is kind of irrelevant, because if this was purely
> a kernel issue, we would use the non-underscore version (ie just
> "BITS_PER_LONG") which is *only* for the kernel, and not exported to
> user space at all.
>

Since x32 uses the same kernel interface as x86-64 with a few
exceptions. The current kernel header files with

#ifdef __x86_64__
# define __BITS_PER_LONG 64
#else
# define __BITS_PER_LONG 32
#endif

#if __BITS_PER_LONG == 64
Define x86-64 types
#endif

work fine for x32 even if long for x32 is 32 bits. If __BITS_PER_LONG
is changed to 32 for x32, many types in kernel header files will be
wrong for x32.


--
H.J.

2012-05-18 11:44:47

by David Howells

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

Linus Torvalds <[email protected]> wrote:

> Note: totally untested. Maybe there's some reason why my anonymous
> union trick wouldn't work.

It might conceivably break a userspace compiler if it gets exported to
userspace. I'm not sure how likely that would be, though.

David

2012-05-18 11:53:22

by David Howells

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

H. Peter Anvin <[email protected]> wrote:

> User mode can, and should, be able to use the exported headers. David
> Howells have been doing even more work to distill out the actual
> exported ABIs from the kernel and remove remaining chaff.

See:

http://git.infradead.org/users/dhowells/linux-headers.git/shortlog/refs/heads/uapi-split

Linus: Any chance of this being pulled at the end of the next merge window? I
know you said you didn't want to do another big header file cleanup for a
while after pulling the system.h disintegration, but I'm wondering if your
distaste has expired yet.

David

2012-05-18 12:06:40

by H.J. Lu

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Fri, May 18, 2012 at 4:53 AM, David Howells <[email protected]> wrote:
> H. Peter Anvin <[email protected]> wrote:
>
>> User mode can, and should, be able to use the exported headers. ?David
>> Howells have been doing even more work to distill out the actual
>> exported ABIs from the kernel and remove remaining chaff.
>
> See:
>
> ? ? ? ?http://git.infradead.org/users/dhowells/linux-headers.git/shortlog/refs/heads/uapi-split
>
> Linus: Any chance of this being pulled at the end of the next merge window? ?I
> know you said you didn't want to do another big header file cleanup for a
> while after pulling the system.h disintegration, but I'm wondering if your
> distaste has expired yet.
>

As shown by x32, the kernel interface types are really arch-dependent.
Any generic types based on long won't work for x32.


--
H.J.

2012-05-18 15:03:20

by Chris Metcalf

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

(Cc'ing libc-ports and Arnd Bergmann.)

On 5/17/2012 8:37 PM, H. Peter Anvin wrote:
> It looks like MIPS has a private definition of struct msqid64_ds, as do
> most other architectures; the MIPS one is completely broken for user
> space usages as it uses CONFIG_* macros:
>
> #if defined(CONFIG_32BIT) && !defined(CONFIG_CPU_LITTLE_ENDIAN)
> unsigned long __unused2;
> #endif
> __kernel_time_t msg_rtime; /* last msgrcv time */
> #if defined(CONFIG_32BIT) && defined(CONFIG_CPU_LITTLE_ENDIAN)
> unsigned long __unused2;
> #endif
>
> It looks like the only users of asm-generic here are:
>
> arch/microblaze/include/asm/msgbuf.h:#include <asm-generic/msgbuf.h>
> arch/score/include/asm/msgbuf.h:#include <asm-generic/msgbuf.h>
> arch/sh/include/asm/msgbuf.h:#include <asm-generic/msgbuf.h>
> arch/x86/include/asm/msgbuf.h:#include <asm-generic/msgbuf.h>
>
> ... and unless I'm mistaken, x86 is the only one of those which isn't 32
> bits only, which explains the reason it "works"...

Don't forget the newer architectures that use generic-y:

arch/blackfin/include/asm/Kbuild:generic-y += msgbuf.h
arch/c6x/include/asm/Kbuild:generic-y += msgbuf.h
arch/hexagon/include/asm/Kbuild:generic-y += msgbuf.h
arch/openrisc/include/asm/Kbuild:generic-y += msgbuf.h
arch/tile/include/asm/Kbuild:generic-y += msgbuf.h
arch/unicore32/include/asm/Kbuild:generic-y += msgbuf.h

On 5/17/2012 8:22 PM, Linus Torvalds wrote:
> That's why I think it's unfixable. It started out broken, and I
> presume that 32-bit user land on a 64-bit MIPS/PPC thing either do not
> work, or there's some compat crap (like special user-land headers)
> fixing things up. Or they just don't use that buggered msqid64_ds
> thing at all.

Yes, it's compat crap. Each architecture defines its own compat_msqid64_ds
that carefully places the padding in the same place as the corresponding
native 32-bit kernel puts it, and glibc doesn't use <asm/msgbuf.h> at all,
but instead provides a hand-rolled <bits/msq.h>.

I added a glibc <bits/msq.h> that targets the asm-generic version of the
header, so it puts the padding after the time_t, even on big-endian
platforms. This is of course crazy, but it's what you need to do to use
the current asm-generic header.

However, now is a good time to ask whether this is the right thing to do
going forward. The tile architecture is the only one that currently offers
a 32-bit big-endian userspace (only in linux-next, queued for 3.5) and also
uses <asm-generic> for the SysV stuff, so if someone wants to advocate for
changing this, now is definitely a good time for it.

We could modify asm-generic so that it puts the padding where it "ought" to
go, easily enough. This would then allow three of the four big-endian
32-bit architectures (sparc, parisc, powerpc) to use the generic headers,
if they wished. (The fourth one, s390, puts the padding after, like
asm-generic currently does.)

This might then allow us to consider making "struct compat_msqid64_ds" and
friends something that is defined in <linux/compat.h> rather than
per-architecture, though we'd need a hook to allow architectures like s390
to override and provide their own definition.

As far as x32 goes, this should all be largely irrelevant, since it likely
won't use the compat structures anyway, and since we're not proposing
breaking the little-endian layout. But future x32-style compat big-endian
architectures would benefit from having a msqid64_ds structure that
actually lays out the same in 32- or 64-bit mode.
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com

2012-05-18 21:06:50

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On 05/17/2012 08:56 PM, H.J. Lu wrote:
>
> We need kernel headers for constants used in system calls. But I don't
> mind if kernel stops exporting data structures to user space.
>

I do. It is idiotic that data structure definitions should have be
replicated by cut and paste; all it is is a completely unnecessary
source of both delay and of error.

-hpa

2012-05-18 21:21:27

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Friday 18 May 2012, H.J. Lu wrote:
> Since x32 uses the same kernel interface as x86-64 with a few
> exceptions. The current kernel header files with
>
> #ifdef __x86_64__
> # define __BITS_PER_LONG 64
> #else
> # define __BITS_PER_LONG 32
> #endif
>
> #if __BITS_PER_LONG == 64
> Define x86-64 types
> #endif
>
> work fine for x32 even if long for x32 is 32 bits. If __BITS_PER_LONG
> is changed to 32 for x32, many types in kernel header files will be
> wrong for x32.
>

A lot of things are broken if __BITS_PER_LONG is set to 64 in x32 user
space. It was specifically introduced to get around places in exported
headers that incorrectly used '#ifdef CONFIG_64BIT' to define a user
space structure, so that we can depend on whatever the user sees
in bitmasks and other data structures.

Arnd

2012-05-18 21:31:39

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Friday 18 May 2012, Linus Torvalds wrote:
> For example, instead of this horrible crap:
>
> __kernel_time_t msg_stime; /* last msgsnd time */
> #if __BITS_PER_LONG != 64
> unsigned long __unused1;
> #endif
>
> which is just nasty, we could have something much cleaner like this:
>
> #define align_64_entry(type,name) \
> union { type name; __u64 __align_##name; }
>
> and then just use
>
> align_64_entry(__kernel_time_t msg_stime);
>
> without any preprocessor #if/#ifdef crap anywhere.
>
> It would keep the current state for the (apparently broken) case of
> 64-bit kernel and 32-bit user space with big-endian architectures, but
> it would also just magically work if __kernel_time_t is 64-bit
> despite "long" being 32-bit.
>
> So it would fix the x32 case, as far as I can tell.
>
> Note: totally untested. Maybe there's some reason why my anonymous
> union trick wouldn't work.

When the header files were written, we still allowed them to be included
by programs that were written for older non-gcc compilers.

We already rely on 'long long' being a valid type these days, so we could
probably extend the requirement to cover anonymous unions as well, but
there may be cases where some code includes this file and needs to get
built with -std=something-old that doesn't allow anonymous unions.

As another historical background on this header, note that some big-endian
architectures put the padding before the variable but others don't,
presumably because the person doing the architecture port didn't understand
the intention or didn't care.

However, in the kernel we *always* copy the fields one by one for compat
mode, even for the architectures that have identical layout between 32 and
64 bit, and at least one libc implementation that I've seen (IIRC uClibc)
hardcodes the data structure to be the same as x86, with the padding
after the 'long', for all architectures. When I introduced the asm-generic
version of this, we had a discussion about whether we should try to use
the version with the "correct" padding but in the end decided to just use
the x86 version because that is what most big-endian architectures do
anyway.

Arnd

2012-05-18 21:41:40

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On 05/18/2012 02:31 PM, Arnd Bergmann wrote:
>
> However, in the kernel we *always* copy the fields one by one for compat
> mode, even for the architectures that have identical layout between 32 and
> 64 bit, and at least one libc implementation that I've seen (IIRC uClibc)
> hardcodes the data structure to be the same as x86, with the padding
> after the 'long', for all architectures. When I introduced the asm-generic
> version of this, we had a discussion about whether we should try to use
> the version with the "correct" padding but in the end decided to just use
> the x86 version because that is what most big-endian architectures do
> anyway.
>

Ouch. Fail. asm-generic should be about what is the right thing going
forward.

-hpa

2012-05-18 21:59:16

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Friday 18 May 2012, H. Peter Anvin wrote:
>
> On 05/18/2012 02:31 PM, Arnd Bergmann wrote:
> >
> > However, in the kernel we always copy the fields one by one for compat
> > mode, even for the architectures that have identical layout between 32 and
> > 64 bit, and at least one libc implementation that I've seen (IIRC uClibc)
> > hardcodes the data structure to be the same as x86, with the padding
> > after the 'long', for all architectures. When I introduced the asm-generic
> > version of this, we had a discussion about whether we should try to use
> > the version with the "correct" padding but in the end decided to just use
> > the x86 version because that is what most big-endian architectures do
> > anyway.
> >
>
> Ouch. Fail. asm-generic should be about what is the right thing going
> forward.

But why do you think it's wrong the way it is? I see the idea of putting
padding in varying places depending on the endianess as a failed experiment
and defining a structure that is always the same as the logical conclusion
from that, even if it's a bit silly to have any padding in it at all.

Being consistent seems more important here than following the intent
of whoever came up with the concept of the ipc64 data structures
and was consequently ignored by most people after him.

If we really wanted the data structures to be compatible between 32 and
64 bit mode, we'd have to use __u64 here but that would mean having to
change all bits of user code that already rely on the existing x86
compatible layout.

Arnd

2012-05-18 22:08:54

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On 05/18/2012 02:58 PM, Arnd Bergmann wrote:
>
> But why do you think it's wrong the way it is? I see the idea of putting
> padding in varying places depending on the endianess as a failed experiment
> and defining a structure that is always the same as the logical conclusion
> from that, even if it's a bit silly to have any padding in it at all.
>

The *whole point* is to make the structure the same across modes, to
make the compat layer's job easier.

> Being consistent seems more important here than following the intent
> of whoever came up with the concept of the ipc64 data structures
> and was consequently ignored by most people after him.

So you're saying because some architectures introduced a bug, we should
*CONTINUE* to introduce the same bug?? WTF??

> If we really wanted the data structures to be compatible between 32 and
> 64 bit mode, we'd have to use __u64 here but that would mean having to
> change all bits of user code that already rely on the existing x86
> compatible layout.

x86 is doing it right. Some bigendian architectures blindly copied what
x86 was doing without thinking. That's a bug on their part, period.

-hpa

2012-05-19 07:56:52

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Friday 18 May 2012, H. Peter Anvin wrote:
> On 05/18/2012 02:58 PM, Arnd Bergmann wrote:
> >
> > But why do you think it's wrong the way it is? I see the idea of putting
> > padding in varying places depending on the endianess as a failed experiment
> > and defining a structure that is always the same as the logical conclusion
> > from that, even if it's a bit silly to have any padding in it at all.
> >
>
> The *whole point* is to make the structure the same across modes, to
> make the compat layer's job easier.

I thought the point was that we could extend time_t to 64 bit at a later
point, which we already concluded isn't really happening for existing
architectures, or at least we will have bigger problems to worry about
if we do.

> > Being consistent seems more important here than following the intent
> > of whoever came up with the concept of the ipc64 data structures
> > and was consequently ignored by most people after him.
>
> So you're saying because some architectures introduced a bug, we should
> CONTINUE to introduce the same bug?? WTF??

The real bug was to have a structure that is defined differently per
architecture.

> > If we really wanted the data structures to be compatible between 32 and
> > 64 bit mode, we'd have to use __u64 here but that would mean having to
> > change all bits of user code that already rely on the existing x86
> > compatible layout.
>
> x86 is doing it right. Some bigendian architectures blindly copied what
> x86 was doing without thinking. That's a bug on their part, period.

But when I did the generic header, the conclusion was that the situation
was already so much screwed up that any attempt to "fix" it would have
caused other problems:

* uclibc never incorporated the concept of per-architecture ipc header
files, meaning it was already wrong on the few architectures that
tried to do the right thing, but worked on those that copied from x86.
Should we trust the next person to to a uclibc port to a new architecture
to understand the situation fully and fix uclibc without breaking
existing architectures?

* __kernel_time_t is not the only type that differs between 32 and 64
bit platforms: the structures also include a __kernel_mode_t, which
is different between 32/64 bit on at least s390, x86, arm and sparc.

* MIPS defines the data structure with padding on the correct side, but
uses the wrong #ifdef, so all user space trying to use the definitions
from the kernel is already broken, and the common case of little-endian
mips32 with uclibc only works by coincidence because uclibc got it wrong
in a different way that results in the same data structure.

* sparc, powerpc, s390, x86, mips and in fact all 32 bit architectures
use 'unsigned long' for msg_cbytes, sem_nsems, shm_nattch etc.
Who cares abouto the padding for time_t when the structure is
still incompatible and requires wrappers for 32 bit mode?

* parisc got all of the above right, but failed in two other aspects:
according to the comment in arch/parisc/include/asm/shmbuf.h, the
shminfo structure is defined too short for 64 bit user space, and
at some point they managed to break all the structures for 64 bit
mode by turning #ifdef __LP64__ into #ifdef CONFIG_64BIT. This only
works because everybody uses 32 bit user space on parisc anyway.
I thought we had fixed it a couple of years ago but looking at the
code now, it's still broken.

So, given that fact that nobody ever implemented this structure correctly,
damage control was the best option available.

Arnd

2012-05-19 14:35:33

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Sat, May 19, 2012 at 07:56:30AM +0000, Arnd Bergmann wrote:

> * __kernel_time_t is not the only type that differs between 32 and 64
> bit platforms: the structures also include a __kernel_mode_t, which
> is different between 32/64 bit on at least s390, x86, arm and sparc.

... and said __kernel_mode_t needs to die. Along with all remaining
instances of mode_t kernel-side.

BTW, ncp_mount_data (i.e. ncp mount with version less than 4) needs to be
added to feature-removal-schedule.txt. That'll bury one of the few places
where we have that crap in public headers.

And no, assorted struct stat do not need that thing at all. They should
use explicit types, TYVM.

BTW, why do we have __kernel_uid32_t? As an invitation for some architecture
to come up with 64bit "32bit UID" type? What's wrong with __u32, or
at least moving the default (== only, fortunately) definitions to
linux/posix_types.h and making them unconditional? Same for gid32_t
stuff, of course...

__kernel_nlink_t is also a bad idea (and an invitation to bugs in the kernel,
while we are at it). BTW, now that I look at it... ulong_t as default is
also wrong; there's no point making the internal nlink_t different on different
platforms. Fortunately, that one is really easy to kill; mips/parisc/ppc
stat.h instances switch to corresponding explictly-sized types, at which point
nothing in userland needs to see it and we can simply switch nlink_t
kernel-side to __u32 and be done with that...

2012-05-19 23:48:24

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On 05/18/2012 02:21 PM, Arnd Bergmann wrote:
> On Friday 18 May 2012, H.J. Lu wrote:
>> Since x32 uses the same kernel interface as x86-64 with a few
>> exceptions. The current kernel header files with
>>
>> #ifdef __x86_64__
>> # define __BITS_PER_LONG 64
>> #else
>> # define __BITS_PER_LONG 32
>> #endif
>>
>> #if __BITS_PER_LONG == 64
>> Define x86-64 types
>> #endif
>>
>> work fine for x32 even if long for x32 is 32 bits. If __BITS_PER_LONG
>> is changed to 32 for x32, many types in kernel header files will be
>> wrong for x32.
>>
>
> A lot of things are broken if __BITS_PER_LONG is set to 64 in x32 user
> space. It was specifically introduced to get around places in exported
> headers that incorrectly used '#ifdef CONFIG_64BIT' to define a user
> space structure, so that we can depend on whatever the user sees
> in bitmasks and other data structures.
>

I'm not entirely sure I follow the above statement, as it seems
contradict itself. Either way, I would agree, __BITS_PER_LONG should be
32 in x32 space and if there are places where that is wrong then we need
to fix them.

Fortunately x32 is littleendian, so no "littleendian bitmask on a
bigendian architecture" drain bamage...

-hpa

2012-05-20 01:32:52

by H.J. Lu

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On Sat, May 19, 2012 at 4:47 PM, H. Peter Anvin <[email protected]> wrote:
> On 05/18/2012 02:21 PM, Arnd Bergmann wrote:
>> On Friday 18 May 2012, H.J. Lu wrote:
>>> Since x32 uses the same kernel interface as x86-64 with a few
>>> exceptions. ?The current kernel header files with
>>>
>>> #ifdef __x86_64__
>>> # define __BITS_PER_LONG 64
>>> #else
>>> # define __BITS_PER_LONG 32
>>> #endif
>>>
>>> #if __BITS_PER_LONG == 64
>>> Define x86-64 types
>>> #endif
>>>
>>> work fine for x32 even if long for x32 is 32 bits. ?If __BITS_PER_LONG
>>> is changed to 32 for x32, many types in kernel header files will be
>>> wrong for x32.
>>>
>>
>> A lot of things are broken if __BITS_PER_LONG is set to 64 in x32 user
>> space. It was specifically introduced to get around places in exported
>> headers that incorrectly used '#ifdef CONFIG_64BIT' to define a user
>> space structure, so that we can depend on whatever the user sees
>> in bitmasks and other data structures.
>>
>
> I'm not entirely sure I follow the above statement, as it seems
> contradict itself. ?Either way, I would agree, __BITS_PER_LONG should be
> 32 in x32 space and if there are places where that is wrong then we need
> to fix them.
>
> Fortunately x32 is littleendian, so no "littleendian bitmask on a
> bigendian architecture" drain bamage...
>

If __BITS_PER_LONG is set to 32 for x32, we need to audit all
exported kernel headers where __BITS_PER_LONG is checked.
If long is used in those places, it has to be updated for x32.

--
H.J.

2012-05-20 02:09:06

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 08/10] Use __kernel_ulong_t in struct msqid64_ds

On 05/19/2012 06:32 PM, H.J. Lu wrote:
>
> If __BITS_PER_LONG is set to 32 for x32, we need to audit all
> exported kernel headers where __BITS_PER_LONG is checked.
> If long is used in those places, it has to be updated for x32.
>

I think we have to do that anyway.

-hpa