From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.5 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS, USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2FB93C43381 for ; Mon, 25 Feb 2019 20:33:43 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id DF71A20C01 for ; Mon, 25 Feb 2019 20:33:42 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=fomichev-me.20150623.gappssmtp.com header.i=@fomichev-me.20150623.gappssmtp.com header.b="DFx4IYzk" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727323AbfBYUdl (ORCPT ); Mon, 25 Feb 2019 15:33:41 -0500 Received: from mail-pg1-f194.google.com ([209.85.215.194]:35757 "EHLO mail-pg1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727179AbfBYUdl (ORCPT ); Mon, 25 Feb 2019 15:33:41 -0500 Received: by mail-pg1-f194.google.com with SMTP id e17so2970244pgd.2 for ; Mon, 25 Feb 2019 12:33:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fomichev-me.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=TacezfxjUJKVQmx48JC2cXdFj9RDokxrNGZM06BRw8s=; b=DFx4IYzkxGBiX3aNVmnPNGY8L6L/vkbxhy+PAKnpr6czpbm8dKtp4qDgBAGePePeiC 5huO1ZlYMz/fHL91iEBvnc5xmCr+T8KJEMDVfbq1JcqB0JiAdU497Ev10RK7I0CH/5bW IHROnLI2WZvffmJB0GOvowJ/vZaat2q7Dxbaf928LUKUUDl4MzMif3PBIU3GKgWgboBL TMdnb9qtwiZNdaw0IuFTkT81Ipgs1IUmrNbS7PgIXRlA4HzxWH7O+kGIeMftTtZBH6kX aS/27oiWBn70YMPhP3vPTus8NwG/IF5E0r1uwvYkc2EUiPOC0ZovAfPTphGrF2MC+ziV g0XA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=TacezfxjUJKVQmx48JC2cXdFj9RDokxrNGZM06BRw8s=; b=IzdlD0XpodyXlVokT76hP53pjNpWlgJRxNoETBndbmPlvv3FzJYfgbxRjJHqN9u1Ie emSmrBR/v4YBU9Jnn3sTHeqncOf/MkilVhKiqxMSA3KaTytUSXhfyAaojK2rZvI2wqft HgznSN80zdqufwTJ216GjPRV+EsKFEwwJB6zG72FK+CG0wkD781Ns4KHuYhu0/J9Wybg J0NtJL41US2hJiYTv0Bik+IKnMLk3UsVnWucOf97XT5SHJMVul3/ciG9kffiOUc/NB3W /VU7QT9CR2Q/i1D8kyCY5rx+YuoTDc0X6BHnQmbCr8F+JLwM6DC2CqmYy9bSKf7/+tWI YMYQ== X-Gm-Message-State: AHQUAuZ4VkyZ7iSmvWVID2QooiEpy9+xwJboJ6Jf8LwkfZTHzn8ncVXu Hnu+E7Eq26QTJE/t+BNstnq5og== X-Google-Smtp-Source: AHgI3IbKzzYZz1PZN6q0iuFPN6ZiouLOy2yDeAhl/zZRhVRggYWGsVRIGhv7hyQMeTcdGa64SWa0Xg== X-Received: by 2002:a62:4815:: with SMTP id v21mr21710608pfa.167.1551126819683; Mon, 25 Feb 2019 12:33:39 -0800 (PST) Received: from localhost (c-98-210-199-22.hsd1.ca.comcast.net. [98.210.199.22]) by smtp.gmail.com with ESMTPSA id l72sm11511780pge.39.2019.02.25.12.33.38 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 25 Feb 2019 12:33:38 -0800 (PST) Date: Mon, 25 Feb 2019 12:33:37 -0800 From: Stanislav Fomichev To: Alexei Starovoitov Cc: Willem de Bruijn , Stanislav Fomichev , Network Development , David Miller , Alexei Starovoitov , Daniel Borkmann , simon.horman@netronome.com, Willem de Bruijn Subject: Re: [RFC bpf-next 0/7] net: flow_dissector: trigger BPF hook when called from eth_get_headlen Message-ID: <20190225203337.GA32115@mini-arch> References: <20190206005931.GF10769@mini-arch> <20190206031215.qldeh7pfgqr3frg3@ast-mbp> <20190206035619.GG10769@mini-arch> <20190206041112.h3ccahimp2uerdfk@ast-mbp> <20190206054946.GH10769@mini-arch> <20190212170232.GB10595@mini-arch> <20190214043931.hdpddeom57rnnlhm@ast-mbp> <20190214055725.GC10595@mini-arch> <20190214063848.7zr5ph3ijp3wmjgx@ast-mbp.dhcp.thefacebook.com> <20190214173542.GA20651@mini-arch> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190214173542.GA20651@mini-arch> User-Agent: Mutt/1.11.3 (2019-02-01) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 02/14, Stanislav Fomichev wrote: > On 02/13, Alexei Starovoitov wrote: > > On Wed, Feb 13, 2019 at 09:57:25PM -0800, Stanislav Fomichev wrote: > > > > > > > That 'stuck with __sk_buff' is what bothers me. > > > I might have use the wrong word here. I don't think there is another > > > option to be honest. Using __sk_buff makes flow dissector programs work > > > with fragmented packets; > > > > good point. indeed real skb is essential. > > > > > > It's an indication that api wasn't thought through if first thing > > > > it needs is this fake skb hack. > > > > If bpf_flow.c is a realistic example of such flow dissector prog > > > > it means that real skb fields are accessed. > > > > In particular skb->vlan_proto, skb->protocol. > > > I do manually set skb->protocol to eth->h_proto in my proposal. This is later > > > correctly handled by bpf_flow.c: parse_eth_proto() is called on skb->protocol > > > and we correctly handle bpf_htons(ETH_P_8021Q) there. So existing > > > bpf_flow.c works as expected. > > ... > > > The goal of this patch series was to essentially make this skb/no-skb > > > context transparent to the bpf_flow.c (i.e. no changes from the user > > > flow programs). Adding another flow dissector for eth_get_headlen case > > > also seems as a no go. > > > > The problem with this thinking is assumption that bpf_flow.c is the only program. > I agree, it's a bad assumption, but it is sort of a reference implementation, > I don't expect other users to do something wildly different. Hopefully :-) > > > Since ctx of flow_dissector prog type is 'struct __sk_buff' > > all fields should be valid or the verifier has to reject access > > to fields that were not set. > > You cannot "manually set skb->protocol to eth->h_proto" in fake skb > > and ignore the rest. > Ugh, I did expect that we only allow a minimal set of __sk_buff fields > to be allowed from the flow dissector program type, but that's not the > case. We explicitly prohibit access only to > family/ips/ports/tc_classid/tstamp/wire_len, everything else is readable :-/ > Any idea why? > Stuff like ingress_ifindex/ifindex/hash/mark/queue_mapping, does flow dissector > programs really need to know that? > > For the most part, using zero-initialized fake skb looks fine, except: > * infindex, where we do skb->dev->ifndex (skb->dev is NULL) > * gso_segs, where we do skb_shinfo(skb)->gso_segs (we are missing > shinfo) > > So there is indeed a couple of problems. > > How do you feel about tightening down the access to sk_buff fields from > the flow dissector program type? That is an API change, but I don't see why > existing users should use those fields. Let's allow access only to > len/data/data_end, protocol, vlan_{present,tci,proto}, cb, flow_keys, > that should be enough to dissect the packet (I also looked at C-based > implementation, it doesn't use anything besides that). > We can always rollback if somebody complains about. To revive the conversation, here is what I was thinking about (whitelist the skb fields, not blacklist them): --- a/net/core/filter.c +++ b/net/core/filter.c @@ -6591,11 +6591,14 @@ static bool flow_dissector_is_valid_access(int off, int size, case bpf_ctx_range_ptr(struct __sk_buff, flow_keys): info->reg_type = PTR_TO_FLOW_KEYS; break; - case bpf_ctx_range(struct __sk_buff, tc_classid): - case bpf_ctx_range(struct __sk_buff, data_meta): - case bpf_ctx_range_till(struct __sk_buff, family, local_port): - case bpf_ctx_range(struct __sk_buff, tstamp): - case bpf_ctx_range(struct __sk_buff, wire_len): + case bpf_ctx_range(struct __sk_buff, len): + case bpf_ctx_range(struct __sk_buff, protocol): + case bpf_ctx_range(struct __sk_buff, vlan_present): + case bpf_ctx_range(struct __sk_buff, vlan_tci): + case bpf_ctx_range(struct __sk_buff, vlan_proto): + case bpf_ctx_range_till(struct __sk_buff, cb[0], cb[4]): + break; + default: return false; } What do you think?