[Qemu-devel] [PATCH v3] linux-user: syscall: Add SO_LINGER for setsockopt

[Qemu-devel] [PATCH v3] linux-user: syscall: Add SO_LINGER for setsockopt

[Carlo Marcelo Arenas Belón]

Original implementation from Chen Gang; code moved around as per v2

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 linux-user/syscall.c      | 16 ++++++++++++++++
 linux-user/syscall_defs.h |  5 +++++
 2 files changed, 21 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 9b6364a266..d3c500ca78 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2829,6 +2829,8 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,
     int val;
     struct ip_mreqn *ip_mreq;
     struct ip_mreq_source *ip_mreq_source;
+    struct linger lg;
+    struct target_linger *tlg;
 
     switch(level) {
     case SOL_TCP:
@@ -3071,6 +3073,20 @@ set_timeout:
 		unlock_user (dev_ifname, optval_addr, 0);
 		return ret;
 	}
+        case TARGET_SO_LINGER:
+                optname = SO_LINGER;
+                if (optlen != sizeof(struct target_linger)) {
+                    return -TARGET_EINVAL;
+                }
+                if (!lock_user_struct(VERIFY_READ, tlg, optval_addr, 1)) {
+                    return -TARGET_EFAULT;
+                }
+                __get_user(lg.l_onoff, &tlg->l_onoff);
+                __get_user(lg.l_linger, &tlg->l_linger);
+                ret = get_errno(setsockopt(sockfd, SOL_SOCKET, optname,
+                                &lg, sizeof(lg)));
+                unlock_user_struct(tlg, optval_addr, 0);
+                return ret;
             /* Options with 'int' argument.  */
         case TARGET_SO_DEBUG:
 		optname = SO_DEBUG;
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 40c5027e93..a60d6bb163 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -202,6 +202,11 @@ struct target_ip_mreq_source {
     uint32_t imr_sourceaddr;
 };
 
+struct target_linger {
+    abi_int l_onoff;        /* Linger active                */
+    abi_int l_linger;       /* How long to linger for       */
+};
+
 struct target_timeval {
     abi_long tv_sec;
     abi_long tv_usec;
-- 
2.11.0 (Apple Git-81)

Re: [Qemu-devel] [PATCH v3] linux-user: syscall: Add SO_LINGER for setsockopt

[Laurent Vivier]

Le 19/09/2017 à 10:15, Carlo Marcelo Arenas Belón a écrit :
> Original implementation from Chen Gang; code moved around as per v2
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  linux-user/syscall.c      | 16 ++++++++++++++++
>  linux-user/syscall_defs.h |  5 +++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 9b6364a266..d3c500ca78 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -2829,6 +2829,8 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,
>      int val;
>      struct ip_mreqn *ip_mreq;
>      struct ip_mreq_source *ip_mreq_source;
> +    struct linger lg;
> +    struct target_linger *tlg;
>  
>      switch(level) {
>      case SOL_TCP:
> @@ -3071,6 +3073,20 @@ set_timeout:
>  		unlock_user (dev_ifname, optval_addr, 0);
>  		return ret;
>  	}
> +        case TARGET_SO_LINGER:
> +                optname = SO_LINGER;
> +                if (optlen != sizeof(struct target_linger)) {
> +                    return -TARGET_EINVAL;
> +                }
> +                if (!lock_user_struct(VERIFY_READ, tlg, optval_addr, 1)) {
> +                    return -TARGET_EFAULT;
> +                }
> +                __get_user(lg.l_onoff, &tlg->l_onoff);
> +                __get_user(lg.l_linger, &tlg->l_linger);
> +                ret = get_errno(setsockopt(sockfd, SOL_SOCKET, optname,
> +                                &lg, sizeof(lg)));
> +                unlock_user_struct(tlg, optval_addr, 0);
> +                return ret;
>              /* Options with 'int' argument.  */

It looks good.

Could you also add it for getsockopt()?

>          case TARGET_SO_DEBUG:
>  		optname = SO_DEBUG;
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 40c5027e93..a60d6bb163 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -202,6 +202,11 @@ struct target_ip_mreq_source {
>      uint32_t imr_sourceaddr;
>  };
>  
> +struct target_linger {
> +    abi_int l_onoff;        /* Linger active                */
> +    abi_int l_linger;       /* How long to linger for       */
> +};
> +
>  struct target_timeval {
>      abi_long tv_sec;
>      abi_long tv_usec;
> 

Could you update the definition of TARGET_SO_LINGER for sparc (kernel
says 0x0080, and we use 13)

Thanks,
Laurent

[Qemu-devel] [PATCH 1/3] linux-user: fix TARGET_SO_LINGER for sparc

[Carlo Marcelo Arenas Belón]

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 linux-user/socket.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/linux-user/socket.h b/linux-user/socket.h
index 7051cd2cf4..129f9b4713 100644
--- a/linux-user/socket.h
+++ b/linux-user/socket.h
@@ -246,8 +246,12 @@
     #define TARGET_SOCK_MAX (TARGET_SOCK_PACKET + 1)
     #define TARGET_SOCK_TYPE_MASK    0xf  /* Covers up to TARGET_SOCK_MAX-1. */
 
+    #define TARGET_SO_LINGER         0x0080
+
     #define TARGET_SO_PASSSEC        31
 #else
+    #define TARGET_SO_LINGER         13
+
     #define TARGET_SO_PASSSEC        34
 #endif
 
@@ -268,7 +272,7 @@
     #define TARGET_SO_OOBINLINE    10
     #define TARGET_SO_NO_CHECK     11
     #define TARGET_SO_PRIORITY     12
-    #define TARGET_SO_LINGER       13
+
     #define TARGET_SO_BSDCOMPAT    14
     /* To add :#define TARGET_SO_REUSEPORT 15 */
 #if defined(TARGET_PPC)
-- 
2.14.1

[Qemu-devel] [PATCH 2/3] linux-user: add SO_LINGER to setsockopt

[Carlo Marcelo Arenas Belón]

Original implementation by Chen Gang; all bugs mine

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 linux-user/syscall.c      | 15 +++++++++++++++
 linux-user/syscall_defs.h |  5 +++++
 2 files changed, 20 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 9b6364a266..ad689dad50 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3071,6 +3071,21 @@ set_timeout:
 		unlock_user (dev_ifname, optval_addr, 0);
 		return ret;
 	}
+        case TARGET_SO_LINGER:
+        {
+                struct linger lg;
+                struct target_linger *tlg;
+
+                if (!lock_user_struct(VERIFY_READ, tlg, optval_addr, 1)) {
+                    return -TARGET_EFAULT;
+                }
+                __get_user(lg.l_onoff, &tlg->l_onoff);
+                __get_user(lg.l_linger, &tlg->l_linger);
+                ret = get_errno(setsockopt(sockfd, SOL_SOCKET, SO_LINGER,
+                                &lg, optlen));
+                unlock_user_struct(tlg, optval_addr, 0);
+                return ret;
+        }
             /* Options with 'int' argument.  */
         case TARGET_SO_DEBUG:
 		optname = SO_DEBUG;
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 40c5027e93..a60d6bb163 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -202,6 +202,11 @@ struct target_ip_mreq_source {
     uint32_t imr_sourceaddr;
 };
 
+struct target_linger {
+    abi_int l_onoff;        /* Linger active                */
+    abi_int l_linger;       /* How long to linger for       */
+};
+
 struct target_timeval {
     abi_long tv_sec;
     abi_long tv_usec;
-- 
2.14.1

[Qemu-devel] [PATCH 3/3] linux-user: add SO_LINGER to getsockopt

[Carlo Marcelo Arenas Belón]

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 linux-user/syscall.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index ad689dad50..91bd27c63a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3178,7 +3178,6 @@ static abi_long do_getsockopt(int sockfd, int level, int optname,
         level = SOL_SOCKET;
         switch (optname) {
         /* These don't just return a single integer */
-        case TARGET_SO_LINGER:
         case TARGET_SO_RCVTIMEO:
         case TARGET_SO_SNDTIMEO:
         case TARGET_SO_PEERNAME:
@@ -3216,6 +3215,39 @@ static abi_long do_getsockopt(int sockfd, int level, int optname,
             }
             break;
         }
+        case TARGET_SO_LINGER:
+        {
+            struct linger lg;
+            socklen_t lglen;
+            struct target_linger *tlg;
+
+            if (get_user_u32(len, optlen)) {
+                return -TARGET_EFAULT;
+            }
+            if (len < 0) {
+                return -TARGET_EINVAL;
+            }
+
+            lglen = sizeof(lg);
+            ret = get_errno(getsockopt(sockfd, level, SO_LINGER,
+                                       &lg, &lglen));
+            if (ret < 0) {
+                return ret;
+            }
+            if (len > lglen) {
+                len = lglen;
+            }
+            if (!lock_user_struct(VERIFY_WRITE, tlg, optval_addr, 0)) {
+                return -TARGET_EFAULT;
+            }
+            __put_user(lg.l_onoff, &tlg->l_onoff);
+            __put_user(lg.l_linger, &tlg->l_linger);
+            unlock_user_struct(tlg, optval_addr, 1);
+            if (put_user_u32(len, optlen)) {
+                return -TARGET_EFAULT;
+            }
+            break;
+        }
         /* Options with 'int' argument.  */
         case TARGET_SO_DEBUG:
             optname = SO_DEBUG;
-- 
2.14.1

Re: [Qemu-devel] [PATCH 1/3] linux-user: fix TARGET_SO_LINGER for sparc

[Laurent Vivier]

Le 20/09/2017 à 01:06, Carlo Marcelo Arenas Belón a écrit :
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  linux-user/socket.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/socket.h b/linux-user/socket.h
> index 7051cd2cf4..129f9b4713 100644
> --- a/linux-user/socket.h
> +++ b/linux-user/socket.h
> @@ -246,8 +246,12 @@
>      #define TARGET_SOCK_MAX (TARGET_SOCK_PACKET + 1)
>      #define TARGET_SOCK_TYPE_MASK    0xf  /* Covers up to TARGET_SOCK_MAX-1. */
>  
> +    #define TARGET_SO_LINGER         0x0080
> +
>      #define TARGET_SO_PASSSEC        31
>  #else
> +    #define TARGET_SO_LINGER         13
> +
>      #define TARGET_SO_PASSSEC        34
>  #endif


The change is correct but it should be added below, in the "For
setsockopt(2)" section, adding a TARGET_SPARC section, like we have a
TARGET_PPC section.

>  
> @@ -268,7 +272,7 @@
>      #define TARGET_SO_OOBINLINE    10
>      #define TARGET_SO_NO_CHECK     11
>      #define TARGET_SO_PRIORITY     12
> -    #define TARGET_SO_LINGER       13
> +

Don't add a blank line

>      #define TARGET_SO_BSDCOMPAT    14
>      /* To add :#define TARGET_SO_REUSEPORT 15 */
>  #if defined(TARGET_PPC)
> 

But a better change would be to move all socket defines to a new file
linux-user/sparc/sockbits.h (like for TARGET_HPPA).

Laurent

Re: [Qemu-devel] [PATCH 2/3] linux-user: add SO_LINGER to setsockopt

[Laurent Vivier]

Le 20/09/2017 à 01:06, Carlo Marcelo Arenas Belón a écrit :
> Original implementation by Chen Gang; all bugs mine
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  linux-user/syscall.c      | 15 +++++++++++++++
>  linux-user/syscall_defs.h |  5 +++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 9b6364a266..ad689dad50 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -3071,6 +3071,21 @@ set_timeout:
>  		unlock_user (dev_ifname, optval_addr, 0);
>  		return ret;
>  	}
> +        case TARGET_SO_LINGER:
> +        {
> +                struct linger lg;
> +                struct target_linger *tlg;
> +

Why did you remove "optname = SO_LINGER" and "if (optlen !=
sizeof(struct target_linger))"?

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 3/3] linux-user: add SO_LINGER to getsockopt

[Laurent Vivier]

Le 20/09/2017 à 01:06, Carlo Marcelo Arenas Belón a écrit :
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  linux-user/syscall.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index ad689dad50..91bd27c63a 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -3178,7 +3178,6 @@ static abi_long do_getsockopt(int sockfd, int level, int optname,
>          level = SOL_SOCKET;
>          switch (optname) {
>          /* These don't just return a single integer */
> -        case TARGET_SO_LINGER:
>          case TARGET_SO_RCVTIMEO:
>          case TARGET_SO_SNDTIMEO:
>          case TARGET_SO_PEERNAME:
> @@ -3216,6 +3215,39 @@ static abi_long do_getsockopt(int sockfd, int level, int optname,
>              }
>              break;
>          }
> +        case TARGET_SO_LINGER:
> +        {
> +            struct linger lg;
> +            socklen_t lglen;
> +            struct target_linger *tlg;
> +
> +            if (get_user_u32(len, optlen)) {
> +                return -TARGET_EFAULT;
> +            }
> +            if (len < 0) {
> +                return -TARGET_EINVAL;
> +            }
> +
> +            lglen = sizeof(lg);
> +            ret = get_errno(getsockopt(sockfd, level, SO_LINGER,
> +                                       &lg, &lglen));
> +            if (ret < 0) {
> +                return ret;
> +            }
> +            if (len > lglen) {
> +                len = lglen;
> +            }
> +            if (!lock_user_struct(VERIFY_WRITE, tlg, optval_addr, 0)) {
> +                return -TARGET_EFAULT;
> +            }
> +            __put_user(lg.l_onoff, &tlg->l_onoff);
> +            __put_user(lg.l_linger, &tlg->l_linger);
> +            unlock_user_struct(tlg, optval_addr, 1);
> +            if (put_user_u32(len, optlen)) {
> +                return -TARGET_EFAULT;
> +            }
> +            break;
> +        }
>          /* Options with 'int' argument.  */
>          case TARGET_SO_DEBUG:
>              optname = SO_DEBUG;
> 

You should merge PATCH 2/3 and PATCH 3/3.

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [Qemu-devel] [PATCH 2/3] linux-user: add SO_LINGER to setsockopt

[Carlo Arenas]

On Wed, Sep 20, 2017 at 1:39 AM, Laurent Vivier <laurent@vivier.eu> wrote:
>
> Why did you remove "optname = SO_LINGER" and "if (optlen !=
> sizeof(struct target_linger))"?
>

the optname assignment is not really needed, since it is only used for the
setsockopt call and that call is clearer using SO_LINGER directly, so to
avoid hard to see bugs like :

  http://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg00980.html

the test for optlen is replaced by passing optlen to the underlying
setsockopt call directly, who would do the test and return the right error.

as an interesting note, I noticed when testing (in ubuntu artful x86_64)
that regardless of how you interpret the documentation, setsockopt won't
fail just because the len is smaller than the size of the struct, and
therefore that code was not equivalent to the setsockopt it was trying to
emulate, and therefore this change doesn't only make the code simpler but
also more correct IMHO

Carlo

Re: [Qemu-devel] [PATCH 2/3] linux-user: add SO_LINGER to setsockopt

[Laurent Vivier]

Le 20/09/2017 à 19:29, Carlo Arenas a écrit :
> On Wed, Sep 20, 2017 at 1:39 AM, Laurent Vivier <laurent@vivier.eu
> <mailto:laurent@vivier.eu>> wrote:
> 
>     Why did you remove "optname = SO_LINGER" and "if (optlen !=
>     sizeof(struct target_linger))"?
> 
> 
> the optname assignment is not really needed, since it is only used for
> the setsockopt call and that call is clearer using SO_LINGER directly,
> so to avoid hard to see bugs like :
> 
>   http://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg00980.html 

Okay

> the test for optlen is replaced by passing optlen to the underlying
> setsockopt call directly, who would do the test and return the right error.

You can't do that, because sizeof(struct linger) may be different from
sizeof(struct target_linger).

> as an interesting note, I noticed when testing (in ubuntu artful x86_64)
> that regardless of how you interpret the documentation, setsockopt won't
> fail just because the len is smaller than the size of the struct, and

Right, see:

http://elixir.free-electrons.com/linux/latest/source/net/core/sock.c#L830

> therefore that code was not equivalent to the setsockopt it was trying
> to emulate, and therefore this change doesn't only make the code simpler
> but also more correct IMHO
Next time add a revision history in your series explaining your changes
(and don't reply to the previous patch series for the new series, it's
better to start a new email thread).

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 2/3] linux-user: add SO_LINGER to setsockopt

[Carlo Arenas]

On Wed, Sep 20, 2017 at 11:53 AM, Laurent Vivier <laurent@vivier.eu> wrote:

> > the test for optlen is replaced by passing optlen to the underlying
> > setsockopt call directly, who would do the test and return the right
> error.
>
> You can't do that, because sizeof(struct linger) may be different from
> sizeof(struct target_linger).
>

Good point, will correct it, but considering that was mostly what I changed
from 陈刚's code, could we merge his instead so I can rebase my changes on
top of it?

just out of curiosity, do you know any such architecture? I assumed that
for everything qemu will care, a struct with 2 ints would be 8 bytes long.


> > as an interesting note, I noticed when testing (in ubuntu artful x86_64)
> > that regardless of how you interpret the documentation, setsockopt won't
> > fail just because the len is smaller than the size of the struct, and
>
> Right, see:
>
> http://elixir.free-electrons.com/linux/latest/source/net/core/sock.c#L830


Sorry; got confused and the one that doesn't fail is actually getsockopt:

http://elixir.free-electrons.com/linux/latest/source/net/core/sock.c#L1178

> therefore that code was not equivalent to the setsockopt it was trying
> > to emulate, and therefore this change doesn't only make the code simpler
> > but also more correct IMHO
> Next time add a revision history in your series explaining your changes
> (and don't reply to the previous patch series for the new series, it's
> better to start a new email thread).
>

Sorry about that, my original intent was to get the original submission to
add support of SO_LINGER to setsockopt out of patchwork limbo[1], hence the
threading and inherited CC

I see there is a lot more work to be done here though, specially when I
found out while trying to test my change for sparc that SOL_SOCKET was also
wrong[2]

is there any testing infrastructure that could be used here to make sure
that no regression is introduced?

Carlo

[1] https://patchwork.ozlabs.org/patch/565659/
[2] https://patchwork.ozlabs.org/patch/816043/