Subject: Re: [PATCH net-next v2 04/12] net: sched: flower: track filter deletion with flag

From: Vlad Buslov <[email-masked]>
Date: Mon, 4 Mar 2019 14:24:05 +0000

On Fri 01 Mar 2019 at 23:51, Stefano Brivio <[email-masked]> wrote:
> Hi Vlad,
>
> On Wed, 27 Feb 2019 12:12:18 +0200
> Vlad Buslov <[email-masked]> 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.