From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jakub Kicinski Subject: Re: [PATCH net-next v2 3/4] net: check tunnel option type in tunnel flags Date: Thu, 28 Jun 2018 09:54:52 -0700 Message-ID: <20180628095452.6f23fdf4@cakuba.netronome.com> References: <20180627043937.25431-1-jakub.kicinski@netronome.com> <20180627043937.25431-4-jakub.kicinski@netronome.com> <20180628094206.62b6d8e2@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: Daniel Borkmann , davem@davemloft.net, Roopa Prabhu , jiri@resnulli.us, jhs@mojatatu.com, xiyou.wangcong@gmail.com, oss-drivers@netronome.com, netdev@vger.kernel.org, Pieter Jansen van Vuuren To: Jiri Benc Return-path: Received: from mail-qt0-f194.google.com ([209.85.216.194]:37247 "EHLO mail-qt0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933192AbeF1Qy5 (ORCPT ); Thu, 28 Jun 2018 12:54:57 -0400 Received: by mail-qt0-f194.google.com with SMTP id a18-v6so5341026qtj.4 for ; Thu, 28 Jun 2018 09:54:57 -0700 (PDT) In-Reply-To: <20180628094206.62b6d8e2@redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: On Thu, 28 Jun 2018 09:42:06 +0200, Jiri Benc wrote: > On Wed, 27 Jun 2018 11:49:49 +0200, Daniel Borkmann wrote: > > Looks good to me, and yes in BPF case a mask like TUNNEL_OPTIONS_PRESENT is > > right approach since this is opaque info and solely defined by the BPF prog > > that is using the generic helper. > > Wouldn't it make sense to introduce some safeguards here (in a backward > compatible way, of course)? It's easy to mistakenly set data for a > different tunnel type in a BPF program and then be surprised by the > result. It might help users if such usage was detected by the kernel, > one way or another. Well, that's how it works today ;) > I'm thinking about something like the BPF program voluntarily > specifying the type of the data; if not specified, the wildcard would be > used as it is now. Hmm... in practice we could steal top bits of the size parameter for some flags, since it seems to be limited to values < 256 today? Is it worth it? It would look something along the lines of: --- diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 59b19b6a40d7..194b40efa8e8 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -2213,6 +2213,13 @@ enum bpf_func_id { /* BPF_FUNC_perf_event_output for sk_buff input context. */ #define BPF_F_CTXLEN_MASK (0xfffffULL << 32) +#define BPF_F_TUN_VXLAN (1U << 31) +#define BPF_F_TUN_GENEVE (1U << 30) +#define BPF_F_TUN_ERSPAN (1U << 29) +#define BPF_F_TUN_FLAGS_ALL (BPF_F_TUN_VXLAN | \ + BPF_F_TUN_GENEVE | \ + BPF_F_TUN_ERSPAN) + /* Mode for BPF_FUNC_skb_adjust_room helper. */ enum bpf_adj_room_mode { BPF_ADJ_ROOM_NET, diff --git a/net/core/filter.c b/net/core/filter.c index dade922678f6..cc592a1e8945 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -3576,6 +3576,22 @@ BPF_CALL_3(bpf_skb_set_tunnel_opt, struct sk_buff *, skb, { struct ip_tunnel_info *info = skb_tunnel_info(skb); const struct metadata_dst *md = this_cpu_ptr(md_dst); + __be16 tun_flags; + u32 flags; + + BUILD_BUG_ON(BPF_F_TUN_FLAGS_ALL & IP_TUNNEL_OPTS_MAX); + + flags = size & BPF_F_TUN_FLAGS_ALL; + size &= ~flags; + if (flags & BPF_F_TUN_VXLAN) + tun_flags |= TUNNEL_VXLAN_OPT; + if (flags & BPF_F_TUN_GENEVE) + tun_flags |= TUNNEL_GENEVE_OPT; + if (flags & BPF_F_TUN_ERSPAN) + tun_flags |= TUNNEL_ERSPAN_OPT; + /* User didn't specify the tunnel type, for backward compat set all */ + if (!(tun_flags & TUNNEL_OPTIONS_PRESENT)) + tun_flags |= TUNNEL_OPTIONS_PRESENT; if (unlikely(info != &md->u.tun_info || (size & (sizeof(u32) - 1)))) return -EINVAL;