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

On Mon, 29 Aug 2016 17:06:34 +0200, Daniel Borkmann wrote:
> On 08/26/2016 08:06 PM, Jakub Kicinski wrote:
> > [...]
> > @@ -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]?

Sure!

> > +		if (!tc_flags_valid(gen_flags))
> > +			return -EINVAL;  
> 
> Shouldn't we: goto errout?

Ugh, right!  I'm missing:

	tcf_exts_destroy(&exts);

Thanks!