On Mon 14 May 2018 at 16:23, Jiri Pirko [off-list ref] wrote:

Mon, May 14, 2018 at 04:27:06PM CEST, vladbu@mellanox.com wrote:

quoted

Without rtnl lock protection it is no longer safe to use pointer to tc
action without holding reference to it. (it can be destroyed concurrently)

Remove unsafe action idr lookup function. Instead of it, implement safe tcf
idr check function that atomically looks up action in idr and increments
its reference and bind counters.

Implement both action search and check using new safe function.

Signed-off-by: Vlad Buslov <redacted>
---
net/sched/act_api.c | 38 ++++++++++++++++----------------------
1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 1331beb..9459cce 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c

@@ -284,44 +284,38 @@ int tcf_generic_walker(struct tc_action_net *tn, struct sk_buff *skb,

}
EXPORT_SYMBOL(tcf_generic_walker);

-static struct tc_action *tcf_idr_lookup(u32 index, struct tcf_idrinfo *idrinfo)
+bool __tcf_idr_check(struct tc_action_net *tn, u32 index, struct tc_action **a,
+		     int bind)
{
-	struct tc_action *p = NULL;
+	struct tcf_idrinfo *idrinfo = tn->idrinfo;
+	struct tc_action *p;

	spin_lock_bh(&idrinfo->lock);

Why "_bh" variant is necessary here?

It is not my code.

quoted

	p = idr_find(&idrinfo->action_idr, index);
+	if (p) {
+		refcount_inc(&p->tcfa_refcnt);
+		if (bind)
+			atomic_inc(&p->tcfa_bindcnt);
+	}
	spin_unlock_bh(&idrinfo->lock);

[...]