From mboxrd@z Thu Jan  1 00:00:00 1970
From: Daniel Borkmann <daniel@iogearbox.net>
Subject: Re: [PATCH bpf-next]: add sock_ops R/W access to ipv4 tos
Date: Wed, 28 Mar 2018 15:47:41 +0200
Message-ID: <67c87527-6f6c-d5fb-5fe9-83c3aa1d000d@iogearbox.net>
References: <20180326153657.2229959-1-tehnerd@fb.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: kernel-team@fb.com
To: "Nikita V. Shirokov" <tehnerd@fb.com>, brakmo@fb.com,
        ast@kernel.org, netdev@vger.kernel.org
Return-path: <netdev-owner@vger.kernel.org>
Received: from www62.your-server.de ([213.133.104.62]:56580 "EHLO
        www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751207AbeC1Nrq (ORCPT
        <rfc822;netdev@vger.kernel.org>); Wed, 28 Mar 2018 09:47:46 -0400
In-Reply-To: <20180326153657.2229959-1-tehnerd@fb.com>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On 03/26/2018 05:36 PM, Nikita V. Shirokov wrote:
>     bpf: Add sock_ops R/W access to ipv4 tos
> 
>     Sample usage for tos:
> 
>       bpf_getsockopt(skops, SOL_IP, IP_TOS, &v, sizeof(v))
> 
>     where skops is a pointer to the ctx (struct bpf_sock_ops).
> 
> Signed-off-by: Nikita V. Shirokov <tehnerd@fb.com>
> ---
>  net/core/filter.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 00c711c..afd8255 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3462,6 +3462,27 @@ BPF_CALL_5(bpf_setsockopt, struct bpf_sock_ops_kern *, bpf_sock,
>  			ret = -EINVAL;
>  		}
>  #ifdef CONFIG_INET
> +	} else if (level == SOL_IP) {
> +		if (optlen != sizeof(int) || sk->sk_family != AF_INET)
> +			return -EINVAL;
> +
> +		val = *((int *)optval);
> +		/* Only some options are supported */
> +		switch (optname) {
> +		case IP_TOS:
> +			if (val < -1 || val > 0xff) {
> +				ret = -EINVAL;
> +			} else {
> +				struct inet_sock *inet = inet_sk(sk);
> +
> +				if (val == -1)
> +					val = 0;
> +				inet->tos = val;

Should this not have the exact same semantics given the helper resembles
the normal setsockopt? do_ip_setsockopt() does the following when setting
IP_TOS:

        case IP_TOS:    /* This sets both TOS and Precedence */
                if (sk->sk_type == SOCK_STREAM) {
                        val &= ~INET_ECN_MASK;
                        val |= inet->tos & INET_ECN_MASK;
                }
                if (inet->tos != val) {
                        inet->tos = val;
                        sk->sk_priority = rt_tos2priority(val);
                        sk_dst_reset(sk);
                }
                break;

E.g. why we don't need to set sk->sk_priority as well or reset the dst
entry here?

> +			}
> +			break;
> +		default:
> +			ret = -EINVAL;
> +		}
>  #if IS_ENABLED(CONFIG_IPV6)
>  	} else if (level == SOL_IPV6) {
>  		if (optlen != sizeof(int) || sk->sk_family != AF_INET6)
> @@ -3561,6 +3582,20 @@ BPF_CALL_5(bpf_getsockopt, struct bpf_sock_ops_kern *, bpf_sock,
>  		} else {
>  			goto err_clear;
>  		}
> +	} else if (level == SOL_IP) {
> +		struct inet_sock *inet = inet_sk(sk);
> +
> +		if (optlen != sizeof(int) || sk->sk_family != AF_INET)
> +			goto err_clear;
> +
> +		/* Only some options are supported */
> +		switch (optname) {
> +		case IP_TOS:
> +			*((int *)optval) = (int)inet->tos;

This part is fine though, same as in do_ip_getsockopt().

> +			break;
> +		default:
> +			goto err_clear;
> +		}
>  #if IS_ENABLED(CONFIG_IPV6)
>  	} else if (level == SOL_IPV6) {
>  		struct ipv6_pinfo *np = inet6_sk(sk);
>