Re: [PATCH bpf-next] bpf: add bpf_get_xdp_hash helper function

[David Ahern <dsahern@gmail.com>]

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.


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

> +	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?


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.