Florian Weimer points out an issue with including linux/posix_types.h
from arbitrary other headers: if an application has defined a macro
named 'fds_bits', it may stop compiling after a change in the kernel
headers. Since fds_bits is a non-reserved identifier in C, that is
considered a bug in the kernel headers.
The structure definition does not really seem to be helpful here,
as the kernel no longer provides macros to manipulate it.
The only user of the structure that we could find was in libsanitize.
This usage is actually broken, as discussed in the email thread,
but this means we cannot just remove the definition from the exported
headers, but we can use the __kernel_* namespace for it, to avoid
the namespace conflict. The kernel itself just uses bit operations
on the typedef without accessing the field by name.
This change should also help with the many other kernel headers that
include linux/posix_types.h directly or through linux/types.h.
Similarly, the definition of __kernel_fsid_t uses a structure with
field identifier 'val' that may have the same problem. Again, user
space should not rely on the specific field name but instead treat
an fsid_t as an opaque identifier. I'm using an #ifdef __KERNEL__
guard here to save us from having to change all kernel code accessing
the field.
Glibc has changed this from 'val' to '__val' back in 1996/97 when
they moved away from using the kernel types directly, it it
is likely that nothing uses __kernel_fsid_t any more. MIPS still
has another copy of __kernel_fsid_t, but that is in the process
of being removed as well.
Link: https://lore.kernel.org/lkml/[email protected]/
Link: https://lore.kernel.org/lkml/[email protected]/
Cc: Florian Weimer <[email protected]>
Cc: Paul Burton <[email protected]>
Fixes: a623a7a1a567 ("y2038: fix socket.h header inclusion")
Signed-off-by: Arnd Bergmann <[email protected]>
---
There was another bug report for the same change just now, so
I wouldn't apply this patch yet before we have fully understood
the other issue. Sending this for review now since the two
problems are most likely independent.
---
include/uapi/asm-generic/posix_types.h | 4 ++++
include/uapi/linux/posix_types.h | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/uapi/asm-generic/posix_types.h b/include/uapi/asm-generic/posix_types.h
index f0733a26ebfc..2a8c68ac88ca 100644
--- a/include/uapi/asm-generic/posix_types.h
+++ b/include/uapi/asm-generic/posix_types.h
@@ -77,7 +77,11 @@ typedef __kernel_long_t __kernel_ptrdiff_t;
#ifndef __kernel_fsid_t
typedef struct {
+#ifdef __KERNEL__
int val[2];
+#else
+ int __kernel_val[2];
+#endif
} __kernel_fsid_t;
#endif
diff --git a/include/uapi/linux/posix_types.h b/include/uapi/linux/posix_types.h
index 9a7a740b35a2..a5a5cfc38bbf 100644
--- a/include/uapi/linux/posix_types.h
+++ b/include/uapi/linux/posix_types.h
@@ -23,7 +23,7 @@
#define __FD_SETSIZE 1024
typedef struct {
- unsigned long fds_bits[__FD_SETSIZE / (8 * sizeof(long))];
+ unsigned long __kernel_fds_bits[__FD_SETSIZE / (8 * sizeof(long))];
} __kernel_fd_set;
/* Type of a signal handler. */
--
2.20.0
From: Arnd Bergmann <[email protected]>
Date: Tue, 19 Mar 2019 17:51:09 +0100
> Florian Weimer points out an issue with including linux/posix_types.h
> from arbitrary other headers: if an application has defined a macro
> named 'fds_bits', it may stop compiling after a change in the kernel
> headers. Since fds_bits is a non-reserved identifier in C, that is
> considered a bug in the kernel headers.
>
> The structure definition does not really seem to be helpful here,
> as the kernel no longer provides macros to manipulate it.
...
> Fixes: a623a7a1a567 ("y2038: fix socket.h header inclusion")
> Signed-off-by: Arnd Bergmann <[email protected]>
Acked-by: David S. Miller <[email protected]>
* Arnd Bergmann:
> diff --git a/include/uapi/asm-generic/posix_types.h b/include/uapi/asm-generic/posix_types.h
> index f0733a26ebfc..2a8c68ac88ca 100644
> --- a/include/uapi/asm-generic/posix_types.h
> +++ b/include/uapi/asm-generic/posix_types.h
> @@ -77,7 +77,11 @@ typedef __kernel_long_t __kernel_ptrdiff_t;
>
> #ifndef __kernel_fsid_t
> typedef struct {
> +#ifdef __KERNEL__
> int val[2];
> +#else
> + int __kernel_val[2];
> +#endif
> } __kernel_fsid_t;
> #endif
>
> diff --git a/include/uapi/linux/posix_types.h b/include/uapi/linux/posix_types.h
> index 9a7a740b35a2..a5a5cfc38bbf 100644
> --- a/include/uapi/linux/posix_types.h
> +++ b/include/uapi/linux/posix_types.h
> @@ -23,7 +23,7 @@
> #define __FD_SETSIZE 1024
>
> typedef struct {
> - unsigned long fds_bits[__FD_SETSIZE / (8 * sizeof(long))];
> + unsigned long __kernel_fds_bits[__FD_SETSIZE / (8 * sizeof(long))];
> } __kernel_fd_set;
>
> /* Type of a signal handler. */
Both changes look reasonably to me, but I have not tested them.
What happened with this patch (posted 19 March)? I found today that we
can't use Linux 5.1 headers in glibc testing because the namespace issues
are still present in the headers as of the release.
--
Joseph S. Myers
[email protected]
* Joseph Myers:
> What happened with this patch (posted 19 March)? I found today that we
> can't use Linux 5.1 headers in glibc testing because the namespace issues
> are still present in the headers as of the release.
This regression fix still hasn't been merged into Linus' tree. What is
going on here?
This might seem rather minor, but the namespace testing is actually
relevant in practice. It prevents accidental clashes with C/C++
identifiers in user code.
If this fairly central UAPI header is not made namespace-clean again,
then we need to duplicate information from more UAPI headers in glibc,
and I don't think that's something we'd want to do.
Thanks,
Florian
On Thu, Jun 6, 2019 at 9:28 PM Florian Weimer <[email protected]> wrote:
>
> This regression fix still hasn't been merged into Linus' tree. What is
> going on here?
.. it was never sent to me.
That said, now that I see the patch, I wonder why we'd have that
#ifdef __KERNEL__ in here:
typedef struct {
+#ifdef __KERNEL__
int val[2];
+#else
+ int __kernel_val[2];
+#endif
} __kernel_fsid_t;
and not just unconditionally do
int __fsid_val[2]
If we're changing kernel header files, it's easy enough to change the
kernel users. I'd be more worried about user space that *uses* that
thing, and currently accesses 'val[]' by name.
So the patch looks a bit odd to me. How are people supposed to use
fsid_t if they can't look at it?
The man-page makes it pretty clear that fsid_t is complete garbage,
but it's *documented* garbage:
The f_fsid field
Solaris, Irix and POSIX have a system call statvfs(2) that
returns a struct statvfs (defined in <sys/statvfs.h>) containing an
unsigned long f_fsid. Linux, SunOS, HP-UX, 4.4BSD have a system call
statfs() that returns a struct
statfs (defined in <sys/vfs.h>) containing a fsid_t f_fsid,
where fsid_t is defined as struct { int val[2]; }. The same holds for
FreeBSD, except that it uses the include file <sys/mount.h>.
so that "val[]" name does seem to be pretty much required.
In other words, I don't think the patch is acceptable. User space sees
"val[]" and _needs_ to see it. Otherwise the type is entirely
pointless.
The proper fix is presumably do make sure the fsid_t type definitions
aren't visible to user space at all in this context, and is only
visible in <sys/statvfs.h>.
So now that I _do_ see the patch, there's no way I'll apply it.
Linus
* Linus Torvalds:
> If we're changing kernel header files, it's easy enough to change the
> kernel users. I'd be more worried about user space that *uses* that
> thing, and currently accesses 'val[]' by name.
>
> So the patch looks a bit odd to me. How are people supposed to use
> fsid_t if they can't look at it?
The problem is that the header was previously not used pervasively in
userspace headers. See commit a623a7a1a5670c25a16881f5078072d272d96b71
("y2038: fix socket.h header inclusion"). Very little code needed it
before.
On the glibc side, we nowadays deal with this by splitting headers
further. (We used to suppress definitions with macros, but that tended
to become convoluted.) In this case, moving the definition of
__kernel_long_t to its own header, so that
include/uapi/asm-generic/socket.h can include that should fix it.
> So now that I _do_ see the patch, there's no way I'll apply it.
Fair enough.
Thanks,
Florian
On Fri, Jun 7, 2019 at 11:43 AM Florian Weimer <[email protected]> wrote:
>
> On the glibc side, we nowadays deal with this by splitting headers
> further. (We used to suppress definitions with macros, but that tended
> to become convoluted.) In this case, moving the definition of
> __kernel_long_t to its own header, so that
> include/uapi/asm-generic/socket.h can include that should fix it.
I think we should strive to do that on the kernel side too, since
clearly we shouldn't expose that "val[]" thing in the core posix types
due to namespace rules, but at the same time I think the patch to
rename val[] is fundamentally broken too.
Can you describe how you split things (perhaps even with a patch ;)?
Is this literally the only issue you currently have? Because I'd
expect similar issues to show up elsewhere too, but who knows.. You
presumably do.
Linus
* Linus Torvalds:
> On Fri, Jun 7, 2019 at 11:43 AM Florian Weimer <[email protected]> wrote:
>>
>> On the glibc side, we nowadays deal with this by splitting headers
>> further. (We used to suppress definitions with macros, but that tended
>> to become convoluted.) In this case, moving the definition of
>> __kernel_long_t to its own header, so that
>> include/uapi/asm-generic/socket.h can include that should fix it.
>
> I think we should strive to do that on the kernel side too, since
> clearly we shouldn't expose that "val[]" thing in the core posix types
> due to namespace rules, but at the same time I think the patch to
> rename val[] is fundamentally broken too.
>
> Can you describe how you split things (perhaps even with a patch ;)?
> Is this literally the only issue you currently have? Because I'd
> expect similar issues to show up elsewhere too, but who knows.. You
> presumably do.
I wanted to introduce a new header, <asm/kernel_long_t.h>, and include
it where the definition of __kernel_long_t is needed, something like
this (incomplete, untested):
diff --git a/arch/sparc/include/uapi/asm/posix_types.h b/arch/sparc/include/uapi/asm/posix_types.h
index f139e0048628..6510d7538605 100644
--- a/arch/sparc/include/uapi/asm/posix_types.h
+++ b/arch/sparc/include/uapi/asm/posix_types.h
@@ -8,6 +8,8 @@
#ifndef __SPARC_POSIX_TYPES_H
#define __SPARC_POSIX_TYPES_H
+#include <asm/kernel_long_t.h>
+
#if defined(__sparc__) && defined(__arch64__)
/* sparc 64 bit */
@@ -19,10 +21,6 @@ typedef unsigned short __kernel_old_gid_t;
typedef int __kernel_suseconds_t;
#define __kernel_suseconds_t __kernel_suseconds_t
-typedef long __kernel_long_t;
-typedef unsigned long __kernel_ulong_t;
-#define __kernel_long_t __kernel_long_t
-
struct __kernel_old_timeval {
__kernel_long_t tv_sec;
__kernel_suseconds_t tv_usec;
diff --git a/arch/x86/include/uapi/asm/kernel_long_t.h b/arch/x86/include/uapi/asm/kernel_long_t.h
new file mode 100644
index 000000000000..ed3bff40e1e8
--- /dev/null
+++ b/arch/x86/include/uapi/asm/kernel_long_t.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef __KERNEL__
+# ifdef defined(__x86_64__) && defined(__ILP32__)
+# include <asm/kernel_long_t_x32.h>
+# else
+# include <asm-generic/kernel_long_t.h>
+# endif
+#endif
diff --git a/arch/x86/include/uapi/asm/kernel_long_t_x32.h b/arch/x86/include/uapi/asm/kernel_long_t_x32.h
new file mode 100644
index 000000000000..a71cbce7e966
--- /dev/null
+++ b/arch/x86/include/uapi/asm/kernel_long_t_x32.h
@@ -0,0 +1,6 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _ASM_X86_KERNEL_LONG_T_X32_H
+#define _ASM_X86_KERNEL_LONG_T_X32_H
+typedef long long __kernel_long_t;
+typedef unsigned long long __kernel_ulong_t;
+#endif /* _ASM_X86_KERNEL_LONG_T_X32_H */
diff --git a/arch/x86/include/uapi/asm/posix_types_x32.h b/arch/x86/include/uapi/asm/posix_types_x32.h
index f60479b07fc8..92c7af21da9e 100644
--- a/arch/x86/include/uapi/asm/posix_types_x32.h
+++ b/arch/x86/include/uapi/asm/posix_types_x32.h
@@ -11,10 +11,6 @@
*
*/
-typedef long long __kernel_long_t;
-typedef unsigned long long __kernel_ulong_t;
-#define __kernel_long_t __kernel_long_t
-
#include <asm/posix_types_64.h>
#endif /* _ASM_X86_POSIX_TYPES_X32_H */
diff --git a/include/uapi/asm-generic/kernel_long_t.h b/include/uapi/asm-generic/kernel_long_t.h
new file mode 100644
index 000000000000..649a97a8c304
--- /dev/null
+++ b/include/uapi/asm-generic/kernel_long_t.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef __ASM_GENERIC_KERNEL_LONG_T_H
+#define __ASM_GENERIC_KERNEL_LONG_T_H
+
+typedef long __kernel_long_t;
+typedef unsigned long __kernel_ulong_t;
+
+#endif /* __ASM_GENERIC_POSIX_TYPES_H */
diff --git a/include/uapi/asm-generic/posix_types.h b/include/uapi/asm-generic/posix_types.h
index f0733a26ebfc..2715ba4599bd 100644
--- a/include/uapi/asm-generic/posix_types.h
+++ b/include/uapi/asm-generic/posix_types.h
@@ -11,10 +11,7 @@
* architectures, so that you can override them.
*/
-#ifndef __kernel_long_t
-typedef long __kernel_long_t;
-typedef unsigned long __kernel_ulong_t;
-#endif
+#include <asm/kernel_long_t.h>
#ifndef __kernel_ino_t
typedef __kernel_ulong_t __kernel_ino_t;
Additional architectures need conversion as well, but I think this
suggests where this is going. Would that be acceptable?
A different approach would rename <asm/posix_types.h> to something more
basic, exclude the two structs, and move all internal #includes which do
need the structs to the new header. A new <asm/posix_types.h> would
include the renamed header and add back the two structs, for
compatibility.
For a less strict definition of compatibility, it would also be possible
to introduce <asm/fsid_t.h> (for __kernel_fsid_t) and <linux/fd_set.h>
(for __kernel_fd_set), and remove the definition of those from
<asm/posix_types.h>.
The other question is whether this __kernel_long_t dependency in
<asm/socket.h> is even valid because it makes the constants SO_RCVTIMEO
etc. unusable in a preprocessor expression (although POSIX does not make
such a requirement as far as I can see).
Thanks,
Florian
On Mon, Jun 17, 2019 at 4:45 AM Florian Weimer <[email protected]> wrote:
>
> I wanted to introduce a new header, <asm/kernel_long_t.h>, and include
> it where the definition of __kernel_long_t is needed, something like
> this (incomplete, untested):
So this doesn't look interesting to me: __kernel_long_t is neither
interesting as a type anyway (it's just a way for user space to
override "long"), nor is it a namespace violation.
So honestly, user space could do whatever it wants for __kernel_long_t anyway.
The thing that I think we should try to fix is just the "val[]" thing, ie
> A different approach would rename <asm/posix_types.h> to something more
> basic, exclude the two structs, and move all internal #includes which do
> need the structs to the new header.
In fact, I wouldn't even rename <posix_types.h> at all, I'd just make
sure it's namespace-clean.
I _think_ the only thing causing problems is '__kernel_fsid_t' due to
that "val[]" thing, so just remove ity entirely, and add it to
<statfs.h> instead.
And yeah, then we'd need to maybe make sure that the (couple) of
__kernel_fsid_t users properly include that statfs.h file.
Hmm?
Linus
* Linus Torvalds:
>> A different approach would rename <asm/posix_types.h> to something more
>> basic, exclude the two structs, and move all internal #includes which do
>> need the structs to the new header.
>
> In fact, I wouldn't even rename <posix_types.h> at all, I'd just make
> sure it's namespace-clean.
>
> I _think_ the only thing causing problems is '__kernel_fsid_t' due to
> that "val[]" thing, so just remove ity entirely, and add it to
> <statfs.h> instead.
There's also __kernel_fd_set in <linux/posix_types.h>. I may have
lumped this up with <asm/posix_types.h>, but it has the same problem.
If it's okay to move them both to more natural places (maybe
<asm/statfs.h> and <linux/socket.h>), I think that should work well for
glibc.
However, application code may have to include additional header files.
I think the GCC/LLVM sanitizers currently get __kernel_fd_set from
<linux/posix_types.h> (but I think we discussed it before, they really
shouldn't use this type because it's misleading).
Thanks,
Florian
On Mon, Jun 17, 2019 at 11:03 AM Florian Weimer <[email protected]> wrote:
>
> There's also __kernel_fd_set in <linux/posix_types.h>. I may have
> lumped this up with <asm/posix_types.h>, but it has the same problem.
Hmm.
That one we might be able to just fix by renaming "fds_bits" to "__fds_bits".
Unlike the "val[]" thing, I don't think anybody is supposed to access
those fields directly.
Of course, "supposed to" and "does" are two very very different things ;)
Linus
On Mon, Jun 17, 2019 at 11:13 AM Linus Torvalds
<[email protected]> wrote:
>
> That one we might be able to just fix by renaming "fds_bits" to "__fds_bits".
That said, moving it to another file might be the better option anyway.
I think fd_set and friends are now supposed to be in <sys/select.h>
anyway, and the "it was in <sys/types.h>" is all legacy.
Linus
* Linus Torvalds:
> On Mon, Jun 17, 2019 at 11:03 AM Florian Weimer <[email protected]> wrote:
>>
>> There's also __kernel_fd_set in <linux/posix_types.h>. I may have
>> lumped this up with <asm/posix_types.h>, but it has the same problem.
>
> Hmm.
>
> That one we might be able to just fix by renaming "fds_bits" to "__fds_bits".
>
> Unlike the "val[]" thing, I don't think anybody is supposed to access
> those fields directly.
Well, glibc already calls it __val …
> I think fd_set and friends are now supposed to be in <sys/select.h>
> anyway, and the "it was in <sys/types.h>" is all legacy.
Do you suggest to create a <linux/select.h> header to mirror this?
Thanks,
Florian
On Mon, Jun 17, 2019 at 11:19 AM Florian Weimer <[email protected]> wrote:
> >
> > Unlike the "val[]" thing, I don't think anybody is supposed to access
> > those fields directly.
>
> Well, glibc already calls it __val …
Hmm. If user space already doesn't see the "val[]" array anyway, I
guess we could just do that in the kernel too.
Looking at the glibc headers I have for fds_bits, glibc seems to do
*both* fds_bits[] and __fds_bits[] depending on __USE_XOPEN or not.
Anyway, that all implies to me that we might as well just go the truly
mindless way, and just do the double underscores and not bother with
renaming any files.
I thought people actually might care about the "val[]" name because I
find that in documentation, but since apparently it's already not
visible to user space anyway, that can't be true.
I guess that makes the original patch acceptable, and we should just
do the same thing to fds_bits..
Linus
* Linus Torvalds:
> On Mon, Jun 17, 2019 at 11:19 AM Florian Weimer <[email protected]> wrote:
>> >
>> > Unlike the "val[]" thing, I don't think anybody is supposed to access
>> > those fields directly.
>>
>> Well, glibc already calls it __val …
>
> Hmm. If user space already doesn't see the "val[]" array anyway, I
> guess we could just do that in the kernel too.
>
> Looking at the glibc headers I have for fds_bits, glibc seems to do
> *both* fds_bits[] and __fds_bits[] depending on __USE_XOPEN or not.
>
> Anyway, that all implies to me that we might as well just go the truly
> mindless way, and just do the double underscores and not bother with
> renaming any files.
>
> I thought people actually might care about the "val[]" name because I
> find that in documentation, but since apparently it's already not
> visible to user space anyway, that can't be true.
>
> I guess that makes the original patch acceptable, and we should just
> do the same thing to fds_bits..
Hah.
I think Arnd's original patch already had both. So it's ready to go in
after all?
Thanks,
Florian