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

[Davide Caratti <dcaratti@redhat.com>]

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!
-- 
davide