From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Borkmann Subject: Re: [PATCH bpf-next,v2 1/2] bpf: add helper for getting xfrm states Date: Tue, 24 Apr 2018 14:54:37 +0200 Message-ID: References: <1524088703-6324-1-git-send-email-eyal.birger@gmail.com> <1524088703-6324-2-git-send-email-eyal.birger@gmail.com> <20180418223101.47jl57wrfnqtv6j6@ast-mbp> <20180420064356.7a703a1e@jimi> <20180423003430.dxc3frkp3opbvl7f@ast-mbp> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, shmulik@metanetworks.com, ast@kernel.org, fw@strlen.de, steffen.klassert@secunet.com To: Alexei Starovoitov , Eyal Birger Return-path: Received: from www62.your-server.de ([213.133.104.62]:35392 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933278AbeDXMyk (ORCPT ); Tue, 24 Apr 2018 08:54:40 -0400 In-Reply-To: <20180423003430.dxc3frkp3opbvl7f@ast-mbp> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 04/23/2018 02:34 AM, Alexei Starovoitov wrote: > On Fri, Apr 20, 2018 at 06:43:56AM +0300, Eyal Birger wrote: >> On Wed, 18 Apr 2018 15:31:03 -0700 >> Alexei Starovoitov wrote: >>> On Thu, Apr 19, 2018 at 12:58:22AM +0300, Eyal Birger wrote: >>>> This commit introduces a helper which allows fetching xfrm state >>>> parameters by eBPF programs attached to TC. >>>> >>>> Prototype: >>>> bpf_skb_get_xfrm_state(skb, index, xfrm_state, size, flags) >>>> >>>> skb: pointer to skb >>>> index: the index in the skb xfrm_state secpath array >>>> xfrm_state: pointer to 'struct bpf_xfrm_state' >>>> size: size of 'struct bpf_xfrm_state' >>>> flags: reserved for future extensions >> >> >> >>>> +#ifdef CONFIG_XFRM >>>> +BPF_CALL_5(bpf_skb_get_xfrm_state, struct sk_buff *, skb, u32, >>>> index, >>>> + struct bpf_xfrm_state *, to, u32, size, u64, flags) >>>> +{ >>>> + const struct sec_path *sp = skb_sec_path(skb); >>>> + const struct xfrm_state *x; >>>> + >>>> + if (!sp || unlikely(index >= sp->len || flags)) >>>> + goto err_clear; >>>> + >>>> + x = sp->xvec[index]; >>>> + >>>> + if (unlikely(size != sizeof(struct bpf_xfrm_state))) >>>> + goto err_clear; >>>> + >>>> + to->reqid = x->props.reqid; >>>> + to->spi = be32_to_cpu(x->id.spi); >>>> + to->family = x->props.family; >>>> + if (to->family == AF_INET6) { >>>> + memcpy(to->remote_ipv6, x->props.saddr.a6, >>>> + sizeof(to->remote_ipv6)); >>>> + } else { >>>> + to->remote_ipv4 = be32_to_cpu(x->props.saddr.a4); >>>> + } >>> >>> that looks inconsistent. Why v4 is cpu endian, but v6 not? >> >> I agree. I followed the reference in bpf_skb_get_tunnel_key(). >> I can keep v4 in net endianess too. > > argh. > On one side it makes sense to be consistent with bpf_skb_get_tunnel_key() > but it's certainly confusing to have v4 and v6 in different endianness. > Imagine man page that says that bpf folks made a mistake in that > helper can kept repeating it in other helpers for consistency... > Daniel, what do you think? > Do you remember the history with bpf_skb_get_tunnel_key and > why it happened that way? Check out d3aa45ce6b94 ("bpf: add helpers to access tunnel metadata"). I presume there was no particular reason for doing it this way, perhaps to mimic old ld_abs kind of behavior, I don't know. >>> Why change endianness of the spi? >> >> I felt it was more consistent with other fields and usually helpful for >> programs. I can keep it in network order. >> >> In which case, do you expect it to be typed as __be32 in bpf.h? >> (I haven't seen other cases)? > > It can be __u32 with a comment /* Stored in network byte order */ > like in bunch of other fields. Yeah, agree. I guess I would have been fine either way given this is the way things are with the get/set tunnel helpers, but on the other hand this helper does not really have a concrete tie to them, so given we start fresh on this one, we should make both v4/v6 consistent and document it appropriately. Eyal, please respin the series with that. The rest was good to go from my pov. Thank you, Daniel