On Thu 11 Apr 2019 at 14:13, Ido Schimmel <ido...@idosch.org> wrote:
> On Fri, Apr 05, 2019 at 08:56:26PM +0300, Vlad Buslov wrote:
>> John reports:
>>
>> Recent refactoring of fl_change aims to use the classifier spinlock to
>> avoid the need for rtnl lock. In doing so, the fl_hw_replace_filer()
>> function was moved to before the lock is taken. This can create problems
>> for drivers if duplicate filters are created (commmon in ovs tc offload
>> due to filters being triggered by user-space matches).
>>
>> Drivers registered for such filters will now receive multiple copies of
>> the same rule, each with a different cookie value. This means that the
>> drivers would need to do a full match field lookup to determine
>> duplicates, repeating work that will happen in flower __fl_lookup().
>> Currently, drivers do not expect to receive duplicate filters.
>>
>> To fix this, verify that filter with same key is not present in flower
>> classifier hash table and insert the new filter to the flower hash table
>> before offloading it to hardware. Implement helper function
>> fl_ht_insert_unique() to atomically verify/insert a filter.
>>
>> This change makes filter visible to fast path at the beginning of
>> fl_change() function, which means it can no longer be freed directly in
>> case of error. Refactor fl_change() error handling code to deallocate the
>> filter with rcu timeout.
>>
>> Fixes: 620da4860827 ("net: sched: flower: refactor fl_change")
>> Reported-by: John Hurley <john.hur...@netronome.com>
>> Signed-off-by: Vlad Buslov <vla...@mellanox.com>
>
> Vlad,
>
> Our regression machines all hit a NULL pointer dereference [1] which I
> bisected to this patch. Created this reproducer that you can use:
>
> ip netns add ns1
> ip -n ns1 link add dev dummy1 type dummy
> tc -n ns1 qdisc add dev dummy1 clsact
> tc -n ns1 filter add dev dummy1 ingress pref 1 proto ip \
> flower skip_hw src_ip 192.0.2.1 action drop
> ip netns del ns1
>
> Can you please look into this? Thanks
Will do.
