Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Kumar Kartikeya Dwivedi <memxor@gmail.com>
To:     netdev@vger.kernel.org
Cc:     Vlad Buslov <vladbu@mellanox.com>,
        =?UTF-8?q?Toke=20H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>,
        Kumar Kartikeya Dwivedi <memxor@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Jamal Hadi Salim <jhs@mojatatu.com>,
        Cong Wang <xiyou.wangcong@gmail.com>,
        Jiri Pirko <jiri@resnulli.us>, linux-kernel@vger.kernel.org
Subject: [PATCH 1/1] net: sched: extend lifetime of new action in replace mode
Date:   Sun, 28 Mar 2021 12:15:55 +0530
Message-Id: <20210328064555.93365-1-memxor@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

When creating an action in replace mode, in tcf_action_add, the refcount
of existing actions is rightly raised during tcf_idr_check_alloc call,
but for new actions a dummy placeholder entry is created. This is then
replaced with the actual action during tcf_idr_insert_many, but between
this and the tcf_action_put_many call made in replace mode, we don't
hold a reference to the newly created action while it has been
published. This means that it can disappear under our feet, and that
newly created actions are destroyed right after their creation as their
refcount drops to 0 in replace mode.

This leads to commands like tc action replace reporting success in
creation of the action and just removing them right after adding the
action, leading to confusion in userspace.

Fix this by raising the refcount of a newly created action and then
pairing that increment with a tcf_action_put call to release the
reference held during insertion. This is only needed in replace mode.
We use a relaxed store as insert will ensure its visibility.

Fixes: cae422f379f3 ("net: sched: use reference counting action init")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/net/act_api.h | 1 +
 net/sched/act_api.c   | 5 ++++-
 net/sched/cls_api.c   | 3 +++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 2bf3092ae7ec..8b375b46cc04 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -181,6 +181,7 @@ int tcf_register_action(struct tc_action_ops *a, struct pernet_operations *ops);
 int tcf_unregister_action(struct tc_action_ops *a,
 			  struct pernet_operations *ops);
 int tcf_action_destroy(struct tc_action *actions[], int bind);
+void tcf_action_put_many(struct tc_action *actions[]);
 int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
 		    int nr_actions, struct tcf_result *res);
 int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index b919826939e0..7e26c18cdb15 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -778,7 +778,7 @@ static int tcf_action_put(struct tc_action *p)
 }
 
 /* Put all actions in this array, skip those NULL's. */
-static void tcf_action_put_many(struct tc_action *actions[])
+void tcf_action_put_many(struct tc_action *actions[])
 {
 	int i;
 
@@ -1042,6 +1042,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	if (err != ACT_P_CREATED)
 		module_put(a_o->owner);
 
+	if (err == ACT_P_CREATED && ovr)
+		refcount_set(&a->tcfa_refcnt, 2);
+
 	return a;
 
 err_out:
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index d3db70865d66..666077c23147 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3074,6 +3074,9 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
 			exts->nr_actions = err;
 		}
 	}
+
+	if (ovr)
+		tcf_action_put_many(exts->actions);
 #else
 	if ((exts->action && tb[exts->action]) ||
 	    (exts->police && tb[exts->police])) {
-- 
2.30.2