> From: [email protected] <[email protected]> On Behalf
> Of David Ahern
> Sent: Monday, August 31, 2020 12:54 PM
> To: Ramamurthy, Harshitha <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Cc: Duyck, Alexander H <[email protected]>; Herbert, Tom
> <[email protected]>
> Subject: Re: [PATCH bpf-next] bpf: add bpf_get_xdp_hash helper function
>
> On 8/31/20 1:25 PM, Harshitha Ramamurthy wrote:
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index
> > a613750d5515..bffe93b526e7 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -3576,6 +3576,14 @@ union bpf_attr {
> > * the data in *dst*. This is a wrapper of
> > copy_from_user().
> > * Return
> > * 0 on success, or a negative error in case of failure.
> > + *
> > + * u32 bpf_get_xdp_hash(struct xdp_buff *xdp_md)
>
> I thought there was a change recently making the uapi reference xdp_md;
> xdp_buff is not exported as part of the uapi.
Not sure what you mean - other xdp related helper functions still use xdp_buff
as an argument. Could you point me to an example of what you are referring to?
>
>
> > + * Description
> > + * Return the hash for the xdp context passed. This function
> > + * calls skb_flow_dissect in non-skb mode to calculate the
> > + * hash for the packet.
> > + * Return
> > + * The 32-bit hash.
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -3727,6 +3735,7 @@ union bpf_attr {
> > FN(inode_storage_delete), \
> > FN(d_path), \
> > FN(copy_from_user), \
> > + FN(get_xdp_hash), \
> > /* */
> >
> > /* integer value in 'imm' field of BPF_CALL instruction selects which
> > helper diff --git a/net/core/filter.c b/net/core/filter.c index
> > 47eef9a0be6a..cfb5a6aea6c3 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -3765,6 +3765,33 @@ static const struct bpf_func_proto
> bpf_xdp_redirect_map_proto = {
> > .arg3_type = ARG_ANYTHING,
> > };
> >
> > +BPF_CALL_1(bpf_get_xdp_hash, struct xdp_buff *, xdp) {
> > + void *data_end = xdp->data_end;
> > + struct ethhdr *eth = xdp->data;
> > + void *data = xdp->data;
> > + struct flow_keys keys;
> > + u32 ret = 0;
> > + int len;
> > +
> > + len = data_end - data;
> > + if (len <= 0)
> > + return ret;
>
> you should verify len covers the ethernet header. Looking at
> __skb_flow_dissect use of hlen presumes it exists.
Yes, I will make sure to return if len < sizeof(struct ethhdr)
>
> > + memset(&keys, 0, sizeof(keys));
> > + __skb_flow_dissect(dev_net(xdp->rxq->dev), NULL,
> &flow_keys_dissector,
> > + &keys, data, eth->h_proto, sizeof(*eth), len,
> > + FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL);
>
> By STOP_AT_FLOW_LABEL I take it you want this to be an L3 hash. Why not
> add a flags argument to the helper and let the hash be L3 or L4?
I wrote this exactly how skb_get_hash calls skb_flow_dissect - with the same
flag STOP_AT_FLOW_LABEL. So it should already cover L3 and L4 hash, right?
From what I understand STOP_AT_FLOW_LABEL flag is used to only stop parsing
when a flow label is seen in ipv6 packets.
>
>
> you should add test cases and have them cover the permutations - e.g., vlan,
> Q-in-Q, ipv4, ipv6, non-IP packet for L3 hash and then udp, tcp for L4 hash.
Sure, I will add test cases using this helper function.
Thanks for the feedback!
Harshitha
