[PATCH] Set socket flags on restore using sock_setsockopt() where possible (v3)

[PATCH] Set socket flags on restore using sock_setsockopt() where possible (v3)

[Dan Smith]

Fail on the TIMESTAMPING_* flags for the moment, with a TODO in place to
handle them later.

Also remove other explicit flag checks because they're no longer copied
blindly into the socket object, so existing checks will be sufficient.

Changes in v3:
 - Move socket flag restore procedures into some common functions and a
   mapping list.  While definitely nicer to look at, my diffstat shows it
   as only saving a single line of code over the previous, while dropping
   the ability to print symbolic flag names in the error messages.

Changes in v2:
 - Avoid removing the sock->sk_socket check before sync'ing the socket.flags
 - Rename sock_rst_flags() to sock_restore_flags()
 - Rebase on top of Oren's cleanup patch

Acked-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 net/checkpoint.c |  112 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 101 insertions(+), 11 deletions(-)

diff --git a/net/checkpoint.c b/net/checkpoint.c
index c64483e..44dc3f1 100644
--- a/net/checkpoint.c
+++ b/net/checkpoint.c
@@ -178,10 +178,6 @@ static int sock_cptrst_verify(struct ckpt_hdr_socket *h)
 	if (!ckpt_validate_errno(h->sock.err))
 		return -EINVAL;
 
-	/* None of our supported types use this flag */
-	if (h->sock.flags & SOCK_DESTROY)
-		return -EINVAL;
-
 	return 0;
 }
 
@@ -238,11 +234,96 @@ static int sock_cptrst_bufopts(int op, struct sock *sk,
 	return 0;
 }
 
+struct sock_flag_mapping {
+	int opt;
+	int flag;
+};
+
+struct sock_flag_mapping sk_flag_map[] = {
+	{SO_OOBINLINE, SOCK_URGINLINE},
+	{SO_KEEPALIVE, SOCK_KEEPOPEN},
+	{SO_BROADCAST, SOCK_BROADCAST},
+	{SO_TIMESTAMP, SOCK_RCVTSTAMP},
+	{SO_TIMESTAMPNS, SOCK_RCVTSTAMPNS},
+	{SO_DEBUG, SOCK_DBG},
+	{SO_DONTROUTE, SOCK_LOCALROUTE},
+};
+
+struct sock_flag_mapping sock_flag_map[] = {
+	{SO_PASSCRED, SOCK_PASSCRED},
+};
+
+static int sock_restore_flag(struct socket *sock,
+			     unsigned long *flags,
+			     int flag,
+			     int option)
+{
+	int v = 1;
+	int ret = 0;
+
+	if (test_and_clear_bit(flag, flags))
+		ret = sock_setsockopt(sock, SOL_SOCKET, option,
+				      (char *)&v, sizeof(v));
+
+	return ret;
+}
+
+
+static int sock_restore_flags(struct socket *sock,
+                             struct ckpt_hdr_socket *h)
+{
+       int ret;
+       int i;
+       unsigned long sk_flags = h->sock.flags;
+       unsigned long sock_flags = h->socket.flags;
+
+       for (i = 0; i < ARRAY_SIZE(sk_flag_map); i++) {
+	       int opt = sk_flag_map[i].opt;
+	       int flag = sk_flag_map[i].flag;
+	       ret = sock_restore_flag(sock, &sk_flags, flag, opt);
+	       if (ret) {
+		       ckpt_debug("Failed to set skopt %i: %i\n", opt, ret);
+		       return ret;
+	       }
+       }
+
+       for (i = 0; i < ARRAY_SIZE(sock_flag_map); i++) {
+	       int opt = sock_flag_map[i].opt;
+	       int flag = sock_flag_map[i].flag;
+	       ret = sock_restore_flag(sock, &sock_flags, flag, opt);
+	       if (ret) {
+		       ckpt_debug("Failed to set sockopt %i: %i\n", opt, ret);
+		       return ret;
+	       }
+       }
+
+       /* TODO: Handle SOCK_TIMESTAMPING_* flags */
+       if (test_bit(SOCK_TIMESTAMPING_TX_HARDWARE, &sk_flags) ||
+           test_bit(SOCK_TIMESTAMPING_TX_SOFTWARE, &sk_flags) ||
+           test_bit(SOCK_TIMESTAMPING_RX_HARDWARE, &sk_flags) ||
+           test_bit(SOCK_TIMESTAMPING_RX_SOFTWARE, &sk_flags) ||
+           test_bit(SOCK_TIMESTAMPING_SOFTWARE, &sk_flags) ||
+           test_bit(SOCK_TIMESTAMPING_RAW_HARDWARE, &sk_flags) ||
+           test_bit(SOCK_TIMESTAMPING_SYS_HARDWARE, &sk_flags)) {
+               ckpt_debug("SOF_TIMESTAMPING_* flags are not supported\n");
+               return -ENOSYS;
+       }
+
+       /* Anything that is still set in the flags that isn't part of
+        * our protocol's default set, indicates an error
+        */
+       if (sk_flags & ~sock->sk->sk_flags) {
+               ckpt_debug("Unhandled sock flags: %lx\n", sk_flags);
+               return -EINVAL;
+       }
+
+       return 0;
+}
+
 static int sock_cptrst(struct ckpt_ctx *ctx, struct sock *sk,
 		       struct ckpt_hdr_socket *h, int op)
 {
 	if (sk->sk_socket) {
-		CKPT_COPY(op, h->socket.flags, sk->sk_socket->flags);
 		CKPT_COPY(op, h->socket.state, sk->sk_socket->state);
 	}
 
@@ -259,12 +340,6 @@ static int sock_cptrst(struct ckpt_ctx *ctx, struct sock *sk,
 	CKPT_COPY(op, h->sock.state, sk->sk_state);
 	CKPT_COPY(op, h->sock.backlog, sk->sk_max_ack_backlog);
 
-	/* TODO:
-	 * Break out setting each of the flags to use setsockopt() or
-	 * perform proper security check
-	 */
-	CKPT_COPY(op, h->sock.flags, sk->sk_flags);
-
 	if (sock_cptrst_bufopts(op, sk, h))
 		return -EINVAL;
 
@@ -298,6 +373,21 @@ static int sock_cptrst(struct ckpt_ctx *ctx, struct sock *sk,
 		return -EINVAL;
 	}
 
+	if (op == CKPT_CPT) {
+		h->sock.flags = sk->sk_flags;
+		h->socket.flags = sk->sk_socket->flags;
+	} else {
+		int ret;
+		mm_segment_t old_fs;
+
+		old_fs = get_fs();
+		set_fs(KERNEL_DS);
+		ret = sock_restore_flags(sk->sk_socket, h);
+		set_fs(old_fs);
+		if (ret)
+			return ret;
+	}
+
 	if ((h->socket.state == SS_CONNECTED) &&
 	    (h->sock.state != TCP_ESTABLISHED)) {
 		ckpt_debug("socket/sock in inconsistent state: %i/%i",
-- 
1.6.2.5

Re: [PATCH] Set socket flags on restore using sock_setsockopt() where possible (v3)

[Oren Laadan]

This one added, too.

Dan Smith wrote:
> Fail on the TIMESTAMPING_* flags for the moment, with a TODO in place to
> handle them later.
> 
> Also remove other explicit flag checks because they're no longer copied
> blindly into the socket object, so existing checks will be sufficient.
> 
> Changes in v3:
>  - Move socket flag restore procedures into some common functions and a
>    mapping list.  While definitely nicer to look at, my diffstat shows it
>    as only saving a single line of code over the previous, while dropping
>    the ability to print symbolic flag names in the error messages.
> 
> Changes in v2:
>  - Avoid removing the sock->sk_socket check before sync'ing the socket.flags
>  - Rename sock_rst_flags() to sock_restore_flags()
>  - Rebase on top of Oren's cleanup patch
> 
> Acked-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
> Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  net/checkpoint.c |  112 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 101 insertions(+), 11 deletions(-)
> 
> diff --git a/net/checkpoint.c b/net/checkpoint.c
> index c64483e..44dc3f1 100644
> --- a/net/checkpoint.c
> +++ b/net/checkpoint.c
> @@ -178,10 +178,6 @@ static int sock_cptrst_verify(struct ckpt_hdr_socket *h)
>  	if (!ckpt_validate_errno(h->sock.err))
>  		return -EINVAL;
>  
> -	/* None of our supported types use this flag */
> -	if (h->sock.flags & SOCK_DESTROY)
> -		return -EINVAL;
> -
>  	return 0;
>  }
>  
> @@ -238,11 +234,96 @@ static int sock_cptrst_bufopts(int op, struct sock *sk,
>  	return 0;
>  }
>  
> +struct sock_flag_mapping {
> +	int opt;
> +	int flag;
> +};
> +
> +struct sock_flag_mapping sk_flag_map[] = {
> +	{SO_OOBINLINE, SOCK_URGINLINE},
> +	{SO_KEEPALIVE, SOCK_KEEPOPEN},
> +	{SO_BROADCAST, SOCK_BROADCAST},
> +	{SO_TIMESTAMP, SOCK_RCVTSTAMP},
> +	{SO_TIMESTAMPNS, SOCK_RCVTSTAMPNS},
> +	{SO_DEBUG, SOCK_DBG},
> +	{SO_DONTROUTE, SOCK_LOCALROUTE},
> +};
> +
> +struct sock_flag_mapping sock_flag_map[] = {
> +	{SO_PASSCRED, SOCK_PASSCRED},
> +};
> +
> +static int sock_restore_flag(struct socket *sock,
> +			     unsigned long *flags,
> +			     int flag,
> +			     int option)
> +{
> +	int v = 1;
> +	int ret = 0;
> +
> +	if (test_and_clear_bit(flag, flags))
> +		ret = sock_setsockopt(sock, SOL_SOCKET, option,
> +				      (char *)&v, sizeof(v));
> +
> +	return ret;
> +}
> +
> +
> +static int sock_restore_flags(struct socket *sock,
> +                             struct ckpt_hdr_socket *h)
> +{
> +       int ret;
> +       int i;
> +       unsigned long sk_flags = h->sock.flags;
> +       unsigned long sock_flags = h->socket.flags;
> +
> +       for (i = 0; i < ARRAY_SIZE(sk_flag_map); i++) {
> +	       int opt = sk_flag_map[i].opt;
> +	       int flag = sk_flag_map[i].flag;
> +	       ret = sock_restore_flag(sock, &sk_flags, flag, opt);
> +	       if (ret) {
> +		       ckpt_debug("Failed to set skopt %i: %i\n", opt, ret);
> +		       return ret;
> +	       }
> +       }
> +
> +       for (i = 0; i < ARRAY_SIZE(sock_flag_map); i++) {
> +	       int opt = sock_flag_map[i].opt;
> +	       int flag = sock_flag_map[i].flag;
> +	       ret = sock_restore_flag(sock, &sock_flags, flag, opt);
> +	       if (ret) {
> +		       ckpt_debug("Failed to set sockopt %i: %i\n", opt, ret);
> +		       return ret;
> +	       }
> +       }
> +
> +       /* TODO: Handle SOCK_TIMESTAMPING_* flags */
> +       if (test_bit(SOCK_TIMESTAMPING_TX_HARDWARE, &sk_flags) ||
> +           test_bit(SOCK_TIMESTAMPING_TX_SOFTWARE, &sk_flags) ||
> +           test_bit(SOCK_TIMESTAMPING_RX_HARDWARE, &sk_flags) ||
> +           test_bit(SOCK_TIMESTAMPING_RX_SOFTWARE, &sk_flags) ||
> +           test_bit(SOCK_TIMESTAMPING_SOFTWARE, &sk_flags) ||
> +           test_bit(SOCK_TIMESTAMPING_RAW_HARDWARE, &sk_flags) ||
> +           test_bit(SOCK_TIMESTAMPING_SYS_HARDWARE, &sk_flags)) {
> +               ckpt_debug("SOF_TIMESTAMPING_* flags are not supported\n");
> +               return -ENOSYS;
> +       }
> +
> +       /* Anything that is still set in the flags that isn't part of
> +        * our protocol's default set, indicates an error
> +        */
> +       if (sk_flags & ~sock->sk->sk_flags) {
> +               ckpt_debug("Unhandled sock flags: %lx\n", sk_flags);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
>  static int sock_cptrst(struct ckpt_ctx *ctx, struct sock *sk,
>  		       struct ckpt_hdr_socket *h, int op)
>  {
>  	if (sk->sk_socket) {
> -		CKPT_COPY(op, h->socket.flags, sk->sk_socket->flags);
>  		CKPT_COPY(op, h->socket.state, sk->sk_socket->state);
>  	}
>  
> @@ -259,12 +340,6 @@ static int sock_cptrst(struct ckpt_ctx *ctx, struct sock *sk,
>  	CKPT_COPY(op, h->sock.state, sk->sk_state);
>  	CKPT_COPY(op, h->sock.backlog, sk->sk_max_ack_backlog);
>  
> -	/* TODO:
> -	 * Break out setting each of the flags to use setsockopt() or
> -	 * perform proper security check
> -	 */
> -	CKPT_COPY(op, h->sock.flags, sk->sk_flags);
> -
>  	if (sock_cptrst_bufopts(op, sk, h))
>  		return -EINVAL;
>  
> @@ -298,6 +373,21 @@ static int sock_cptrst(struct ckpt_ctx *ctx, struct sock *sk,
>  		return -EINVAL;
>  	}
>  
> +	if (op == CKPT_CPT) {
> +		h->sock.flags = sk->sk_flags;
> +		h->socket.flags = sk->sk_socket->flags;
> +	} else {
> +		int ret;
> +		mm_segment_t old_fs;
> +
> +		old_fs = get_fs();
> +		set_fs(KERNEL_DS);
> +		ret = sock_restore_flags(sk->sk_socket, h);
> +		set_fs(old_fs);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if ((h->socket.state == SS_CONNECTED) &&
>  	    (h->sock.state != TCP_ESTABLISHED)) {
>  		ckpt_debug("socket/sock in inconsistent state: %i/%i",