netdev - Re: [RFCv2 03/16] net: cls_bpf: limit hardware offload by software-only flag

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: <57C44F7A.5060501@iogearbox.net>
Date:   Mon, 29 Aug 2016 17:06:34 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        netdev@...r.kernel.org
CC:     ast@...nel.org, dinan.gunawardena@...ronome.com, jiri@...nulli.us,
        john.fastabend@...il.com, kubakici@...pl
Subject: Re: [RFCv2 03/16] net: cls_bpf: limit hardware offload by software-only
 flag

On 08/26/2016 08:06 PM, Jakub Kicinski wrote:
> Add cls_bpf support for the TCA_CLS_FLAGS_SKIP_HW flag.
> Unlike U32 and flower cls_bpf already has some netlink
> flags defined.  I chose to create a new attribute to be
> able to use the same flag values as the above.
>
> Unknown flags are ignored and not reported upon dump.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
[...]
> @@ -55,6 +58,7 @@ struct cls_bpf_prog {
>   static const struct nla_policy bpf_policy[TCA_BPF_MAX + 1] = {
>   	[TCA_BPF_CLASSID]	= { .type = NLA_U32 },
>   	[TCA_BPF_FLAGS]		= { .type = NLA_U32 },
> +	[TCA_BPF_FLAGS_GEN]	= { .type = NLA_U32 },
>   	[TCA_BPF_FD]		= { .type = NLA_U32 },
>   	[TCA_BPF_NAME]		= { .type = NLA_NUL_STRING, .len = CLS_BPF_NAME_LEN },
>   	[TCA_BPF_OPS_LEN]	= { .type = NLA_U16 },
> @@ -156,6 +160,7 @@ static int cls_bpf_offload_cmd(struct tcf_proto *tp, struct cls_bpf_prog *prog,
>   	bpf_offload.filter = prog->filter;
>   	bpf_offload.name = prog->bpf_name;
>   	bpf_offload.exts_integrated = prog->exts_integrated;
> +	bpf_offload.gen_flags = prog->gen_flags;
>
>   	return dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
>   					     tp->protocol, &offload);
> @@ -169,14 +174,14 @@ static void cls_bpf_offload(struct tcf_proto *tp, struct cls_bpf_prog *prog,
>   	enum tc_clsbpf_command cmd;
>
>   	if (oldprog && oldprog->offloaded) {
> -		if (tc_should_offload(dev, tp, 0)) {
> +		if (tc_should_offload(dev, tp, prog->gen_flags)) {
>   			cmd = TC_CLSBPF_REPLACE;
>   		} else {
>   			obj = oldprog;
>   			cmd = TC_CLSBPF_DESTROY;
>   		}
>   	} else {
> -		if (!tc_should_offload(dev, tp, 0))
> +		if (!tc_should_offload(dev, tp, prog->gen_flags))
>   			return;
>   		cmd = TC_CLSBPF_ADD;
>   	}
> @@ -372,6 +377,7 @@ static int cls_bpf_modify_existing(struct net *net, struct tcf_proto *tp,
>   {
>   	bool is_bpf, is_ebpf, have_exts = false;
>   	struct tcf_exts exts;
> +	u32 gen_flags = 0;
>   	int ret;
>
>   	is_bpf = tb[TCA_BPF_OPS_LEN] && tb[TCA_BPF_OPS];
> @@ -396,8 +402,16 @@ static int cls_bpf_modify_existing(struct net *net, struct tcf_proto *tp,
>
>   		have_exts = bpf_flags & TCA_BPF_FLAG_ACT_DIRECT;
>   	}
> +	if (tb[TCA_BPF_FLAGS_GEN]) {
> +		gen_flags = nla_get_u32(tb[TCA_BPF_FLAGS_GEN]);
> +		/* Make sure dump doesn't report back flags we don't handle */
> +		gen_flags &= CLS_BPF_SUPPORTED_GEN_FLAGS;

Instead of above rather ...

	if (gen_flags & ~CLS_BPF_SUPPORTED_GEN_FLAGS) {
		ret = -EINVAL;
		goto errout;
	}

... so that we can handle further additions properly like we do with
tb[TCA_BPF_FLAGS]?

> +		if (!tc_flags_valid(gen_flags))
> +			return -EINVAL;

Shouldn't we: goto errout?

> +	}
>
>   	prog->exts_integrated = have_exts;
> +	prog->gen_flags = gen_flags;
>
>   	ret = is_bpf ? cls_bpf_prog_from_ops(tb, prog) :
>   		       cls_bpf_prog_from_efd(tb, prog, tp);
> @@ -569,6 +583,9 @@ static int cls_bpf_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
>   		bpf_flags |= TCA_BPF_FLAG_ACT_DIRECT;
>   	if (bpf_flags && nla_put_u32(skb, TCA_BPF_FLAGS, bpf_flags))
>   		goto nla_put_failure;
> +	if (prog->gen_flags &&
> +	    nla_put_u32(skb, TCA_BPF_FLAGS_GEN, prog->gen_flags))
> +		goto nla_put_failure;
>
>   	nla_nest_end(skb, nest);

Rest looks good:

Acked-by: Daniel Borkmann <daniel@...earbox.net>