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 <alexei.starovoi...@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?
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
