uapi: fix linux/if.h userspace compilation errors

PATCH In review Dmitry V. Levin 3 participants 8 messages Feb 20, 2017 – Sep 27, 2017

Subsystems (4)

Networking Network drivers Timekeeping, clocksource core, NTP, alarmtimer Posix clocks and timers

lore

Share & tools

lore

Dmitry V. Levin

author

2017-02-20 11:58 UTC #0 lore raw

PATCH 1/1 uapi: fix linux/if.h userspace compilation errors

Include <sys/socket.h> (guarded by ifndef __KERNEL__) to fix the following linux/if.h userspace compilation errors:

/usr/include/linux/if.h:234:19: error: field 'ifru_addr' has incomplete type
struct sockaddr ifru_addr;
/usr/include/linux/if.h:235:19: error: field 'ifru_dstaddr' has incomplete type
struct sockaddr ifru_dstaddr;
/usr/include/linux/if.h:236:19: error: field 'ifru_broadaddr' has incomplete type
struct sockaddr ifru_broadaddr;
/usr/include/linux/if.h:237:19: error: field 'ifru_netmask' has incomplete type
struct sockaddr ifru_netmask;
/usr/include/linux/if.h:238:20: error: field 'ifru_hwaddr' has incomplete type
struct sockaddr ifru_hwaddr;

This also fixes userspace compilation of the following uapi headers:
linux/atmbr2684.h
linux/gsmmux.h
linux/if_arp.h
linux/if_bonding.h
linux/if_frad.h
linux/if_pppox.h
linux/if_tunnel.h
linux/netdevice.h
linux/route.h
linux/wireless.h

As no uapi header provides a definition of struct sockaddr, inclusion of <sys/socket.h> seems to be the most conservative and the only safe fix available.

All current users of <linux/if.h> are very likely to be including <sys/socket.h> already because the latter is the sole provider of struct sockaddr definition in libc, so adding a uapi header with a definition of struct sockaddr would create a potential conflict with <sys/socket.h>.

Replacing struct sockaddr in the definition of struct ifreq with a different type would create a potential incompatibility with current users of struct ifreq who might rely on ifru_addr et al members being of type struct sockaddr.

Signed-off-by Dmitry V. Levin <[email protected]>

---
 include/uapi/linux/if.h | 4 ++++
 1 file changed, 4 insertions(+)

include/uapi/linux/if.h +4 -0

index 1158a04..259617a 100644
@@ -24,6 +24,10 @@
 #include <linux/socket.h>		/* for "struct sockaddr" et al	*/
 #include <linux/compiler.h>		/* for "__user" et al           */
 
+#ifndef __KERNEL__
+#include <sys/socket.h>			/* for struct sockaddr.		*/
+#endif
+
 #if __UAPI_DEF_IF_IFNAMSIZ
 #define	IFNAMSIZ	16
 #endif /* __UAPI_DEF_IF_IFNAMSIZ */

Signature · 1 line

ldv

David Miller

maintainer

2017-02-21 17:10 UTC #1 lore raw

From: "Dmitry V. Levin" <[email protected]>
Date: Mon, 20 Feb 2017 14:58:41 +0300

Quoted 2 lines

Include <sys/socket.h> (guarded by ifndef __KERNEL__) to fix the following linux/if.h userspace compilation errors:

Wouldn't it be so much better to do this in include/uapi/linux/socket.h?

I mean, look at the comment right by the change you are making:

include/uapi/linux/if.h line 24

Quoted 2 lines

include/uapi/linux/if.h +0 -0

@@ -24,6 +24,10 @@
 #include <linux/socket.h>		/* for "struct sockaddr" et al	*/

                                           ^^^^^^^^^^^^^^^^^^^^^^^

Anyone including linux/socket.h is trying to obtain that type.

Dmitry V. Levin

author

2017-02-21 20:19 UTC #2 lore raw

Quoted 7 lines · David Miller

From: "Dmitry V. Levin" <[email protected]>
Date: Mon, 20 Feb 2017 14:58:41 +0300
Quoted 2 lines
Include <sys/socket.h> (guarded by ifndef __KERNEL__) to fix the following linux/if.h userspace compilation errors:
Wouldn't it be so much better to do this in include/uapi/linux/socket.h?

Yes, it would be nicer if we could afford it. However, changing uapi/linux/socket.h to include <sys/socket.h> is less conservative than changing every uapi header that fails to compile because of its use of struct sockaddr. It's risky because <sys/socket.h> pulls in other types that might conflict with definitions provided by uapi headers.

I've checked all uapi headers that have no compilation errors whether inclusion of <sys/socket.h> before them causes any issues, and found that linux/time.h and linux/uio.h (and their users) stop compiling because of type conflicts:

$ gcc -S -o/dev/null -xc /dev/null -include /usr/include/sys/socket.h -include /usr/include/linux/time.h
In file included from <command-line>:32:0:
/usr/include/linux/time.h:9:8: error: redefinition of 'struct timespec'
 struct timespec {
        ^~~~~~~~
In file included from /usr/include/sys/select.h:45:0,
                 from /usr/include/sys/types.h:219,
                 from /usr/include/sys/uio.h:23,
                 from /usr/include/sys/socket.h:26,
                 from <command-line>:32:
/usr/include/time.h:120:8: note: originally defined here
 struct timespec
        ^~~~~~~~
In file included from <command-line>:32:0:
/usr/include/linux/time.h:15:8: error: redefinition of 'struct timeval'
 struct timeval {
        ^~~~~~~
In file included from /usr/include/sys/select.h:47:0,
                 from /usr/include/sys/types.h:219,
                 from /usr/include/sys/uio.h:23,
                 from /usr/include/sys/socket.h:26,
                 from <command-line>:32:
/usr/include/bits/time.h:30:8: note: originally defined here
 struct timeval
        ^~~~~~~
$ gcc -S -o/dev/null -xc /dev/null -include /usr/include/sys/socket.h -include /usr/include/linux/uio.h
In file included from <command-line>:32:0:
/usr/include/linux/uio.h:16:8: error: redefinition of 'struct iovec'
 struct iovec
        ^~~~~
In file included from /usr/include/sys/uio.h:28:0,
                 from /usr/include/sys/socket.h:26,
                 from <command-line>:32:
/usr/include/bits/uio.h:43:8: note: originally defined here
 struct iovec
        ^~~~~

We can try to workaround these ancient <time.h> against <linux/time.h> and <sys/uio.h> against <linux/uio.h> conflicts on uapi side by introducing __UAPI_DEF_TIMESPEC, __UAPI_DEF_TIMEVAL, and __UAPI_DEF_IOVEC guards, but these workarounds will work only one way (when libc headers are included before uapi ones) until glibc catches up, and the latter may take a lot of time.

Quoted 6 lines

I mean, look at the comment right by the change you are making:
include/uapi/linux/if.h line 24
Quoted 2 lines
include/uapi/linux/if.h +0 -0
@@ -24,6 +24,10 @@
 #include <linux/socket.h>		/* for "struct sockaddr" et al	*/
Anyone including linux/socket.h is trying to obtain that type.

I think this comment is true for #ifdef __KERNEL__ code only.

Signature · 1 line

ldv

Dmitry V. Levin

author

2017-02-22 02:22 UTC #3 lore raw

Quoted 8 lines · Dmitry V. Levin

Quoted 7 lines · David Miller
From: "Dmitry V. Levin" <[email protected]>
Date: Mon, 20 Feb 2017 14:58:41 +0300
Quoted 2 lines
Include <sys/socket.h> (guarded by ifndef __KERNEL__) to fix the following linux/if.h userspace compilation errors:
Wouldn't it be so much better to do this in include/uapi/linux/socket.h?

[...]

Quoted 6 lines

We can try to workaround these ancient <time.h> against <linux/time.h> and <sys/uio.h> against <linux/uio.h> conflicts on uapi side by introducing __UAPI_DEF_TIMESPEC, __UAPI_DEF_TIMEVAL, and __UAPI_DEF_IOVEC guards, but these workarounds will work only one way (when libc headers are included before uapi ones) until glibc catches up, and the latter may take a lot of time.

OK, these conflicts need to be fixed regardless of the way how struct sockaddr related uapi compilation errors are fixed. I'll submit these fixes shortly.

Signature · 1 line

ldv

Dmitry V. Levin

author

2017-02-22 02:29 UTC #4 lore raw

PATCH 1/2 uapi: add a compatibility layer between linux/time.h and glibc

Do not define struct itimerspec, struct timespec, struct timeval, and TIMER_ABSTIME in linux/time.h when <time.h> is already included and provides these definitions.

This fixes the following compilation errors and warnings when <time.h> is included before <linux/time.h>:

Do not define struct itimerval, struct timespec, struct timeval, struct timezone, ITIMER_REAL, ITIMER_VIRTUAL, and ITIMER_PROF in linux/time.h when <sys/time.h> is already included and provides these definitions.

This fixes the following compilation errors and warnings when <sys/time.h> is included before <linux/time.h>:

/usr/include/linux/time.h:9:8: error: redefinition of 'struct timespec' /usr/include/linux/time.h:15:8: error: redefinition of 'struct timeval' /usr/include/linux/time.h:20:8: error: redefinition of 'struct timezone' /usr/include/linux/time.h:30:0: warning: "ITIMER_REAL" redefined /usr/include/linux/time.h:31:0: warning: "ITIMER_VIRTUAL" redefined /usr/include/linux/time.h:32:0: warning: "ITIMER_PROF" redefined /usr/include/linux/time.h:39:8: error: redefinition of 'struct itimerval'

Signed-off-by Dmitry V. Levin <[email protected]>

---
 include/uapi/linux/libc-compat.h | 56 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/time.h        | 31 ++++++++++++++++++++--
 2 files changed, 85 insertions(+), 2 deletions(-)

include/uapi/linux/libc-compat.h +56 -0

index 44b8a6b..481e3b1 100644
@@ -158,6 +158,53 @@
 
 #endif /* defined(__NETIPX_IPX_H) */
 
+/* Coordinate with glibc time.h header. */
+#if (defined(__itimerspec_defined) && __itimerspec_defined) || \
+    (defined(_TIME_H) && defined(__USE_POSIX199309))
+/* __itimerspec_defined was introduced in glibc-2.25 */
+#define __UAPI_DEF_ITIMERSPEC		0
+#else
+#define __UAPI_DEF_ITIMERSPEC		1
+#endif
+
+#if (defined(__timespec_defined) && __timespec_defined) || \
+    (defined(_STRUCT_TIMESPEC) && _STRUCT_TIMESPEC)
+/* __timespec_defined was introduced in glibc-2.3.2 */
+#define __UAPI_DEF_TIMESPEC		0
+#else
+#define __UAPI_DEF_TIMESPEC		1
+#endif
+
+#if (defined(__timeval_defined) && __timeval_defined) || \
+    (defined(_STRUCT_TIMEVAL) && _STRUCT_TIMEVAL)
+/* __timeval_defined was introduced in glibc-2.25 */
+#define __UAPI_DEF_TIMEVAL		0
+#else
+#define __UAPI_DEF_TIMEVAL		1
+#endif
+
+/* Coordinate with glibc bits/time.h header. */
+#if defined(_BITS_TIME_H) && defined(__USE_POSIX199309)
+#define __UAPI_DEF_TIMER_ABSTIME	0
+#else
+#define __UAPI_DEF_TIMER_ABSTIME	1
+#endif
+
+/* Coordinate with glibc sys/time.h header. */
+#if defined(_SYS_TIME_H)
+#define __UAPI_DEF_ITIMERVAL			0
+#define __UAPI_DEF_ITIMER_REAL_VIRTUAL_PROF	0
+#else
+#define __UAPI_DEF_ITIMERVAL			1
+#define __UAPI_DEF_ITIMER_REAL_VIRTUAL_PROF	1
+#endif
+
+#if defined(_SYS_TIME_H) && defined(__USE_MISC)
+#define __UAPI_DEF_TIMEZONE		0
+#else
+#define __UAPI_DEF_TIMEZONE		1
+#endif
+
 /* Definitions for xattr.h */
 #if defined(_SYS_XATTR_H)
 #define __UAPI_DEF_XATTR		0

@@ -205,6 +252,15 @@
 #define __UAPI_DEF_IPX_CONFIG_DATA		1
 #define __UAPI_DEF_IPX_ROUTE_DEF		1
 
+/* Definitions for time.h */
+#define __UAPI_DEF_ITIMERSPEC			1
+#define __UAPI_DEF_ITIMERVAL			1
+#define __UAPI_DEF_ITIMER_REAL_VIRTUAL_PROF	1
+#define __UAPI_DEF_TIMER_ABSTIME		1
+#define __UAPI_DEF_TIMESPEC			1
+#define __UAPI_DEF_TIMEVAL			1
+#define __UAPI_DEF_TIMEZONE			1
+
 /* Definitions for xattr.h */
 #define __UAPI_DEF_XATTR		1

include/uapi/linux/time.h +29 -2

index e75e1b6..9d03ca4 100644
@@ -1,45 +1,70 @@
 #ifndef _UAPI_LINUX_TIME_H
 #define _UAPI_LINUX_TIME_H
 
+#include <linux/libc-compat.h>
 #include <linux/types.h>
 
 
+#if __UAPI_DEF_TIMESPEC
+#ifndef __timespec_defined
+#define __timespec_defined	1
+#endif
 #ifndef _STRUCT_TIMESPEC
-#define _STRUCT_TIMESPEC
+#define _STRUCT_TIMESPEC	1
+#endif
 struct timespec {
 	__kernel_time_t	tv_sec;			/* seconds */
 	long		tv_nsec;		/* nanoseconds */
 };
-#endif
+#endif /* __UAPI_DEF_TIMESPEC */
 
+#if __UAPI_DEF_TIMEVAL
+#ifndef __timeval_defined
+#define __timeval_defined	1
+#endif
+#ifndef _STRUCT_TIMEVAL
+#define _STRUCT_TIMEVAL		1
+#endif
 struct timeval {
 	__kernel_time_t		tv_sec;		/* seconds */
 	__kernel_suseconds_t	tv_usec;	/* microseconds */
 };
+#endif /* __UAPI_DEF_TIMEVAL */
 
+#if __UAPI_DEF_TIMEZONE
 struct timezone {
 	int	tz_minuteswest;	/* minutes west of Greenwich */
 	int	tz_dsttime;	/* type of dst correction */
 };
+#endif /* __UAPI_DEF_TIMEZONE */
 
 
 /*
  * Names of the interval timers, and structure
  * defining a timer setting:
  */
+#if __UAPI_DEF_ITIMER_REAL_VIRTUAL_PROF
 #define	ITIMER_REAL		0
 #define	ITIMER_VIRTUAL		1
 #define	ITIMER_PROF		2
+#endif /* __UAPI_DEF_ITIMER_REAL_VIRTUAL_PROF */
 
+#if __UAPI_DEF_ITIMERSPEC
+#ifndef __itimerspec_defined
+#define __itimerspec_defined	1
+#endif
 struct itimerspec {
 	struct timespec it_interval;	/* timer period */
 	struct timespec it_value;	/* timer expiration */
 };
+#endif /* __UAPI_DEF_ITIMERSPEC */
 
+#if __UAPI_DEF_ITIMERVAL
 struct itimerval {
 	struct timeval it_interval;	/* timer interval */
 	struct timeval it_value;	/* current value */
 };
+#endif /* __UAPI_DEF_ITIMERVAL */
 
 /*
  * The IDs of the various system clocks (for POSIX.1b interval timers):

@@ -64,6 +89,8 @@ struct itimerval {
 /*
  * The various flags for setting POSIX.1b interval timers:
  */
+#if __UAPI_DEF_TIMER_ABSTIME
 #define TIMER_ABSTIME			0x01
+#endif
 
 #endif /* _UAPI_LINUX_TIME_H */

Signature · 1 line

ldv

Dmitry V. Levin

author

2017-02-22 02:29 UTC #5 lore raw

PATCH 2/2 uapi: add a compatibility layer between linux/uio.h and glibc

In reply to Dmitry V. Levin

Do not define struct iovec in linux/uio.h when <sys/uio.h> or <fcntl.h> is already included and provides these definitions.

This fixes the following compilation error when <sys/uio.h> or <fcntl.h> is included before <linux/uio.h>:

/usr/include/linux/uio.h:16:8: error: redefinition of 'struct iovec'

Signed-off-by Dmitry V. Levin <[email protected]>

---
 include/uapi/linux/libc-compat.h | 10 ++++++++++
 include/uapi/linux/uio.h         |  3 +++
 2 files changed, 13 insertions(+)

include/uapi/linux/libc-compat.h +10 -0

index 481e3b1..9b88586 100644
@@ -205,6 +205,13 @@
 #define __UAPI_DEF_TIMEZONE		1
 #endif
 
+/* Coordinate with glibc bits/uio.h header. */
+#if defined(_SYS_UIO_H) || defined(_FCNTL_H)
+#define __UAPI_DEF_IOVEC		0
+#else
+#define __UAPI_DEF_IOVEC		1
+#endif
+
 /* Definitions for xattr.h */
 #if defined(_SYS_XATTR_H)
 #define __UAPI_DEF_XATTR		0

@@ -261,6 +268,9 @@
 #define __UAPI_DEF_TIMEVAL			1
 #define __UAPI_DEF_TIMEZONE			1
 
+/* Definitions for uio.h */
+#define __UAPI_DEF_IOVEC		1
+
 /* Definitions for xattr.h */
 #define __UAPI_DEF_XATTR		1

include/uapi/linux/uio.h +3 -0

index 2731d56..e6e12cf 100644
@@ -9,15 +9,18 @@
 #ifndef _UAPI__LINUX_UIO_H
 #define _UAPI__LINUX_UIO_H
 
+#include <linux/libc-compat.h>
 #include <linux/compiler.h>
 #include <linux/types.h>
 
 
+#if __UAPI_DEF_IOVEC
 struct iovec
 {
 	void __user *iov_base;	/* BSD uses caddr_t (1003.1g requires void *) */
 	__kernel_size_t iov_len; /* Must be size_t (1003.1g) */
 };
+#endif /* __UAPI_DEF_IOVEC */
 
 /*
  *	UIO_MAXIOV shall be at least 16 1003.1g (5.4.1.1)

Signature · 1 line

ldv

David Miller

maintainer

2017-02-22 21:09 UTC #6 lore raw

In reply to Dmitry V. Levin

From: "Dmitry V. Levin" <[email protected]>
Date: Tue, 21 Feb 2017 23:19:14 +0300

Quoted 7 lines · David Miller

Quoted 6 lines
From: "Dmitry V. Levin" <[email protected]>
Date: Mon, 20 Feb 2017 14:58:41 +0300
Quoted 2 lines
Include <sys/socket.h> (guarded by ifndef __KERNEL__) to fix the following linux/if.h userspace compilation errors:

Quoted 7 lines

Quoted 1 line
Wouldn't it be so much better to do this in include/uapi/linux/socket.h?
Yes, it would be nicer if we could afford it. However, changing uapi/linux/socket.h to include <sys/socket.h> is less conservative than changing every uapi header that fails to compile because of its use of struct sockaddr. It's risky because <sys/socket.h> pulls in other types that might conflict with definitions provided by uapi headers.

Ok, I'll apply this for now.

Lee Duncan

2017-09-27 16:46 UTC #7 lore raw

Re: [PATCH 2/2] uapi: add a compatibility layer between linux/uio.h and glibc

In reply to Dmitry V. Levin

Ping? I never saw any response to this.

Quoted 67 lines · Dmitry V Levin

Do not define struct iovec in linux/uio.h when <sys/uio.h> or <fcntl.h> is already included and provides these definitions.

This fixes the following compilation error when <sys/uio.h> or <fcntl.h> is included before <linux/uio.h>:

/usr/include/linux/uio.h:16:8: error: redefinition of 'struct iovec'

Signed-off-by Dmitry V. Levin <[email protected]>

---
 include/uapi/linux/libc-compat.h | 10 ++++++++++
 include/uapi/linux/uio.h         |  3 +++
 2 files changed, 13 insertions(+)

include/uapi/linux/libc-compat.h +10 -0

index 481e3b1..9b88586 100644
@@ -205,6 +205,13 @@
 #define __UAPI_DEF_TIMEZONE		1
 #endif
 
+/* Coordinate with glibc bits/uio.h header. */
+#if defined(_SYS_UIO_H) || defined(_FCNTL_H)
+#define __UAPI_DEF_IOVEC		0
+#else
+#define __UAPI_DEF_IOVEC		1
+#endif
+
 /* Definitions for xattr.h */
 #if defined(_SYS_XATTR_H)
 #define __UAPI_DEF_XATTR		0

@@ -261,6 +268,9 @@
 #define __UAPI_DEF_TIMEVAL			1
 #define __UAPI_DEF_TIMEZONE			1
 
+/* Definitions for uio.h */
+#define __UAPI_DEF_IOVEC		1
+
 /* Definitions for xattr.h */
 #define __UAPI_DEF_XATTR		1

include/uapi/linux/uio.h +3 -0

index 2731d56..e6e12cf 100644
@@ -9,15 +9,18 @@
 #ifndef _UAPI__LINUX_UIO_H
 #define _UAPI__LINUX_UIO_H
 
+#include <linux/libc-compat.h>
 #include <linux/compiler.h>
 #include <linux/types.h>
 
 
+#if __UAPI_DEF_IOVEC
 struct iovec
 {
 	void __user *iov_base;	/* BSD uses caddr_t (1003.1g requires void *) */
 	__kernel_size_t iov_len; /* Must be size_t (1003.1g) */
 };
+#endif /* __UAPI_DEF_IOVEC */
 
 /*
  *	UIO_MAXIOV shall be at least 16 1003.1g (5.4.1.1)

Signature · 1 line

ldv

Signature · 2 lines

Lee Duncan
SUSE Labs