matchall race (was: Re: [PATCH net-next] net/sched: cls_flower: verify root pointer before dereferncing it)

[Daniel Borkmann <daniel@iogearbox.net>]

[ Making this a different thread, since unrelated to the other one. ]

On 11/25/2016 07:23 AM, Cong Wang wrote:
> On Thu, Nov 24, 2016 at 4:17 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
[...]
>>
>> (Btw, matchall is still broken besides this fix. mall_delete() sets the
>>   RCU_INIT_POINTER(head->filter, NULL), so that a mall_delete() plus
>>   mall_destroy() combo doesn't free head->filter twice, but doing that is
>>   racy with mall_classify() where head->filter is dereferenced unchecked.
>>   Requires additional fix.)
>
> This seems due to matchall only has one filter per tp. But you don't need
> to worry since readers never read a freed pointer, right?

The race would be that CPU0 is in tc_ctl_tfilter() with RTM_DELTFILTER and
tcm_handle = 1 request. matchall has only single handle and id of it is 1
(or a prior user-specified one). So in tc_ctl_tfilter() ->get() will find it,
returns pointer as fh. Then we call ->delete(). mall_delete() sets head->filter
to NULL with RCU_INIT_POINTER() (head->filter is not even an RCU pointer btw),
real filter destruction comes via call_rcu(). If we got here, that tp is still
visible and not unlinked from tp chain. So in CPU1 tc_classify() executes
mall_classify(), we fetch the head (tp->root) and the corresponding head->filter
from there, and dereference it next. Probably best would have been to just
return -EOPNOTSUPP for ->delete() like in cls_cgroup case. It probably makes
sense to just defer real destruction to mall_destroy() instead of mall_delete()
if one wants to avoid extra !f test in mall_classify().