Re: [RFC Patch net-next 5/6] net_sched: use rcu in fast path

[John Fastabend <john.fastabend@gmail.com>]

On 16-09-06 07:52 AM, Eric Dumazet wrote:
> On Thu, 2016-09-01 at 22:57 -0700, Cong Wang wrote:
> 
> 
> 
> Missing changelog ?
> 

Yeah this stuff is a bit tricky more notes to walk us through
this would be helpful. (btw you can disregard my comment from
earlier this morning I've tracked most of this down now)

> Here I have no idea what you want to fix, since John already took care
> all this infra.
> 
> Adding extra rcu_dereference() and rcu_read_lock() while the critical
> RCU dereferences already happen in callers is not needed.
> 

Agreed drop all the extra rcu_read_lock/unlock here.

>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>> ---
>>  net/sched/act_api.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index 2f8db3c..fb6ff52 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -470,10 +470,14 @@ int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
>>  		goto exec_done;
>>  	}
>>  	for (i = 0; i < nr_actions; i++) {
>> -		const struct tc_action *a = actions[i];
>> +		const struct tc_action *a;
>>  
>> +		rcu_read_lock();
> 
> But the caller already has rcu_read_lock() or rcu_read_lock_bh()
> 
> This was done in commit 25d8c0d55f241ce2 ("net: rcu-ify tcf_proto")
> 
>> +		a = rcu_dereference(actions[i]);
> 
> 
> Add in your .config :
> CONFIG_SPARSE_RCU_POINTER=y
> make C=2 M=net/sched
> 
>>  repeat:
>>  		ret = a->ops->act(skb, a, res);
>> +		rcu_read_unlock();
>> +
>>  		if (ret == TC_ACT_REPEAT)
>>  			goto repeat;	/* we need a ttl - JHS */
>>  		if (ret != TC_ACT_PIPE)
> 
> 
> 
> I do not believe this patch is necessary.
> 
> Please add John as reviewer next time.
> 
> Thanks.
> 
> 

So the actual issue as I see it is with the late binding actions the
ones created with the ill documented 'tc action' syntax.

If you add a rule here and then bind it to a filter (stealing Jamals
example),

  tc actions add action skbedit mark 10 index 1
  tc filter add dev $DEV parent ffff: protocol ip prio 1 u32\
             match ip dst 17.0.0.1/32 flowid 1:10 action skbedit index 1

then you can modify this action later with the following

  tc actions replace action ....

So for an action such as act_mirred we may end up running with a
partially updated state from this,

	tcf_mirred_init(...)
	[...]

		m->tcf_action = parm->action;
	        m->tcfm_eaction = parm->eaction;
	
		[...]
	

and then in the execution of the action,


	tcf_mirred(...)
		[...]
	        retval = READ_ONCE(m->tcf_action);
		[...]
                if (m->tcfm_eaction != TCA_EGRESS_MIRROR)
		[...]


So its at least possible I think these could be interleaved on multiple
cpus.

Notice that some of the actions are fine though and don't have this
issue act_bpf for example is fine.

I think we can either fix it in the hash table create part of the
list as this series does or just let each action handle it on its own.

.John