| Message ID | 20170912163435.4049-1-stefanha@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | VSOCK: fix uapi/linux/vm_sockets.h incomplete types | expand |
> On Sep 12, 2017, at 6:34 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > This patch fixes the following compiler errors when userspace > applications use the vm_sockets.h header: > > include/uapi/linux/vm_sockets.h:148:32: error: invalid application of ‘sizeof’ to incomplete type ‘struct sockaddr’ > unsigned char svm_zero[sizeof(struct sockaddr) - > ^~~~~~ > include/uapi/linux/vm_sockets.h:149:18: error: ‘sa_family_t’ undeclared here (not in a function) > sizeof(sa_family_t) - > ^~~~~~~~~~~ > > Two issues: > 1. In the kernel struct sockaddr comes in via <linux/socket.h> but in > userspace <sys/socket.h> is required. > 2. struct sockaddr_vm has a __kernel_sa_family_t field so let's be > consistent and use the same type for the sizeof(sa_family_t) > calculation. > > Currently userspace applications work around this broken header by first > including <sys/socket.h>. In the kernel there is no compiler error > because <linux/socket.h> provides everything. It's worth fixing the > header file though. > > Cc: Jorgen Hansen <jhansen@vmware.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > include/uapi/linux/vm_sockets.h | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h > index b4ed5d895699..4ae5c625ac56 100644 > --- a/include/uapi/linux/vm_sockets.h > +++ b/include/uapi/linux/vm_sockets.h > @@ -18,6 +18,10 @@ > > #include <linux/socket.h> > > +#ifndef __KERNEL__ > +#include <sys/socket.h> /* struct sockaddr */ > +#endif > + > /* Option name for STREAM socket buffer size. Use as the option name in > * setsockopt(3) or getsockopt(3) to set or get an unsigned long long that > * specifies the size of the buffer underlying a vSockets STREAM socket. > @@ -146,7 +150,7 @@ struct sockaddr_vm { > unsigned int svm_port; > unsigned int svm_cid; > unsigned char svm_zero[sizeof(struct sockaddr) - > - sizeof(sa_family_t) - > + sizeof(__kernel_sa_family_t) - > sizeof(unsigned short) - > sizeof(unsigned int) - sizeof(unsigned int)]; > }; > -- > 2.13.5 > Thanks for fixing this. Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
From: Stefan Hajnoczi <stefanha@redhat.com> Date: Tue, 12 Sep 2017 17:34:35 +0100 > This patch fixes the following compiler errors when userspace > applications use the vm_sockets.h header: > > include/uapi/linux/vm_sockets.h:148:32: error: invalid application of ‘sizeof’ to incomplete type ‘struct sockaddr’ > unsigned char svm_zero[sizeof(struct sockaddr) - > ^~~~~~ > include/uapi/linux/vm_sockets.h:149:18: error: ‘sa_family_t’ undeclared here (not in a function) > sizeof(sa_family_t) - > ^~~~~~~~~~~ > > Two issues: > 1. In the kernel struct sockaddr comes in via <linux/socket.h> but in > userspace <sys/socket.h> is required. > 2. struct sockaddr_vm has a __kernel_sa_family_t field so let's be > consistent and use the same type for the sizeof(sa_family_t) > calculation. > > Currently userspace applications work around this broken header by first > including <sys/socket.h>. In the kernel there is no compiler error > because <linux/socket.h> provides everything. It's worth fixing the > header file though. > > Cc: Jorgen Hansen <jhansen@vmware.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > include/uapi/linux/vm_sockets.h | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h > index b4ed5d895699..4ae5c625ac56 100644 > --- a/include/uapi/linux/vm_sockets.h > +++ b/include/uapi/linux/vm_sockets.h > @@ -18,6 +18,10 @@ > > #include <linux/socket.h> > > +#ifndef __KERNEL__ > +#include <sys/socket.h> /* struct sockaddr */ > +#endif > + There is no precedence whatsoever to include sys/socket.h in _any_ UAPI header file provided by the kernel. __kernel_sa_family_t is what should be used in UAPI headers, only non-UAPI headers can use plain sa_family_t. So that is the correct fix for this problem. Thank you.
On Fri, Sep 15, 2017 at 02:14:32PM -0700, David Miller wrote: > > diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h > > index b4ed5d895699..4ae5c625ac56 100644 > > --- a/include/uapi/linux/vm_sockets.h > > +++ b/include/uapi/linux/vm_sockets.h > > @@ -18,6 +18,10 @@ > > > > #include <linux/socket.h> > > > > +#ifndef __KERNEL__ > > +#include <sys/socket.h> /* struct sockaddr */ > > +#endif > > + > > There is no precedence whatsoever to include sys/socket.h in _any_ UAPI > header file provided by the kernel. <linux/if.h> does it for the same reason: include/uapi/linux/if.h:#include <sys/socket.h> /* for struct sockaddr. */ If that is not acceptable for <linux/vm_sockets.h> then we could do the same thing as <linux/in.h>: #define __SOCK_SIZE__ 16 /* sizeof(struct sockaddr) */ Is that okay or do you have another suggestion for getting sizeof(struct sockaddr)? It's needed to pad struct sockaddr_vm. > __kernel_sa_family_t is what should be used in UAPI headers, only > non-UAPI headers can use plain sa_family_t. > > So that is the correct fix for this problem. Thanks, will fix in v2.
From: Stefan Hajnoczi <stefanha@redhat.com> Date: Mon, 18 Sep 2017 16:21:00 +0100 > On Fri, Sep 15, 2017 at 02:14:32PM -0700, David Miller wrote: >> > diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h >> > index b4ed5d895699..4ae5c625ac56 100644 >> > --- a/include/uapi/linux/vm_sockets.h >> > +++ b/include/uapi/linux/vm_sockets.h >> > @@ -18,6 +18,10 @@ >> > >> > #include <linux/socket.h> >> > >> > +#ifndef __KERNEL__ >> > +#include <sys/socket.h> /* struct sockaddr */ >> > +#endif >> > + >> >> There is no precedence whatsoever to include sys/socket.h in _any_ UAPI >> header file provided by the kernel. > > <linux/if.h> does it for the same reason: > > include/uapi/linux/if.h:#include <sys/socket.h> /* for struct sockaddr. */ You don't need it for struct sockaddr, you need it for sa_family_t, the comment is very misleading. Please do as I have instructed and it will fix this problem. Thank you.
On Tue, Sep 19, 2017 at 10:38:40AM -0700, David Miller wrote: > From: Stefan Hajnoczi <stefanha@redhat.com> > Date: Mon, 18 Sep 2017 16:21:00 +0100 > > > On Fri, Sep 15, 2017 at 02:14:32PM -0700, David Miller wrote: > >> > diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h > >> > index b4ed5d895699..4ae5c625ac56 100644 > >> > --- a/include/uapi/linux/vm_sockets.h > >> > +++ b/include/uapi/linux/vm_sockets.h > >> > @@ -18,6 +18,10 @@ > >> > > >> > #include <linux/socket.h> > >> > > >> > +#ifndef __KERNEL__ > >> > +#include <sys/socket.h> /* struct sockaddr */ > >> > +#endif > >> > + > >> > >> There is no precedence whatsoever to include sys/socket.h in _any_ UAPI > >> header file provided by the kernel. > > > > <linux/if.h> does it for the same reason: > > > > include/uapi/linux/if.h:#include <sys/socket.h> /* for struct sockaddr. */ > > You don't need it for struct sockaddr, you need it for sa_family_t, > the comment is very misleading. > > Please do as I have instructed and it will fix this problem. No, you really cannot rely on struct sockaddr from <linux/socket.h> in uapi headers. You can check this yourself: $ cd /tmp && gcc -o a.o -c /usr/include/linux/vm_sockets.h /usr/include/linux/vm_sockets.h:148:32: error: invalid application of ‘sizeof’ to incomplete type ‘struct sockaddr’ unsigned char svm_zero[sizeof(struct sockaddr) - ^~~~~~ The weird situation is: 1. When compiling the kernel, <linux/socket.h> brings in struct sockaddr because the compiler finds include/linux/socket.h first before include/uapi/linux/socket.h. 2. When compiling a userspace application, <linux/socket.h> does not bring in struct sockaddr because include/uapi/linux/socket.h is found. This is why I added the #include <sys/socket> when !__KERNEL__. Sorry that the commit description wasn't clear on this. Am I misunderstanding something? Stefan
diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h index b4ed5d895699..4ae5c625ac56 100644 --- a/include/uapi/linux/vm_sockets.h +++ b/include/uapi/linux/vm_sockets.h @@ -18,6 +18,10 @@ #include <linux/socket.h> +#ifndef __KERNEL__ +#include <sys/socket.h> /* struct sockaddr */ +#endif + /* Option name for STREAM socket buffer size. Use as the option name in * setsockopt(3) or getsockopt(3) to set or get an unsigned long long that * specifies the size of the buffer underlying a vSockets STREAM socket. @@ -146,7 +150,7 @@ struct sockaddr_vm { unsigned int svm_port; unsigned int svm_cid; unsigned char svm_zero[sizeof(struct sockaddr) - - sizeof(sa_family_t) - + sizeof(__kernel_sa_family_t) - sizeof(unsigned short) - sizeof(unsigned int) - sizeof(unsigned int)]; };
This patch fixes the following compiler errors when userspace applications use the vm_sockets.h header: include/uapi/linux/vm_sockets.h:148:32: error: invalid application of ‘sizeof’ to incomplete type ‘struct sockaddr’ unsigned char svm_zero[sizeof(struct sockaddr) - ^~~~~~ include/uapi/linux/vm_sockets.h:149:18: error: ‘sa_family_t’ undeclared here (not in a function) sizeof(sa_family_t) - ^~~~~~~~~~~ Two issues: 1. In the kernel struct sockaddr comes in via <linux/socket.h> but in userspace <sys/socket.h> is required. 2. struct sockaddr_vm has a __kernel_sa_family_t field so let's be consistent and use the same type for the sizeof(sa_family_t) calculation. Currently userspace applications work around this broken header by first including <sys/socket.h>. In the kernel there is no compiler error because <linux/socket.h> provides everything. It's worth fixing the header file though. Cc: Jorgen Hansen <jhansen@vmware.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- include/uapi/linux/vm_sockets.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)