[ 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 <dan...@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().
