Re: [RFC bpf-next v3 6/8] flow_dissector: handle no-skb use case

From: Stanislav Fomichev <sdf@fomichev.me>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Stanislav Fomichev <sdf@google.com>,
	netdev@vger.kernel.org, bpf@vger.kernel.org, davem@davemloft.net,
	ast@kernel.org, daniel@iogearbox.net, simon.horman@netronome.com,
	willemb@google.com, peterpenkov96@gmail.com
Subject: Re: [RFC bpf-next v3 6/8] flow_dissector: handle no-skb use case
Date: Fri, 22 Mar 2019 18:19:57 -0700	[thread overview]
Message-ID: <20190323011957.GY7431@mini-arch.hsd1.ca.comcast.net> (raw)
In-Reply-To: <20190323010049.dbywat3n6kfelwth@ast-mbp.dhcp.thefacebook.com>

On 03/22, Alexei Starovoitov wrote:
> On Fri, Mar 22, 2019 at 12:59:01PM -0700, Stanislav Fomichev wrote:
> > When called without skb, gather all required data from the
> > __skb_flow_dissect's arguments and use recently introduces
> > no-skb mode of bpf flow dissector.
> > 
> > Note: WARN_ON_ONCE(!net) will now trigger for eth_get_headlen users.
> > 
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > +			struct bpf_flow_keys flow_keys;
> > +			struct bpf_flow_dissector ctx = {
> > +				.flow_keys = &flow_keys,
> > +				.data = data,
> > +				.data_end = data + hlen,
> > +				.protocol = proto,
> > +			};
> > +
> > +			if (skb) {
> > +				ctx.skb = skb;
> > +				ctx.protocol = skb->protocol;
> > +				ctx.vlan_tci = skb->vlan_tci;
> > +				ctx.vlan_proto = skb->vlan_proto;
> > +				ctx.vlan_present = skb->vlan_present;
> > +			}
> 
> are you suggesting to have vlan* fields that only work properly for skb case?
> It means that progs/bpf_flow.c would not work as-is for eth_get_headlen.
> And to have unified program that works in both cases the program would need to rely
> on above internal implementation detail, like checking that ctx->protocol == 0 ?
> imo that is worse than having two different flow dissector program types.
Yeah. The reference implementation (bpf_flow.c) should handle it though.
In parse_eth_proto we handle ETH_P_8021Q/ETH_P_8021AD, so I'm not sure
why we even had that skb->vlan_present check in the first place.
[But, again, who knows what's out there besides bpf_flow.c]

In general, this RFC's approach is to have a "best effort" vlan
detection, bpf program would still have to handle it.
Agree, that it's confusing.

> May be remove protocol and vlan* from ctx ?
> Then the only thing program can do is look at the packet data which is
> eth_get_headlen use case. For skb case the existence of vlan can be retrofitted into
> dissector results by the kernel after the program finished.
Are we ok with breaking api in this case? I'm all in on removing this
extra information. We can always put it back if somebody complains (and
manually parse in eth_get_headlen case).

Regarding dissector results: that's currently not implemented, we don't
export vlan information.

We can still have protocol, because in both skb/skb-less cases we have
it.