netdev - Re: [PATCH net-next 1/3] net/sched: Introduce action hash

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: <830de215-ec74-1c96-4a41-7b07608e3407@iogearbox.net>
Date:   Fri, 19 Jun 2020 01:42:54 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Ariel Levkovich <lariel@...lanox.com>, netdev@...r.kernel.org
Cc:     jiri@...nulli.us, kuba@...nel.org, jhs@...atatu.com,
        xiyou.wangcong@...il.com, ast@...nel.org,
        Jiri Pirko <jiri@...lanox.com>, bpf@...r.kernel.org
Subject: Re: [PATCH net-next 1/3] net/sched: Introduce action hash

On 6/19/20 12:15 AM, Ariel Levkovich wrote:
> Allow setting a hash value to a packet for a future match.
> 
> The action will determine the packet's hash result according to
> the selected hash type.
> 
> The first option is to select a basic asymmetric l4 hash calculation
> on the packet headers which will either use the skb->hash value as
> if such was already calculated and set by the device driver, or it
> will perform the kernel jenkins hash function on the packet which will
> generate the result otherwise.
> 
> The other option is for user to provide an BPF program which is
> dedicated to calculate the hash. In such case the program is loaded
> and used by tc to perform the hash calculation and provide it to
> the hash action to be stored in skb->hash field.

The commit description is a bit misleading, since setting a custom skb->hash
from BPF program can be done already via {cls,act}_bpf from *within* BPF in
tc for ~3 years by now via [0].

   [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ded092cd73c2c56a394b936f86897f29b2e131c0

> The BPF option can be useful for future HW offload support of the hash
> calculation by emulating the HW hash function when it's different than
> the kernel's but yet we want to maintain consistency between the SW and
> the HW.

... use case should say 'dummy SW module for HW offload'. ;-/

> Usage is as follows:
> 
> $ tc filter add dev ens1f0_0 ingress \
> prio 1 chain 0 proto ip \
> flower ip_proto tcp \
> action hash bpf object-file <bpf file> \
> action goto chain 2
[...]
> 
> Signed-off-by: Ariel Levkovich <lariel@...lanox.com>
> Reviewed-by: Jiri Pirko <jiri@...lanox.com>
[...]
> +static int tcf_hash_act(struct sk_buff *skb, const struct tc_action *a,
> +			struct tcf_result *res)
> +{
> +	struct tcf_hash *h = to_hash(a);
> +	struct tcf_hash_params *p;
> +	int action;
> +	u32 hash;
> +
> +	tcf_lastuse_update(&h->tcf_tm);
> +	tcf_action_update_bstats(&h->common, skb);
> +
> +	p = rcu_dereference_bh(h->hash_p);
> +
> +	action = READ_ONCE(h->tcf_action);
> +
> +	switch (p->alg) {
> +	case TCA_HASH_ALG_L4:
> +		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, p->basis);
> +		__skb_set_sw_hash(skb, hash, skb->l4_hash);
> +		break;
> +	case TCA_HASH_ALG_BPF:
> +		__skb_push(skb, skb->mac_len);
> +		bpf_compute_data_pointers(skb);
> +		hash = BPF_PROG_RUN(p->prog, skb);
> +		__skb_pull(skb, skb->mac_len);
> +		/* The BPF program hash function type is
> +		 * unknown so only the sw hash bit is set.
> +		 */
> +		__skb_set_sw_hash(skb, hash, false);

Given this runs BPF_PROG_TYPE_SCHED_ACT-typed programs, act_hash.c now becomes
pretty much another clone of {cls,act}_bpf.c with the full feature set in terms
of what it can do with the program, like header mangling, encap/decap, setting
tunnel keys, cloning + redirecting skbs, etc. This is rather misleading given you
basically would only want the direct pkt access bits and plain insns for calc'ing
the hash, but none of the other features.

> +		break;
> +	}
> +
> +	return action;
> +}
[...]
> +static int tcf_hash_bpf_init(struct nlattr **tb, struct tcf_hash_params *params)
> +{
> +	struct bpf_prog *fp;
> +	char *name = NULL;
> +	u32 bpf_fd;
> +
> +	bpf_fd = nla_get_u32(tb[TCA_HASH_BPF_FD]);
> +
> +	fp = bpf_prog_get_type(bpf_fd, BPF_PROG_TYPE_SCHED_ACT);
> +	if (IS_ERR(fp))
> +		return PTR_ERR(fp);
> +
> +	if (tb[TCA_HASH_BPF_NAME]) {
> +		name = nla_memdup(tb[TCA_HASH_BPF_NAME], GFP_KERNEL);
> +		if (!name) {
> +			bpf_prog_put(fp);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	params->bpf_name = name;
> +	params->prog = fp;
> +
> +	return 0;
> +}