netdev - Re: [PATCH net-next v2 04/12] net: sched: flower: track filter deletion with 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: <20190305005652.6b39e464@redhat.com>
Date:   Tue, 5 Mar 2019 00:56:52 +0100
From:   Stefano Brivio <sbrivio@...hat.com>
To:     Vlad Buslov <vladbu@...lanox.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "jhs@...atatu.com" <jhs@...atatu.com>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>,
        "jiri@...nulli.us" <jiri@...nulli.us>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH net-next v2 04/12] net: sched: flower: track filter
 deletion with flag

On Mon, 4 Mar 2019 14:24:05 +0000
Vlad Buslov <vladbu@...lanox.com> wrote:

> On Fri 01 Mar 2019 at 23:51, Stefano Brivio <sbrivio@...hat.com> wrote:
> > Hi Vlad,
> >
> > On Wed, 27 Feb 2019 12:12:18 +0200
> > Vlad Buslov <vladbu@...lanox.com> wrote:
> >  
> >> -static bool __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,
> >> -			struct netlink_ext_ack *extack)
> >> +static int __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,
> >> +		       bool *last, struct netlink_ext_ack *extack)
> >>  {
> >>  	struct cls_fl_head *head = fl_head_dereference(tp);
> >>  	bool async = tcf_exts_get_net(&f->exts);
> >> -	bool last;
> >>  
> >> +	*last = false;
> >> +
> >> +	if (f->deleted)
> >> +		return -ENOENT;
> >> +
> >> +	f->deleted = true;  
> >
> > Now that I can read this more easily :) I have a doubt: you say this
> > flag "prevent[s] double deletion of filter by concurrent tasks".
> >
> > However, if this has no further protections (which I can't readily
> > see), I think this is racy:
> >
> > task 1				task 2
> > if (f->deleted) [false]
> > 				if (f->deleted) [false]
> > f->deleted = true;		f->deleted = true;
> >
> > what am I missing here?  
> 
> Of course! Lock is added in "[PATCH net-next v2 10/12] net: sched:
> flower: protect flower classifier state with spinlock". This is safe to
> do because everything is still protected by rtnl mutex until last patch
> in this series sets the TCF_PROTO_OPS_DOIT_UNLOCKED flag for flower.

Indeed! My bad, I forgot about 10/12 here.

-- 
Stefano