From mboxrd@z Thu Jan  1 00:00:00 1970
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Subject: Re: [PATCH bpf-next,v2 1/2] bpf: add helper for getting xfrm states
Date: Sun, 22 Apr 2018 18:34:31 -0600
Message-ID: <20180423003430.dxc3frkp3opbvl7f@ast-mbp>
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>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: netdev@vger.kernel.org, shmulik@metanetworks.com, ast@kernel.org,
        daniel@iogearbox.net, fw@strlen.de, steffen.klassert@secunet.com
To: Eyal Birger <eyal.birger@gmail.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-pf0-f196.google.com ([209.85.192.196]:34837 "EHLO
        mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1753669AbeDWAee (ORCPT
        <rfc822;netdev@vger.kernel.org>); Sun, 22 Apr 2018 20:34:34 -0400
Received: by mail-pf0-f196.google.com with SMTP id j5so7766963pfh.2
        for <netdev@vger.kernel.org>; Sun, 22 Apr 2018 17:34:34 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <20180420064356.7a703a1e@jimi>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On Fri, Apr 20, 2018 at 06:43:56AM +0300, Eyal Birger wrote:
> Hi,
> 
> On Wed, 18 Apr 2018 15:31:03 -0700
> Alexei Starovoitov <alexei.starovoitov@gmail.com> 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
> > > 
> 
> <snip>
>  
> > > +#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?

> > 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.