uapi: avoid namespace conflict in linux/posix_types.h

Message ID 20190319165123.3967889-1-arnd@arndb.de
State New
Headers show
Series
  • uapi: avoid namespace conflict in linux/posix_types.h
Related show

Commit Message

Arnd Bergmann March 19, 2019, 4:51 p.m.
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

Comments

David Miller March 19, 2019, 9:46 p.m. | #1
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>
Florian Weimer March 19, 2019, 9:55 p.m. | #2
* 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.
Joseph Myers May 7, 2019, 10:50 p.m. | #3
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
Florian Weimer June 7, 2019, 4:28 a.m. | #4
* 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
Linus Torvalds June 7, 2019, 6:27 p.m. | #5
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
Florian Weimer June 7, 2019, 6:43 p.m. | #6
* 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
Florian Weimer June 17, 2019, 11:45 a.m. | #7
* 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
Linus Torvalds June 17, 2019, 5:49 p.m. | #8
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
Florian Weimer June 17, 2019, 6:02 p.m. | #9
* 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
Linus Torvalds June 17, 2019, 6:48 p.m. | #10
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
Florian Weimer June 18, 2019, 7:44 a.m. | #11
* 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

Patch

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.  */