* [PATCH] VSOCK: fix uapi/linux/vm_sockets.h incomplete types
@ 2017-09-12 16:34 Stefan Hajnoczi
2017-09-13 7:55 ` Jorgen S. Hansen
2017-09-15 21:14 ` David Miller
0 siblings, 2 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2017-09-12 16:34 UTC (permalink / raw)
To: netdev; +Cc: David S . Miller, Jorgen Hansen, Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] VSOCK: fix uapi/linux/vm_sockets.h incomplete types
2017-09-12 16:34 [PATCH] VSOCK: fix uapi/linux/vm_sockets.h incomplete types Stefan Hajnoczi
@ 2017-09-13 7:55 ` Jorgen S. Hansen
2017-09-15 21:14 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: Jorgen S. Hansen @ 2017-09-13 7:55 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: netdev, David S . Miller
> 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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] VSOCK: fix uapi/linux/vm_sockets.h incomplete types
2017-09-12 16:34 [PATCH] VSOCK: fix uapi/linux/vm_sockets.h incomplete types Stefan Hajnoczi
2017-09-13 7:55 ` Jorgen S. Hansen
@ 2017-09-15 21:14 ` David Miller
2017-09-18 15:21 ` Stefan Hajnoczi
1 sibling, 1 reply; 6+ messages in thread
From: David Miller @ 2017-09-15 21:14 UTC (permalink / raw)
To: stefanha; +Cc: netdev, jhansen
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] VSOCK: fix uapi/linux/vm_sockets.h incomplete types
2017-09-15 21:14 ` David Miller
@ 2017-09-18 15:21 ` Stefan Hajnoczi
2017-09-19 17:38 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2017-09-18 15:21 UTC (permalink / raw)
To: David Miller; +Cc: netdev, jhansen
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] VSOCK: fix uapi/linux/vm_sockets.h incomplete types
2017-09-18 15:21 ` Stefan Hajnoczi
@ 2017-09-19 17:38 ` David Miller
2017-09-20 15:22 ` Stefan Hajnoczi
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2017-09-19 17:38 UTC (permalink / raw)
To: stefanha; +Cc: netdev, jhansen
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] VSOCK: fix uapi/linux/vm_sockets.h incomplete types
2017-09-19 17:38 ` David Miller
@ 2017-09-20 15:22 ` Stefan Hajnoczi
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2017-09-20 15:22 UTC (permalink / raw)
To: David Miller; +Cc: netdev, jhansen
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-20 15:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-12 16:34 [PATCH] VSOCK: fix uapi/linux/vm_sockets.h incomplete types Stefan Hajnoczi
2017-09-13 7:55 ` Jorgen S. Hansen
2017-09-15 21:14 ` David Miller
2017-09-18 15:21 ` Stefan Hajnoczi
2017-09-19 17:38 ` David Miller
2017-09-20 15:22 ` Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.