diff mbox series

[net-next,v3,1/4] net/sched: Add skb->hash field editing via act_skbedit

Message ID 20200711212848.20914-2-lariel@mellanox.com
State Changes Requested
Delegated to: David Miller
Headers show
Series ] TC datapath hash api | expand

Commit Message

Ariel Levkovich July 11, 2020, 9:28 p.m. UTC 
Extend act_skbedit api to allow writing into skb->hash
field.

To modify skb->hash user selects the hash algorithm
to use for the hash computation and can provide a
hash basis value to be used in the calculation.
The hash value will be calculated on the packet in the
datapath and will be set into skb->hash field.

Current implementation supports only the asymmetric l4 hash
algorithm that first checks whether the skb->hash was already
set with l4 hash value (possibly by the device driver) and uses
the existing value. If hash value wasn't set, it computes the
hash value in place using the kernel implementation of the
Jenkins hash algorithm.

Usage example:

$ tc filter add dev ens1f0_0 ingress \
prio 1 chain 0 proto ip \
flower ip_proto tcp \
action skbedit hash asym_l4 basis 5 \
action goto chain 2

Signed-off-by: Ariel Levkovich <lariel@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
 include/net/tc_act/tc_skbedit.h        |  2 ++
 include/uapi/linux/tc_act/tc_skbedit.h |  7 +++++
 net/sched/act_skbedit.c                | 38 ++++++++++++++++++++++++++
 3 files changed, 47 insertions(+)

Comments

Davide Caratti July 13, 2020, 5:11 p.m. UTC | #1 
On Sun, 2020-07-12 at 00:28 +0300, Ariel Levkovich wrote:
> Extend act_skbedit api to allow writing into skb->hash
> field.
> 
[...]

> Usage example:
> 
> $ tc filter add dev ens1f0_0 ingress \
> prio 1 chain 0 proto ip \
> flower ip_proto tcp \
> action skbedit hash asym_l4 basis 5 \
> action goto chain 2

hello Ariel, thanks for the patch!

> Signed-off-by: Ariel Levkovich <lariel@mellanox.com>
> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  include/net/tc_act/tc_skbedit.h        |  2 ++
>  include/uapi/linux/tc_act/tc_skbedit.h |  7 +++++
>  net/sched/act_skbedit.c                | 38 ++++++++++++++++++++++++++
>  3 files changed, 47 insertions(+)

this diffstat is ok for l4 hash calculation :)

> diff --git a/include/net/tc_act/tc_skbedit.h b/include/net/tc_act/tc_skbedit.h
> index 00bfee70609e..44a8a4625556 100644
> --- a/include/net/tc_act/tc_skbedit.h
> +++ b/include/net/tc_act/tc_skbedit.h
> @@ -18,6 +18,8 @@ struct tcf_skbedit_params {
>  	u32 mask;
>  	u16 queue_mapping;
>  	u16 ptype;
> +	u32 hash_alg;
> +	u32 hash_basis;
>  	struct rcu_head rcu;
>  };
>  
> diff --git a/include/uapi/linux/tc_act/tc_skbedit.h b/include/uapi/linux/tc_act/tc_skbedit.h
> index 800e93377218..5877811b093b 100644
> --- a/include/uapi/linux/tc_act/tc_skbedit.h
> +++ b/include/uapi/linux/tc_act/tc_skbedit.h
> @@ -29,6 +29,11 @@
>  #define SKBEDIT_F_PTYPE			0x8
>  #define SKBEDIT_F_MASK			0x10
>  #define SKBEDIT_F_INHERITDSFIELD	0x20
> +#define SKBEDIT_F_HASH			0x40
> +
> +enum {
> +	TCA_SKBEDIT_HASH_ALG_ASYM_L4,
> +};

nit:

it's a common practice, when specifying enums in the uAPI, to set the
first value  "UNSPEC", and last one as "MAX":

enum {
	TCA_SKBEDIT_HASH_ALG_UNSPEC,
	TCA_SKBEDIT_HASH_ALG_ASYM_L4,
	__TCA_SKBEDIT_HASH_ALG_MAX
};

see below the rationale:

>  struct tc_skbedit {
>  	tc_gen;
> @@ -45,6 +50,8 @@ enum {
>  	TCA_SKBEDIT_PTYPE,
>  	TCA_SKBEDIT_MASK,
>  	TCA_SKBEDIT_FLAGS,
> +	TCA_SKBEDIT_HASH,
> +	TCA_SKBEDIT_HASH_BASIS,
>  	__TCA_SKBEDIT_MAX
>  };
>  #define TCA_SKBEDIT_MAX (__TCA_SKBEDIT_MAX - 1)
> diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
> index b125b2be4467..2cc66c798afb 100644
> --- a/net/sched/act_skbedit.c
> +++ b/net/sched/act_skbedit.c
> @@ -66,6 +66,20 @@ static int tcf_skbedit_act(struct sk_buff *skb, const struct tc_action *a,
>  	}
>  	if (params->flags & SKBEDIT_F_PTYPE)
>  		skb->pkt_type = params->ptype;
> +
> +	if (params->flags & SKBEDIT_F_HASH) {
> +		u32 hash;
> +
> +		hash = skb_get_hash(skb);
> +		/* If a hash basis was provided, add it into
> +		 * hash calculation here and re-set skb->hash
> +		 * to the new result with sw_hash indication
> +		 * and keeping the l4 hash indication.
> +		 */
> +		hash = jhash_1word(hash, params->hash_basis);
> +		__skb_set_sw_hash(skb, hash, skb->l4_hash);
> +	}

in this way you don't need to define a value in 'flags'
(SKBEDIT_F_HASH), you just need to check if params->hash_alg is not
zero:
	if (params->hash_alg) {
		....
	}

>  	return action;
>  
>  err:
> @@ -91,6 +105,8 @@ static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
>  	[TCA_SKBEDIT_PTYPE]		= { .len = sizeof(u16) },
>  	[TCA_SKBEDIT_MASK]		= { .len = sizeof(u32) },
>  	[TCA_SKBEDIT_FLAGS]		= { .len = sizeof(u64) },
> +	[TCA_SKBEDIT_HASH]		= { .len = sizeof(u32) },
> +	[TCA_SKBEDIT_HASH_BASIS]	= { .len = sizeof(u32) },
>  };
>  
>  static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
> @@ -107,6 +123,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
>  	struct tcf_skbedit *d;
>  	u32 flags = 0, *priority = NULL, *mark = NULL, *mask = NULL;
>  	u16 *queue_mapping = NULL, *ptype = NULL;
> +	u32 hash_alg, hash_basis = 0;
>  	bool exists = false;
>  	int ret = 0, err;
>  	u32 index;
> @@ -156,6 +173,17 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
>  			flags |= SKBEDIT_F_INHERITDSFIELD;
>  	}
>  
> +	if (tb[TCA_SKBEDIT_HASH] != NULL) {
> +		hash_alg = nla_get_u32(tb[TCA_SKBEDIT_HASH]);
> +		if (hash_alg > TCA_SKBEDIT_HASH_ALG_ASYM_L4)
> +			return -EINVAL;

moreover, even without doing the strict validation, when somebody in the
future will extend the uAPI, he will not need to change the line above.
The following test will validate all good values of hash_alg:

	if (!hash_alg || hash_alg >= __TCA_SKBEDIT_HASH_ALG_MAX) {
		NL_SET_ERR_MSG_MOD(extack, "hash_alg is out of range");
		return -EINVAL;
 	}

WDYT?

thanks!
David Ahern July 15, 2020, 1:14 a.m. UTC | #2 
On 7/11/20 3:28 PM, Ariel Levkovich wrote:
> @@ -156,6 +173,17 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
>  			flags |= SKBEDIT_F_INHERITDSFIELD;
>  	}
>  
> +	if (tb[TCA_SKBEDIT_HASH] != NULL) {
> +		hash_alg = nla_get_u32(tb[TCA_SKBEDIT_HASH]);
> +		if (hash_alg > TCA_SKBEDIT_HASH_ALG_ASYM_L4)
> +			return -EINVAL;

tcf_skbedit_init has an extack argument, so fill in a message stating
why it is failing EINVAL.
Ariel Levkovich July 15, 2020, 2:25 p.m. UTC | #3 
On 7/13/20 1:11 PM, Davide Caratti wrote:
> On Sun, 2020-07-12 at 00:28 +0300, Ariel Levkovich wrote:
>> Extend act_skbedit api to allow writing into skb->hash
>> field.
>>
> [...]
>
>> Usage example:
>>
>> $ tc filter add dev ens1f0_0 ingress \
>> prio 1 chain 0 proto ip \
>> flower ip_proto tcp \
>> action skbedit hash asym_l4 basis 5 \
>> action goto chain 2
> hello Ariel, thanks for the patch!
>
>> Signed-off-by: Ariel Levkovich <lariel@mellanox.com>
>> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>>   include/net/tc_act/tc_skbedit.h        |  2 ++
>>   include/uapi/linux/tc_act/tc_skbedit.h |  7 +++++
>>   net/sched/act_skbedit.c                | 38 ++++++++++++++++++++++++++
>>   3 files changed, 47 insertions(+)
> this diffstat is ok for l4 hash calculation :)
>
>> diff --git a/include/net/tc_act/tc_skbedit.h b/include/net/tc_act/tc_skbedit.h
>> index 00bfee70609e..44a8a4625556 100644
>> --- a/include/net/tc_act/tc_skbedit.h
>> +++ b/include/net/tc_act/tc_skbedit.h
>> @@ -18,6 +18,8 @@ struct tcf_skbedit_params {
>>   	u32 mask;
>>   	u16 queue_mapping;
>>   	u16 ptype;
>> +	u32 hash_alg;
>> +	u32 hash_basis;
>>   	struct rcu_head rcu;
>>   };
>>   
>> diff --git a/include/uapi/linux/tc_act/tc_skbedit.h b/include/uapi/linux/tc_act/tc_skbedit.h
>> index 800e93377218..5877811b093b 100644
>> --- a/include/uapi/linux/tc_act/tc_skbedit.h
>> +++ b/include/uapi/linux/tc_act/tc_skbedit.h
>> @@ -29,6 +29,11 @@
>>   #define SKBEDIT_F_PTYPE			0x8
>>   #define SKBEDIT_F_MASK			0x10
>>   #define SKBEDIT_F_INHERITDSFIELD	0x20
>> +#define SKBEDIT_F_HASH			0x40
>> +
>> +enum {
>> +	TCA_SKBEDIT_HASH_ALG_ASYM_L4,
>> +};
> nit:
>
> it's a common practice, when specifying enums in the uAPI, to set the
> first value  "UNSPEC", and last one as "MAX":
>
> enum {
> 	TCA_SKBEDIT_HASH_ALG_UNSPEC,
> 	TCA_SKBEDIT_HASH_ALG_ASYM_L4,
> 	__TCA_SKBEDIT_HASH_ALG_MAX
> };
>
> see below the rationale:


Agree. Missed that. Actual enums should start at 1.

>
>>   struct tc_skbedit {
>>   	tc_gen;
>> @@ -45,6 +50,8 @@ enum {
>>   	TCA_SKBEDIT_PTYPE,
>>   	TCA_SKBEDIT_MASK,
>>   	TCA_SKBEDIT_FLAGS,
>> +	TCA_SKBEDIT_HASH,
>> +	TCA_SKBEDIT_HASH_BASIS,
>>   	__TCA_SKBEDIT_MAX
>>   };
>>   #define TCA_SKBEDIT_MAX (__TCA_SKBEDIT_MAX - 1)
>> diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
>> index b125b2be4467..2cc66c798afb 100644
>> --- a/net/sched/act_skbedit.c
>> +++ b/net/sched/act_skbedit.c
>> @@ -66,6 +66,20 @@ static int tcf_skbedit_act(struct sk_buff *skb, const struct tc_action *a,
>>   	}
>>   	if (params->flags & SKBEDIT_F_PTYPE)
>>   		skb->pkt_type = params->ptype;
>> +
>> +	if (params->flags & SKBEDIT_F_HASH) {
>> +		u32 hash;
>> +
>> +		hash = skb_get_hash(skb);
>> +		/* If a hash basis was provided, add it into
>> +		 * hash calculation here and re-set skb->hash
>> +		 * to the new result with sw_hash indication
>> +		 * and keeping the l4 hash indication.
>> +		 */
>> +		hash = jhash_1word(hash, params->hash_basis);
>> +		__skb_set_sw_hash(skb, hash, skb->l4_hash);
>> +	}
> in this way you don't need to define a value in 'flags'
> (SKBEDIT_F_HASH), you just need to check if params->hash_alg is not
> zero:
> 	if (params->hash_alg) {
> 		....
> 	}
>
>>   	return action;
>>   
>>   err:
>> @@ -91,6 +105,8 @@ static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
>>   	[TCA_SKBEDIT_PTYPE]		= { .len = sizeof(u16) },
>>   	[TCA_SKBEDIT_MASK]		= { .len = sizeof(u32) },
>>   	[TCA_SKBEDIT_FLAGS]		= { .len = sizeof(u64) },
>> +	[TCA_SKBEDIT_HASH]		= { .len = sizeof(u32) },
>> +	[TCA_SKBEDIT_HASH_BASIS]	= { .len = sizeof(u32) },
>>   };
>>   
>>   static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
>> @@ -107,6 +123,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
>>   	struct tcf_skbedit *d;
>>   	u32 flags = 0, *priority = NULL, *mark = NULL, *mask = NULL;
>>   	u16 *queue_mapping = NULL, *ptype = NULL;
>> +	u32 hash_alg, hash_basis = 0;
>>   	bool exists = false;
>>   	int ret = 0, err;
>>   	u32 index;
>> @@ -156,6 +173,17 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
>>   			flags |= SKBEDIT_F_INHERITDSFIELD;
>>   	}
>>   
>> +	if (tb[TCA_SKBEDIT_HASH] != NULL) {
>> +		hash_alg = nla_get_u32(tb[TCA_SKBEDIT_HASH]);
>> +		if (hash_alg > TCA_SKBEDIT_HASH_ALG_ASYM_L4)
>> +			return -EINVAL;
> moreover, even without doing the strict validation, when somebody in the
> future will extend the uAPI, he will not need to change the line above.
> The following test will validate all good values of hash_alg:
>
> 	if (!hash_alg || hash_alg >= __TCA_SKBEDIT_HASH_ALG_MAX) {
> 		NL_SET_ERR_MSG_MOD(extack, "hash_alg is out of range");
> 		return -EINVAL;
>   	}
>
> WDYT?
>
> thanks!

I actually thought about it during implementation. Dropped it eventually.


Thanks for the comments.
diff mbox series

Patch

diff --git a/include/net/tc_act/tc_skbedit.h b/include/net/tc_act/tc_skbedit.h
index 00bfee70609e..44a8a4625556 100644
--- a/include/net/tc_act/tc_skbedit.h
+++ b/include/net/tc_act/tc_skbedit.h
@@ -18,6 +18,8 @@  struct tcf_skbedit_params {
 	u32 mask;
 	u16 queue_mapping;
 	u16 ptype;
+	u32 hash_alg;
+	u32 hash_basis;
 	struct rcu_head rcu;
 };
 
diff --git a/include/uapi/linux/tc_act/tc_skbedit.h b/include/uapi/linux/tc_act/tc_skbedit.h
index 800e93377218..5877811b093b 100644
--- a/include/uapi/linux/tc_act/tc_skbedit.h
+++ b/include/uapi/linux/tc_act/tc_skbedit.h
@@ -29,6 +29,11 @@ 
 #define SKBEDIT_F_PTYPE			0x8
 #define SKBEDIT_F_MASK			0x10
 #define SKBEDIT_F_INHERITDSFIELD	0x20
+#define SKBEDIT_F_HASH			0x40
+
+enum {
+	TCA_SKBEDIT_HASH_ALG_ASYM_L4,
+};
 
 struct tc_skbedit {
 	tc_gen;
@@ -45,6 +50,8 @@  enum {
 	TCA_SKBEDIT_PTYPE,
 	TCA_SKBEDIT_MASK,
 	TCA_SKBEDIT_FLAGS,
+	TCA_SKBEDIT_HASH,
+	TCA_SKBEDIT_HASH_BASIS,
 	__TCA_SKBEDIT_MAX
 };
 #define TCA_SKBEDIT_MAX (__TCA_SKBEDIT_MAX - 1)
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index b125b2be4467..2cc66c798afb 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -66,6 +66,20 @@  static int tcf_skbedit_act(struct sk_buff *skb, const struct tc_action *a,
 	}
 	if (params->flags & SKBEDIT_F_PTYPE)
 		skb->pkt_type = params->ptype;
+
+	if (params->flags & SKBEDIT_F_HASH) {
+		u32 hash;
+
+		hash = skb_get_hash(skb);
+		/* If a hash basis was provided, add it into
+		 * hash calculation here and re-set skb->hash
+		 * to the new result with sw_hash indication
+		 * and keeping the l4 hash indication.
+		 */
+		hash = jhash_1word(hash, params->hash_basis);
+		__skb_set_sw_hash(skb, hash, skb->l4_hash);
+	}
+
 	return action;
 
 err:
@@ -91,6 +105,8 @@  static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
 	[TCA_SKBEDIT_PTYPE]		= { .len = sizeof(u16) },
 	[TCA_SKBEDIT_MASK]		= { .len = sizeof(u32) },
 	[TCA_SKBEDIT_FLAGS]		= { .len = sizeof(u64) },
+	[TCA_SKBEDIT_HASH]		= { .len = sizeof(u32) },
+	[TCA_SKBEDIT_HASH_BASIS]	= { .len = sizeof(u32) },
 };
 
 static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
@@ -107,6 +123,7 @@  static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 	struct tcf_skbedit *d;
 	u32 flags = 0, *priority = NULL, *mark = NULL, *mask = NULL;
 	u16 *queue_mapping = NULL, *ptype = NULL;
+	u32 hash_alg, hash_basis = 0;
 	bool exists = false;
 	int ret = 0, err;
 	u32 index;
@@ -156,6 +173,17 @@  static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 			flags |= SKBEDIT_F_INHERITDSFIELD;
 	}
 
+	if (tb[TCA_SKBEDIT_HASH] != NULL) {
+		hash_alg = nla_get_u32(tb[TCA_SKBEDIT_HASH]);
+		if (hash_alg > TCA_SKBEDIT_HASH_ALG_ASYM_L4)
+			return -EINVAL;
+
+		flags |= SKBEDIT_F_HASH;
+
+		if (tb[TCA_SKBEDIT_HASH_BASIS])
+			hash_basis = nla_get_u32(tb[TCA_SKBEDIT_HASH_BASIS]);
+	}
+
 	parm = nla_data(tb[TCA_SKBEDIT_PARMS]);
 	index = parm->index;
 	err = tcf_idr_check_alloc(tn, &index, a, bind);
@@ -213,6 +241,10 @@  static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 	params_new->mask = 0xffffffff;
 	if (flags & SKBEDIT_F_MASK)
 		params_new->mask = *mask;
+	if (flags & SKBEDIT_F_HASH) {
+		params_new->hash_alg = hash_alg;
+		params_new->hash_basis = hash_basis;
+	}
 
 	spin_lock_bh(&d->tcf_lock);
 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
@@ -276,6 +308,12 @@  static int tcf_skbedit_dump(struct sk_buff *skb, struct tc_action *a,
 	if (pure_flags != 0 &&
 	    nla_put(skb, TCA_SKBEDIT_FLAGS, sizeof(pure_flags), &pure_flags))
 		goto nla_put_failure;
+	if (params->flags & SKBEDIT_F_HASH) {
+		if (nla_put_u32(skb, TCA_SKBEDIT_HASH, params->hash_alg))
+			goto nla_put_failure;
+		if (nla_put_u32(skb, TCA_SKBEDIT_HASH_BASIS, params->hash_basis))
+			goto nla_put_failure;
+	}
 
 	tcf_tm_dump(&t, &d->tcf_tm);
 	if (nla_put_64bit(skb, TCA_SKBEDIT_TM, sizeof(t), &t, TCA_SKBEDIT_PAD))