Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 19 Dec 2022 10:41:36 -0800
In-Reply-To: <20221218051734.31411-1-cehrig@cloudflare.com>
Mime-Version: 1.0
References: <20221218051734.31411-1-cehrig@cloudflare.com>
Message-ID: <Y6CwYK4xMTJv/R7u@google.com>
Subject: Re: [PATCH bpf-next 1/2] bpf: Add flag BPF_F_NO_TUNNEL_KEY to bpf_skb_set_tunnel_key()
From:   sdf@google.com
To:     Christian Ehrig <cehrig@cloudflare.com>
Cc:     bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>, Hao Luo <haoluo@google.com>,
        Jiri Olsa <jolsa@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Mykola Lysenko <mykolal@fb.com>, Shuah Khan <shuah@kernel.org>,
        Joanne Koong <joannelkoong@gmail.com>,
        Kui-Feng Lee <kuifeng@fb.com>,
        Maxim Mikityanskiy <maximmi@nvidia.com>,
        Kaixi Fan <fankaixi.li@bytedance.com>,
        Shmulik Ladkani <shmulik@metanetworks.com>,
        Paul Chaignon <paul@isovalent.com>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        linux-kselftest@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"; format=flowed; delsp=yes
Precedence: bulk

On 12/18, Christian Ehrig wrote:
> This patch allows to remove TUNNEL_KEY from the tunnel flags bitmap
> when using bpf_skb_set_tunnel_key by providing a BPF_F_NO_TUNNEL_KEY
> flag. On egress, the resulting tunnel header will not contain a tunnel
> key if the protocol and implementation supports it.

> At the moment bpf_tunnel_key wants a user to specify a numeric tunnel
> key. This will wrap the inner packet into a tunnel header with the key
> bit and value set accordingly. This is problematic when using a tunnel
> protocol that supports optional tunnel keys and a receiving tunnel
> device that is not expecting packets with the key bit set. The receiver
> won't decapsulate and drop the packet.

> RFC 2890 and RFC 2784 GRE tunnels are examples where this flag is
> useful. It allows for generating packets, that can be decapsulated by
> a GRE tunnel device not operating in collect metadata mode or not
> expecting the key bit set.

> Signed-off-by: Christian Ehrig <cehrig@cloudflare.com>

Acked-by: Stanislav Fomichev <sdf@google.com>

> ---
>   include/uapi/linux/bpf.h       | 4 ++++
>   net/core/filter.c              | 5 ++++-
>   tools/include/uapi/linux/bpf.h | 4 ++++
>   3 files changed, 12 insertions(+), 1 deletion(-)

> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 464ca3f01fe7..bc1a3d232ae4 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2001,6 +2001,9 @@ union bpf_attr {
>    * 			sending the packet. This flag was added for GRE
>    * 			encapsulation, but might be used with other protocols
>    * 			as well in the future.
> + * 		**BPF_F_NO_TUNNEL_KEY**
> + * 			Add a flag to tunnel metadata indicating that no tunnel
> + * 			key should be set in the resulting tunnel header.
>    *
>    * 		Here is a typical usage on the transmit path:
>    *
> @@ -5764,6 +5767,7 @@ enum {
>   	BPF_F_ZERO_CSUM_TX		= (1ULL << 1),
>   	BPF_F_DONT_FRAGMENT		= (1ULL << 2),
>   	BPF_F_SEQ_NUMBER		= (1ULL << 3),
> +	BPF_F_NO_TUNNEL_KEY		= (1ULL << 4),
>   };

>   /* BPF_FUNC_skb_get_tunnel_key flags. */
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 929358677183..c746e4d77214 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4615,7 +4615,8 @@ BPF_CALL_4(bpf_skb_set_tunnel_key, struct sk_buff  
> *, skb,
>   	struct ip_tunnel_info *info;

>   	if (unlikely(flags & ~(BPF_F_TUNINFO_IPV6 | BPF_F_ZERO_CSUM_TX |
> -			       BPF_F_DONT_FRAGMENT | BPF_F_SEQ_NUMBER)))
> +			       BPF_F_DONT_FRAGMENT | BPF_F_SEQ_NUMBER |
> +			       BPF_F_NO_TUNNEL_KEY)))
>   		return -EINVAL;
>   	if (unlikely(size != sizeof(struct bpf_tunnel_key))) {
>   		switch (size) {
> @@ -4653,6 +4654,8 @@ BPF_CALL_4(bpf_skb_set_tunnel_key, struct sk_buff  
> *, skb,
>   		info->key.tun_flags &= ~TUNNEL_CSUM;
>   	if (flags & BPF_F_SEQ_NUMBER)
>   		info->key.tun_flags |= TUNNEL_SEQ;
> +	if (flags & BPF_F_NO_TUNNEL_KEY)
> +		info->key.tun_flags &= ~TUNNEL_KEY;

>   	info->key.tun_id = cpu_to_be64(from->tunnel_id);
>   	info->key.tos = from->tunnel_tos;
> diff --git a/tools/include/uapi/linux/bpf.h  
> b/tools/include/uapi/linux/bpf.h
> index 464ca3f01fe7..bc1a3d232ae4 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -2001,6 +2001,9 @@ union bpf_attr {
>    * 			sending the packet. This flag was added for GRE
>    * 			encapsulation, but might be used with other protocols
>    * 			as well in the future.
> + * 		**BPF_F_NO_TUNNEL_KEY**
> + * 			Add a flag to tunnel metadata indicating that no tunnel
> + * 			key should be set in the resulting tunnel header.
>    *
>    * 		Here is a typical usage on the transmit path:
>    *
> @@ -5764,6 +5767,7 @@ enum {
>   	BPF_F_ZERO_CSUM_TX		= (1ULL << 1),
>   	BPF_F_DONT_FRAGMENT		= (1ULL << 2),
>   	BPF_F_SEQ_NUMBER		= (1ULL << 3),
> +	BPF_F_NO_TUNNEL_KEY		= (1ULL << 4),
>   };

>   /* BPF_FUNC_skb_get_tunnel_key flags. */
> --
> 2.37.4