[ + John ]
On 11/22/2016 03:48 PM, Jiri Pirko wrote:
Tue, Nov 22, 2016 at 03:25:26PM CET, r...@mellanox.com wrote:
tp->root is being allocated in init() time and kfreed in destroy()
however it is being dereferenced in classify() path.
We could be in classify() path after destroy() was called and thus
tp->root is null. Verifying if tp->root is null in classify() path
is enough because it's being freed with kfree_rcu() and classify()
path is under rcu_read_lock().
Fixes: 1e052be69d04 ("net_sched: destroy proto tp when all filters are gone")
Signed-off-by: Roi Dayan <r...@mellanox.com>
Cc: Cong Wang <cw...@twopensource.com>
This is correct
Reviewed-by: Jiri Pirko <j...@mellanox.com>
The other way to fix this would be to move tp->ops->destroy call to
call_rcu phase. That would require bigger changes though. net-next
perhaps?
Hmm, I don't think we want to have such an additional test in fast
path for each and every classifier. Can we think of ways to avoid that?
My question is, since we unlink individual instances from such tp-internal
lists through RCU and release the instance through call_rcu() as well as
the head (tp->root) via kfree_rcu() eventually, against what are we protecting
setting RCU_INIT_POINTER(tp->root, NULL) in ->destroy() callback? Something
not respecting grace period?
The only thing that actually checks if tp->root is NULL right now is the
get() callback. Is that the reason why tp->root is RCU'ified? John?
Thanks,
Daniel
Hi Cong, all
As stated above, the issue was introduced with commit 1e052be69d04 ("net_sched:
destroy
proto tp when all filters are gone"). This patch provides a fix only for
cls_flower where
I succeeded in reproducing the issue. Cong, if you can/want to come up with a
fix that
will be applicable for all the others classifiners, I am fine with that.
Thanks,
Roi
net/sched/cls_flower.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index e8dd09a..88a26c4 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -135,7 +135,7 @@ static int fl_classify(struct sk_buff *skb, const struct
tcf_proto *tp,
struct fl_flow_key skb_mkey;
struct ip_tunnel_info *info;
- if (!atomic_read(&head->ht.nelems))
+ if (!head || !atomic_read(&head->ht.nelems))
return -1;
fl_clear_masked_range(&skb_key, &head->mask);
--
2.7.4
