Subject: Re: [RFC 03/12] net: cls_bpf: limit hardware offload by software-only flag

From: Daniel Borkmann <[email-masked]>
Date: Wed, 01 Jun 2016 21:40:23 +0200

On 06/01/2016 06:50 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.

Yeah, that's totally fine to make it a new flag attribute.

> Unknown flags are ignored and not reported upon dump.
>
> Signed-off-by: Jakub Kicinski <[email-masked]>
> Reviewed-by: Dinan Gunawardena <[email-masked]>
> Reviewed-by: Simon Horman <[email-masked]>

[...]
> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> index f4297c8a42fe..93a86edf3bd8 100644
> --- a/include/uapi/linux/pkt_cls.h
> +++ b/include/uapi/linux/pkt_cls.h
> @@ -395,6 +395,7 @@ enum {
>   	TCA_BPF_FD,
>   	TCA_BPF_NAME,
>   	TCA_BPF_FLAGS,
> +	TCA_BPF_GEN_TCA_FLAGS,

Small nit for the non-RFC set: I'd simply name that TCA_BPF_FLAGS_GEN.

>   	__TCA_BPF_MAX,
>   };
[...]
> @@ -400,8 +406,11 @@ 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_GEN_TCA_FLAGS])
> +		gen_flags = nla_get_u32(tb[TCA_BPF_GEN_TCA_FLAGS]);
>
>   	prog->exts_integrated = have_exts;
> +	prog->gen_flags = gen_flags & CLS_BPF_SUPPORTED_GEN_FLAGS;

Invalid flags should probably be rejected here with -EINVAL or something.

>   	ret = is_bpf ? cls_bpf_prog_from_ops(tb, prog) :
>   		       cls_bpf_prog_from_efd(tb, prog, tp);
> @@ -568,6 +577,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_GEN_TCA_FLAGS, prog->gen_flags))
> +		goto nla_put_failure;
>
>   	nla_nest_end(skb, nest);
>
>

Otherwise looks good:

Acked-by: Daniel Borkmann <[email-masked]>