VSOCK: fix uapi/linux/vm_sockets.h incomplete types

Message ID 20170912163435.4049-1-stefanha@redhat.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • VSOCK: fix uapi/linux/vm_sockets.h incomplete types
Related show

Commit Message

Stefan Hajnoczi Sept. 12, 2017, 4:34 p.m.
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(-)

Comments

Jorgen S. Hansen Sept. 13, 2017, 7:55 a.m. | #1
> 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>
David Miller Sept. 15, 2017, 9:14 p.m. | #2
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.
Stefan Hajnoczi Sept. 18, 2017, 3:21 p.m. | #3
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.
David Miller Sept. 19, 2017, 5:38 p.m. | #4
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.
Stefan Hajnoczi Sept. 20, 2017, 3:22 p.m. | #5
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

Patch

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)];
 };