| Message ID | 20190319165123.3967889-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | uapi: avoid namespace conflict in linux/posix_types.h | expand |
From: Arnd Bergmann <arnd@arndb.de> 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 <arnd@arndb.de> Acked-by: David S. Miller <davem@davemloft.net>
* 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 joseph@codesourcery.com
* 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 <fweimer@redhat.com> 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
* Linus Torvalds: > On Fri, Jun 7, 2019 at 11:43 AM Florian Weimer <fweimer@redhat.com> 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 <fweimer@redhat.com> 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:19 AM Florian Weimer <fweimer@redhat.com> 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 <fweimer@redhat.com> 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
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. */
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/87a7hvded7.fsf@mid.deneb.enyo.de/ Link: https://lore.kernel.org/lkml/20190314173900.25454-1-paul.burton@mips.com/ Cc: Florian Weimer <fw@deneb.enyo.de> Cc: Paul Burton <pburton@wavecomp.com> Fixes: a623a7a1a567 ("y2038: fix socket.h header inclusion") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- 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(-) -- 2.20.0