On Thu, Nov 24, 2016 at 4:17 PM, Daniel Borkmann <dan...@iogearbox.net> wrote: > > > I'm not sure if setting a dummy object for each affected classifier is > making things better. Are you having this in mind as a target for -net? > > We do kfree_rcu() the head (tp->root) and likewise do we kfree_rcu() the > tp immediately after the callback. The head object's lifetime for such > classifiers is thus always bound to the tp lifetime. And besides that, > nothing apart from get() checks whether it's actually really NULL (and > that check in get() is odd anyway; some cls do it, some don't). >
Excellent point. I thought we should exclude any parallel readers when we call destroy(), you are taking a different approach by observing we only have to exclude readers when we really free them, this seems fine to me after a second thought, because the RCU API should take care of races with readers so as long as we free everything in RCU callback we are good. Hmm... But I may miss something since I am not an RCU expert. [...] > > (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?
