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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5ba341b6-dd8d-9861-7bb4-4d8b900f7d56@mellanox.com>
Date:   Wed, 15 Jul 2020 10:25:29 -0400
From:   Ariel Levkovich <lariel@...lanox.com>
To:     Davide Caratti <dcaratti@...hat.com>, netdev@...r.kernel.org
Cc:     jiri@...nulli.us, kuba@...nel.org, jhs@...atatu.com,
        xiyou.wangcong@...il.com, ast@...nel.org, daniel@...earbox.net,
        Jiri Pirko <jiri@...lanox.com>
Subject: Re: [PATCH net-next v3 1/4] net/sched: Add skb->hash field editing
 via act_skbedit

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@...lanox.com>
>> Reviewed-by: Jiri Pirko <jiri@...lanox.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.