netdev - [PATCH RFC 2/4] net: sched: fix err handler in tcf_action

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210331164012.28653-3-vladbu@nvidia.com>
Date:   Wed, 31 Mar 2021 19:40:10 +0300
From:   Vlad Buslov <vladbu@...dia.com>
To:     <netdev@...r.kernel.org>
CC:     <memxor@...il.com>, <xiyou.wangcong@...il.com>,
        <davem@...emloft.net>, <jhs@...atatu.com>, <jiri@...nulli.us>,
        <kuba@...nel.org>, <toke@...hat.com>,
        Vlad Buslov <vladbu@...dia.com>
Subject: [PATCH RFC 2/4] net: sched: fix err handler in tcf_action_init()

With recent changes that separated action module load from action
initialization tcf_action_init() function error handling code was modified
to manually release the loaded modules if loading/initialization of any
further action in same batch failed. For the case when all modules
successfully loaded and some of the actions were initialized before one of
them failed in init handler. In this case for all previous actions the
module will be released twice by the error handler: First time by the loop
that manually calls module_put() for all ops, and second time by the action
destroy code that puts the module after destroying the action.

Reproduction:

$ sudo tc actions add action simple sdata \"2\" index 2
$ sudo tc actions add action simple sdata \"1\" index 1 action simple sdata \"2\" index 2
RTNETLINK answers: File exists
We have an error talking to the kernel
$ sudo tc actions ls action simple
total acts 1

        action order 0: Simple <"2">
         index 2 ref 1 bind 0
$ sudo tc actions flush action simple
$ sudo tc actions ls action simple
$ sudo tc actions add action simple sdata \"2\" index 2
Error: Failed to load TC action module.
We have an error talking to the kernel
$ lsmod | grep simple
act_simple             20480  -1

Fix the issue by refactoring tcf_action_init() error handling code to
properly account for the case of partially initialized action list and only
put the module for actions that haven't been initialized.

Fixes: d349f9976868 ("net_sched: fix RTNL deadlock again caused by request_module()")
Signed-off-by: Vlad Buslov <vladbu@...dia.com>
---
 net/sched/act_api.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index eb20a75796d5..4ef556906e32 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -753,20 +753,28 @@ int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
 }
 EXPORT_SYMBOL(tcf_action_exec);
 
-int tcf_action_destroy(struct tc_action *actions[], int bind)
+static int tcf_action_destroy_1(struct tc_action *a, int bind)
 {
 	const struct tc_action_ops *ops;
+	int ret;
+
+	ops = a->ops;
+	ret = __tcf_idr_release(a, bind, true);
+	if (ret == ACT_P_DELETED)
+		module_put(ops->owner);
+	return ret;
+}
+
+int tcf_action_destroy(struct tc_action *actions[], int bind)
+{
 	struct tc_action *a;
 	int ret = 0, i;
 
 	for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) {
 		a = actions[i];
 		actions[i] = NULL;
-		ops = a->ops;
-		ret = __tcf_idr_release(a, bind, true);
-		if (ret == ACT_P_DELETED)
-			module_put(ops->owner);
-		else if (ret < 0)
+		ret = tcf_action_destroy_1(a, bind);
+		if (ret < 0)
 			return ret;
 	}
 	return ret;
@@ -1082,7 +1090,7 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
 		a_o = tc_action_load_ops(name, tb[i], rtnl_held, extack);
 		if (IS_ERR(a_o)) {
 			err = PTR_ERR(a_o);
-			goto err_mod;
+			goto err;
 		}
 		ops[i - 1] = a_o;
 	}
@@ -1109,11 +1117,13 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
 	return i - 1;
 
 err:
-	tcf_action_destroy(actions, bind);
-err_mod:
 	for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
-		if (ops[i])
+		if (actions[i]) {
+			tcf_action_destroy_1(actions[i], bind);
+			actions[i] = NULL;
+		} else if (ops[i]) {
 			module_put(ops[i]->owner);
+		}
 	}
 	return err;
 }
-- 
2.29.2