Re: [net-next PATCH 2/2] net: sched: cls_u32 changes to knode must appear atomic to readers

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

On 09/18/2014 04:38 AM, Jamal Hadi Salim wrote:
> On 09/17/14 15:12, John Fastabend wrote:
>> Changes to the cls_u32 classifier must appear atomic to the
>> readers. Before this patch if a change is requested for both
>> the exts and ifindex, first the ifindex is updated then the
>> exts with tcf_exts_change(). This opens a small window where
>> a reader can have a exts chain with an incorrect ifindex. This
>> violates the the RCU semantics.
>>
>> Here we resolve this by always passing u32_set_parms() a copy
>> of the tc_u_knode to work on and then inserting it into the hash
>> table after the updates have been successfully applied.
>>
>> Tested with the following short script:
>>
>
>>
>> #tc filter add dev p3p2 parent 8001:0 protocol ip prio 99 handle 1: \
>>            u32 divisor 256
>>
>> #tc filter add dev p3p2 parent 8001:0 protocol ip prio 99 \
>>            u32 link 1: hashkey mask ffffff00 at 12    \
>>            match ip src 192.168.8.0/2
>>
>> #tc filter add dev p3p2 parent 8001:0 protocol ip prio 102    \
>>            handle 1::10 u32 classid 1:2 ht 1:           \
>>            match ip src 192.168.8.0/8 match ip tos 0x0a 1e
>>
>> #tc filter change dev p3p2 parent 8001:0 protocol ip prio 102 \
>>          handle 1::10 u32 classid 1:2 ht 1:        \
>>          match ip src 1.1.0.0/8 match ip tos 0x0b 1e
>>
>> CC: Eric Dumazet <edumazet@google.com>
>> CC: Jamal Hadi Salim <jhs@mojatatu.com>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>
>
> Looks good to me.
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> cheers,
> jamal

Thanks for looking it over! I made v2 though to address a comment
that my variable/function names could be better and added a comment
around the perhaps tricky cases where it is safe to free the percpu
variables.

Because I did touch the patch and make some changes I dropped your
ACK. I always thought it was a bit of bad form to carry ack's around
after modifying the code without an explicit approval. Please add it
back though if you want.

Thanks,
John

-- 
John Fastabend         Intel Corporation