netdev - Re: [net-next PATCH v5 13/16] net: sched: make tc

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]

Date:	Fri, 12 Sep 2014 17:05:00 -0700
From:	John Fastabend <john.fastabend@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	xiyou.wangcong@...il.com, davem@...emloft.net, jhs@...atatu.com,
	netdev@...r.kernel.org, paulmck@...ux.vnet.ibm.com,
	brouer@...hat.com
Subject: Re: [net-next PATCH v5 13/16] net: sched: make tc_action safe to
 walk under RCU

On 09/12/2014 01:51 PM, Eric Dumazet wrote:
> On Fri, 2014-09-12 at 09:34 -0700, John Fastabend wrote:
>
>> Second there is a suspect usage of list_splice_init_rcu() in the
>> tcf_exts_change() routine. Notice how it is used twice in succession
>> and the second init works on the src tcf_exts. There is probably a
>> better way to accomplish that.

Not only is it suspect its broke as best as I can tell.

>>
>
>
>> +/* It is not safe to use src->actions after this due to _init_rcu usage
>> + * INIT_LIST_HEAD_RCU() is called on src->actions
>> + */
>>   void tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
>>   		     struct tcf_exts *src)
>>   {
>>   #ifdef CONFIG_NET_CLS_ACT
>>   	LIST_HEAD(tmp);
>> -	tcf_tree_lock(tp);
>> -	list_splice_init(&dst->actions, &tmp);



>> -	list_splice(&src->actions, &dst->actions);
>> -	tcf_tree_unlock(tp);

Here we have three lists,

	 tmp  (which has just been initialized)

	  dst -> act1 -> act2 -> ... -> actn ->

	  src -> acta -> actb -> ... -> actn ->

The active list is dst from the tc_classify and we want to
switch to the src list


>> +	list_splice_init_rcu(&dst->actions, &tmp, synchronize_rcu);

Now we spliced in tmp and did an INIT_LIST_HEAD_RCU() on dst->actions
(oops) so the lists look like this

	tmp -> act1 -> act2 -> ... -> actn ->

	dst

	src  -> acta -> actb -> ... -> actn ->

The active list is still dst from the tc_classify caller so now we are
missing both the old and new action lists. This is broke.


>> +	list_splice_init_rcu(&src->actions,
>> +			     &dst->actions,
>> +			     synchronize_rcu);

But the final state is what we wanted,

	tmp -> act1 -> act2 -> ... -> actn ->

	dst -> acta -> actb -> ... -> actn ->

	src

>>   	tcf_action_destroy(&tmp, TCA_ACT_UNBIND);

Finally clean it all up,

	tmp

	dst -> acta -> actb -> ... -> actn ->

	src


>>   #endif
>>   }
>
> I am afraid I do not understand this part.

Because it doesn't work as far as I can tell. What I really want here
is to swap the head pointer with,


	struct list_head *tmp = dst->actions;

	rcu_assign_pointer(dst->actions, src->actions)
	synchronize_rcu()
	tcf_action_destroy(tmp);


This requires a bit more work. I'll work out a patch after a bit more
thought.

Thanks again!

.John



-- 
John Fastabend         Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html