From: bpf-owner@vger.kernel.org <redacted> On Behalf
Of David Ahern
Sent: Monday, August 31, 2020 12:54 PM
To: Ramamurthy, Harshitha <redacted>;
bpf@vger.kernel.org; netdev@vger.kernel.org; ast@kernel.org;
daniel@iogearbox.net; davem@davemloft.net; kuba@kernel.org
Cc: Duyck, Alexander H <redacted>; Herbert, Tom
[off-list ref]
Subject: Re: [PATCH bpf-next] bpf: add bpf_get_xdp_hash helper function

On 8/31/20 1:25 PM, Harshitha Ramamurthy wrote:

quoted

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?

quoted

+ *	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 = {

quoted

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

quoted

+	memset(&keys, 0, sizeof(keys));
+	__skb_flow_dissect(dev_net(xdp->rxq->dev), NULL,

&flow_keys_dissector,

quoted

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