[PATCH RFC 0/4] Action initalization fixes

[PATCH RFC 0/4] Action initalization fixes

[Vlad Buslov]

This series contains fixes and relevant tests for two issues that are being
discussed in mailing list thread "net: sched: bump refcount for new action in ACT
replace mode".

Sending this as RFC to gather feedbeck. Non-RFC submission will probably split
in two: fixes to net, tests to net-next.

Vlad Buslov (4):
  net: sched: fix action overwrite reference counting
  net: sched: fix err handler in tcf_action_init()
  tc-testing: add simple action test to verify batch add cleanup
  tc-testing: add simple action test to verify batch change cleanup

 include/net/act_api.h                         |  5 +-
 net/sched/act_api.c                           | 57 +++++++++++-------
 net/sched/cls_api.c                           |  9 +--
 .../tc-testing/tc-tests/actions/simple.json   | 59 +++++++++++++++++++
 4 files changed, 104 insertions(+), 26 deletions(-)

-- 
2.29.2

[PATCH RFC 1/4] net: sched: fix action overwrite reference counting

[Vlad Buslov]

Action init code increments reference counter when it changes an action.
This is the desired behavior for cls API which needs to obtain action
reference for every classifier that points to action. However, act API just
needs to change the action and releases the reference before returning.
This sequence breaks when the requested action doesn't exist, which causes
act API init code to create new action with specified index, but action is
still released before returning and is deleted (unless it was referenced
concurrently by cls API).

Fixes: cae422f379f3 ("net: sched: use reference counting action init")
Reported-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Vlad Buslov <vladbu@nvidia.com>
---
 include/net/act_api.h |  5 +++--
 net/sched/act_api.c   | 27 +++++++++++++++++----------
 net/sched/cls_api.c   |  9 +++++----
 3 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 2bf3092ae7ec..312f0f6554a0 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -185,7 +185,7 @@ 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,
 		    struct nlattr *est, char *name, int ovr, int bind,
-		    struct tc_action *actions[], size_t *attr_size,
+		    struct tc_action *actions[], int init_res[], size_t *attr_size,
 		    bool rtnl_held, struct netlink_ext_ack *extack);
 struct tc_action_ops *tc_action_load_ops(char *name, struct nlattr *nla,
 					 bool rtnl_held,
@@ -193,7 +193,8 @@ struct tc_action_ops *tc_action_load_ops(char *name, struct nlattr *nla,
 struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 				    struct nlattr *nla, struct nlattr *est,
 				    char *name, int ovr, int bind,
-				    struct tc_action_ops *ops, bool rtnl_held,
+				    struct tc_action_ops *a_o, int *init_res,
+				    bool rtnl_held,
 				    struct netlink_ext_ack *extack);
 int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[], int bind,
 		    int ref, bool terse);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index b919826939e0..eb20a75796d5 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -777,8 +777,11 @@ static int tcf_action_put(struct tc_action *p)
 	return __tcf_action_put(p, false);
 }
 
-/* Put all actions in this array, skip those NULL's. */
-static void tcf_action_put_many(struct tc_action *actions[])
+/* Put all actions in this array, skip those NULL's. If cond array is provided
+ * by caller, then only put actions that match.
+ */
+static void tcf_action_put_many(struct tc_action *actions[], int *cond,
+				int match)
 {
 	int i;
 
@@ -786,7 +789,7 @@ static void tcf_action_put_many(struct tc_action *actions[])
 		struct tc_action *a = actions[i];
 		const struct tc_action_ops *ops;
 
-		if (!a)
+		if (!a || (cond && cond[i] != match))
 			continue;
 		ops = a->ops;
 		if (tcf_action_put(a))
@@ -992,7 +995,8 @@ struct tc_action_ops *tc_action_load_ops(char *name, struct nlattr *nla,
 struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 				    struct nlattr *nla, struct nlattr *est,
 				    char *name, int ovr, int bind,
-				    struct tc_action_ops *a_o, bool rtnl_held,
+				    struct tc_action_ops *a_o, int *init_res,
+				    bool rtnl_held,
 				    struct netlink_ext_ack *extack)
 {
 	struct nla_bitfield32 flags = { 0, 0 };
@@ -1028,6 +1032,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	}
 	if (err < 0)
 		goto err_out;
+	*init_res = err;
 
 	if (!name && tb[TCA_ACT_COOKIE])
 		tcf_set_action_cookie(&a->act_cookie, cookie);
@@ -1056,7 +1061,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 
 int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
 		    struct nlattr *est, char *name, int ovr, int bind,
-		    struct tc_action *actions[], size_t *attr_size,
+		    struct tc_action *actions[], int init_res[], size_t *attr_size,
 		    bool rtnl_held, struct netlink_ext_ack *extack)
 {
 	struct tc_action_ops *ops[TCA_ACT_MAX_PRIO] = {};
@@ -1084,7 +1089,8 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
 
 	for (i = 1; i <= TCA_ACT_MAX_PRIO && tb[i]; i++) {
 		act = tcf_action_init_1(net, tp, tb[i], est, name, ovr, bind,
-					ops[i - 1], rtnl_held, extack);
+					ops[i - 1], &init_res[i - 1], rtnl_held,
+					extack);
 		if (IS_ERR(act)) {
 			err = PTR_ERR(act);
 			goto err;
@@ -1462,7 +1468,7 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 		return 0;
 	}
 err:
-	tcf_action_put_many(actions);
+	tcf_action_put_many(actions, NULL, 0);
 	return ret;
 }
 
@@ -1499,10 +1505,11 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
 	size_t attr_size = 0;
 	int loop, ret;
 	struct tc_action *actions[TCA_ACT_MAX_PRIO] = {};
+	int init_res[TCA_ACT_MAX_PRIO] = {};
 
 	for (loop = 0; loop < 10; loop++) {
 		ret = tcf_action_init(net, NULL, nla, NULL, NULL, ovr, 0,
-				      actions, &attr_size, true, extack);
+				      actions, init_res, &attr_size, true, extack);
 		if (ret != -EAGAIN)
 			break;
 	}
@@ -1510,8 +1517,8 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
 	if (ret < 0)
 		return ret;
 	ret = tcf_add_notify(net, n, actions, portid, attr_size, extack);
-	if (ovr)
-		tcf_action_put_many(actions);
+	/* Only put existing actions that were changed by init (res==0). */
+	tcf_action_put_many(actions, init_res, 0);
 
 	return ret;
 }
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index d3db70865d66..f7425bb9fc3d 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3040,6 +3040,7 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
 {
 #ifdef CONFIG_NET_CLS_ACT
 	{
+		int init_res[TCA_ACT_MAX_PRIO] = {};
 		struct tc_action *act;
 		size_t attr_size = 0;
 
@@ -3051,8 +3052,8 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
 				return PTR_ERR(a_o);
 			act = tcf_action_init_1(net, tp, tb[exts->police],
 						rate_tlv, "police", ovr,
-						TCA_ACT_BIND, a_o, rtnl_held,
-						extack);
+						TCA_ACT_BIND, a_o, init_res,
+						rtnl_held, extack);
 			if (IS_ERR(act)) {
 				module_put(a_o->owner);
 				return PTR_ERR(act);
@@ -3067,8 +3068,8 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
 
 			err = tcf_action_init(net, tp, tb[exts->action],
 					      rate_tlv, NULL, ovr, TCA_ACT_BIND,
-					      exts->actions, &attr_size,
-					      rtnl_held, extack);
+					      exts->actions, init_res,
+					      &attr_size, rtnl_held, extack);
 			if (err < 0)
 				return err;
 			exts->nr_actions = err;
-- 
2.29.2

Re: [PATCH RFC 1/4] net: sched: fix action overwrite reference counting

[Cong Wang]

On Wed, Mar 31, 2021 at 9:41 AM Vlad Buslov <vladbu@nvidia.com> wrote:
>
> Action init code increments reference counter when it changes an action.
> This is the desired behavior for cls API which needs to obtain action
> reference for every classifier that points to action. However, act API just
> needs to change the action and releases the reference before returning.
> This sequence breaks when the requested action doesn't exist, which causes
> act API init code to create new action with specified index, but action is
> still released before returning and is deleted (unless it was referenced
> concurrently by cls API).

Please also add a summary of how you fix it. From what I understand,
it seems you just skip the refcnt put of successful cases?

One comment below.

>
> Fixes: cae422f379f3 ("net: sched: use reference counting action init")
> Reported-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> Signed-off-by: Vlad Buslov <vladbu@nvidia.com>
> ---
>  include/net/act_api.h |  5 +++--
>  net/sched/act_api.c   | 27 +++++++++++++++++----------
>  net/sched/cls_api.c   |  9 +++++----
>  3 files changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 2bf3092ae7ec..312f0f6554a0 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -185,7 +185,7 @@ 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,
>                     struct nlattr *est, char *name, int ovr, int bind,
> -                   struct tc_action *actions[], size_t *attr_size,
> +                   struct tc_action *actions[], int init_res[], size_t *attr_size,
>                     bool rtnl_held, struct netlink_ext_ack *extack);
>  struct tc_action_ops *tc_action_load_ops(char *name, struct nlattr *nla,
>                                          bool rtnl_held,
> @@ -193,7 +193,8 @@ struct tc_action_ops *tc_action_load_ops(char *name, struct nlattr *nla,
>  struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>                                     struct nlattr *nla, struct nlattr *est,
>                                     char *name, int ovr, int bind,
> -                                   struct tc_action_ops *ops, bool rtnl_held,
> +                                   struct tc_action_ops *a_o, int *init_res,
> +                                   bool rtnl_held,
>                                     struct netlink_ext_ack *extack);
>  int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[], int bind,
>                     int ref, bool terse);
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index b919826939e0..eb20a75796d5 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -777,8 +777,11 @@ static int tcf_action_put(struct tc_action *p)
>         return __tcf_action_put(p, false);
>  }
>
> -/* Put all actions in this array, skip those NULL's. */
> -static void tcf_action_put_many(struct tc_action *actions[])
> +/* Put all actions in this array, skip those NULL's. If cond array is provided
> + * by caller, then only put actions that match.
> + */
> +static void tcf_action_put_many(struct tc_action *actions[], int *cond,
> +                               int match)
>  {
>         int i;
>
> @@ -786,7 +789,7 @@ static void tcf_action_put_many(struct tc_action *actions[])
>                 struct tc_action *a = actions[i];
>                 const struct tc_action_ops *ops;
>
> -               if (!a)
> +               if (!a || (cond && cond[i] != match))

This looks a bit odd. How about passing an array of action pointers which
only contains those that need to be put?

Thanks.

Re: [PATCH RFC 1/4] net: sched: fix action overwrite reference counting

[Vlad Buslov]

On Sat 03 Apr 2021 at 01:13, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Wed, Mar 31, 2021 at 9:41 AM Vlad Buslov <vladbu@nvidia.com> wrote:
>>
>> Action init code increments reference counter when it changes an action.
>> This is the desired behavior for cls API which needs to obtain action
>> reference for every classifier that points to action. However, act API just
>> needs to change the action and releases the reference before returning.
>> This sequence breaks when the requested action doesn't exist, which causes
>> act API init code to create new action with specified index, but action is
>> still released before returning and is deleted (unless it was referenced
>> concurrently by cls API).
>
> Please also add a summary of how you fix it. From what I understand,
> it seems you just skip the refcnt put of successful cases?

Oops, I didn't regenerate patches after amending the commit message.
This should include the following paragraph:

Extend tcf_action_init() to accept 'init_res' array and initialize it with
action->ops->init() result. Refactor tcf_action_put_many() to also accept
such array and only put actions for which init result match provided value.
Modify tcf_action_add() to only put actions with init_res==0 instead of
unconditionally putting all actions when user set NLM_F_REPLACE netlink
message flag.

>
> One comment below.
>
>>
>> Fixes: cae422f379f3 ("net: sched: use reference counting action init")
>> Reported-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>> Signed-off-by: Vlad Buslov <vladbu@nvidia.com>
>> ---
>>  include/net/act_api.h |  5 +++--
>>  net/sched/act_api.c   | 27 +++++++++++++++++----------
>>  net/sched/cls_api.c   |  9 +++++----
>>  3 files changed, 25 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/net/act_api.h b/include/net/act_api.h
>> index 2bf3092ae7ec..312f0f6554a0 100644
>> --- a/include/net/act_api.h
>> +++ b/include/net/act_api.h
>> @@ -185,7 +185,7 @@ 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,
>>                     struct nlattr *est, char *name, int ovr, int bind,
>> -                   struct tc_action *actions[], size_t *attr_size,
>> +                   struct tc_action *actions[], int init_res[], size_t *attr_size,
>>                     bool rtnl_held, struct netlink_ext_ack *extack);
>>  struct tc_action_ops *tc_action_load_ops(char *name, struct nlattr *nla,
>>                                          bool rtnl_held,
>> @@ -193,7 +193,8 @@ struct tc_action_ops *tc_action_load_ops(char *name, struct nlattr *nla,
>>  struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>>                                     struct nlattr *nla, struct nlattr *est,
>>                                     char *name, int ovr, int bind,
>> -                                   struct tc_action_ops *ops, bool rtnl_held,
>> +                                   struct tc_action_ops *a_o, int *init_res,
>> +                                   bool rtnl_held,
>>                                     struct netlink_ext_ack *extack);
>>  int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[], int bind,
>>                     int ref, bool terse);
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index b919826939e0..eb20a75796d5 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -777,8 +777,11 @@ static int tcf_action_put(struct tc_action *p)
>>         return __tcf_action_put(p, false);
>>  }
>>
>> -/* Put all actions in this array, skip those NULL's. */
>> -static void tcf_action_put_many(struct tc_action *actions[])
>> +/* Put all actions in this array, skip those NULL's. If cond array is provided
>> + * by caller, then only put actions that match.
>> + */
>> +static void tcf_action_put_many(struct tc_action *actions[], int *cond,
>> +                               int match)
>>  {
>>         int i;
>>
>> @@ -786,7 +789,7 @@ static void tcf_action_put_many(struct tc_action *actions[])
>>                 struct tc_action *a = actions[i];
>>                 const struct tc_action_ops *ops;
>>
>> -               if (!a)
>> +               if (!a || (cond && cond[i] != match))
>
> This looks a bit odd. How about passing an array of action pointers which
> only contains those that need to be put?

I wanted to make it extensible with cond array instead of make every
user manually filter the action array before calling
tcf_action_put_many(). But I guess there is currently no need for that
and just extending tcf_action_add() with a loop to zero-out the pointers
for newly created actions will be clearer. Will change it in V2.

>
> Thanks.

[PATCH RFC 2/4] net: sched: fix err handler in tcf_action_init()

[Vlad Buslov]

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@nvidia.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

Re: [PATCH RFC 2/4] net: sched: fix err handler in tcf_action_init()

[Cong Wang]

On Wed, Mar 31, 2021 at 9:41 AM Vlad Buslov <vladbu@nvidia.com> wrote:
>
> 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.

This is really strange. Isn't tc_action_load_ops() paired with module_put()
under 'err_mod'? And the one in tcf_action_destroy() paired with
tcf_action_init_1()? Is it the one below which causes the imbalance?

1038         /* module count goes up only when brand new policy is created
1039          * if it exists and is only bound to in a_o->init() then
1040          * ACT_P_CREATED is not returned (a zero is).
1041          */
1042         if (err != ACT_P_CREATED)
1043                 module_put(a_o->owner);
1044

Thanks.

Re: [PATCH RFC 2/4] net: sched: fix err handler in tcf_action_init()

[Vlad Buslov]

On Sat 03 Apr 2021 at 02:14, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Wed, Mar 31, 2021 at 9:41 AM Vlad Buslov <vladbu@nvidia.com> wrote:
>>
>> 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.
>
> This is really strange. Isn't tc_action_load_ops() paired with module_put()
> under 'err_mod'? And the one in tcf_action_destroy() paired with
> tcf_action_init_1()? Is it the one below which causes the imbalance?
>
> 1038         /* module count goes up only when brand new policy is created
> 1039          * if it exists and is only bound to in a_o->init() then
> 1040          * ACT_P_CREATED is not returned (a zero is).
> 1041          */
> 1042         if (err != ACT_P_CREATED)
> 1043                 module_put(a_o->owner);
> 1044

This problem is not related to action change reference counting
imbalance which is addressed in previous commit. The issue is that
function tcf_action_init_1() doesn't take another reference to module.
It expects caller to get the reference before calling init and "takes
over" the reference in case of success (e.g. action instance now owns
the reference which will be released when action instance is destroyed).

So, the following happens in reproduction provided in commit message
when executing "tc actions add action simple sdata \"1\" index 1
action simple sdata \"2\" index 2" command:

1. tcf_action_init() is called with batch of two actions of same type,
no module references are held, 'actions' array is empty:

act_simple refcnt balance = 0
actions[] = {}

2. tc_action_load_ops() is called for first action:

act_simple refcnt balance = +1
actions[] = {}

3. tc_action_load_ops() is called for second action:

act_simple refcnt balance = +2
actions[] = {}

4. tcf_action_init_1() called for first action, succeeds, action
instance is assigned to 'actions' array:

act_simple refcnt balance = +2
actions[] = { [0]=act1 }

5. tcf_action_init_1() fails for second action, 'actions' array not
changed, goto err:

act_simple refcnt balance = +2
actions[] = { [0]=act1 }

6. tcf_action_destroy() is called for 'actions' array, last reference to
first action is released, tcf_action_destroy_1() calls module_put() for
actions module:

act_simple refcnt balance = +1
actions[] = {}

7. err_mod loop starts iterating over ops array, executes module_put()
for first actions ops:

act_simple refcnt balance = 0
actions[] = {}

7. err_mod loop executes module_put() for second actions ops:

act_simple refcnt balance = -1
actions[] = {}


The goal of my fix is to not unconditionally release the module
reference for successfully initialized actions because this is already
handled by action destroy code. Hope this explanation clarifies things.

Regards,
Vlad

Re: [PATCH RFC 2/4] net: sched: fix err handler in tcf_action_init()

[Cong Wang]

On Sat, Apr 3, 2021 at 3:01 AM Vlad Buslov <vladbu@nvidia.com> wrote:
> So, the following happens in reproduction provided in commit message
> when executing "tc actions add action simple sdata \"1\" index 1
> action simple sdata \"2\" index 2" command:
>
> 1. tcf_action_init() is called with batch of two actions of same type,
> no module references are held, 'actions' array is empty:
>
> act_simple refcnt balance = 0
> actions[] = {}
>
> 2. tc_action_load_ops() is called for first action:
>
> act_simple refcnt balance = +1
> actions[] = {}
>
> 3. tc_action_load_ops() is called for second action:
>
> act_simple refcnt balance = +2
> actions[] = {}
>
> 4. tcf_action_init_1() called for first action, succeeds, action
> instance is assigned to 'actions' array:
>
> act_simple refcnt balance = +2
> actions[] = { [0]=act1 }
>
> 5. tcf_action_init_1() fails for second action, 'actions' array not
> changed, goto err:
>
> act_simple refcnt balance = +2
> actions[] = { [0]=act1 }
>
> 6. tcf_action_destroy() is called for 'actions' array, last reference to
> first action is released, tcf_action_destroy_1() calls module_put() for
> actions module:
>
> act_simple refcnt balance = +1
> actions[] = {}
>
> 7. err_mod loop starts iterating over ops array, executes module_put()
> for first actions ops:
>
> act_simple refcnt balance = 0
> actions[] = {}
>
> 7. err_mod loop executes module_put() for second actions ops:
>
> act_simple refcnt balance = -1
> actions[] = {}
>
>
> The goal of my fix is to not unconditionally release the module
> reference for successfully initialized actions because this is already
> handled by action destroy code. Hope this explanation clarifies things.

Great explanation! It seems harder and harder to understand the
module refcnt here. How about we just take the refcnt when we
successfully create an action? Something like this:

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index b919826939e0..075cc80480bf 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -493,6 +493,7 @@ int tcf_idr_create(struct tc_action_net *tn, u32
index, struct nlattr *est,
        }

        p->idrinfo = idrinfo;
+       __module_get(ops->owner);
        p->ops = ops;
        *a = p;
        return 0;
@@ -1035,13 +1036,6 @@ struct tc_action *tcf_action_init_1(struct net
*net, struct tcf_proto *tp,
        if (!name)
                a->hw_stats = hw_stats;

-       /* module count goes up only when brand new policy is created
-        * if it exists and is only bound to in a_o->init() then
-        * ACT_P_CREATED is not returned (a zero is).
-        */
-       if (err != ACT_P_CREATED)
-               module_put(a_o->owner);
-
        return a;

 err_out:
@@ -1100,7 +1094,8 @@ int tcf_action_init(struct net *net, struct
tcf_proto *tp, struct nlattr *nla,
        tcf_idr_insert_many(actions);

        *attr_size = tcf_action_full_attrs_size(sz);
-       return i - 1;
+       err = i - 1;
+       goto err_mod:

 err:
        tcf_action_destroy(actions, bind);

The idea is on the higher level we hold refcnt when loading module and
put it back _unconditionally_ when returning, and hold a refcnt only when
we create an action and conditionally put it back when an error happens.
With pseudo code, it is something like this:

load_ops() // module refcnt +1
init_actions(); // module refcnt +1 only when create a new one
if (err)
  // module refcnt -1 when we delete one
  module_put();
module_put(); // module refcnt -1

This looks much easier to track. What do you think?

Thanks!

Re: [PATCH RFC 2/4] net: sched: fix err handler in tcf_action_init()

[Vlad Buslov]

On Tue 06 Apr 2021 at 01:56, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Sat, Apr 3, 2021 at 3:01 AM Vlad Buslov <vladbu@nvidia.com> wrote:
>> So, the following happens in reproduction provided in commit message
>> when executing "tc actions add action simple sdata \"1\" index 1
>> action simple sdata \"2\" index 2" command:
>>
>> 1. tcf_action_init() is called with batch of two actions of same type,
>> no module references are held, 'actions' array is empty:
>>
>> act_simple refcnt balance = 0
>> actions[] = {}
>>
>> 2. tc_action_load_ops() is called for first action:
>>
>> act_simple refcnt balance = +1
>> actions[] = {}
>>
>> 3. tc_action_load_ops() is called for second action:
>>
>> act_simple refcnt balance = +2
>> actions[] = {}
>>
>> 4. tcf_action_init_1() called for first action, succeeds, action
>> instance is assigned to 'actions' array:
>>
>> act_simple refcnt balance = +2
>> actions[] = { [0]=act1 }
>>
>> 5. tcf_action_init_1() fails for second action, 'actions' array not
>> changed, goto err:
>>
>> act_simple refcnt balance = +2
>> actions[] = { [0]=act1 }
>>
>> 6. tcf_action_destroy() is called for 'actions' array, last reference to
>> first action is released, tcf_action_destroy_1() calls module_put() for
>> actions module:
>>
>> act_simple refcnt balance = +1
>> actions[] = {}
>>
>> 7. err_mod loop starts iterating over ops array, executes module_put()
>> for first actions ops:
>>
>> act_simple refcnt balance = 0
>> actions[] = {}
>>
>> 7. err_mod loop executes module_put() for second actions ops:
>>
>> act_simple refcnt balance = -1
>> actions[] = {}
>>
>>
>> The goal of my fix is to not unconditionally release the module
>> reference for successfully initialized actions because this is already
>> handled by action destroy code. Hope this explanation clarifies things.
>
> Great explanation! It seems harder and harder to understand the
> module refcnt here. How about we just take the refcnt when we
> successfully create an action? Something like this:
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index b919826939e0..075cc80480bf 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -493,6 +493,7 @@ int tcf_idr_create(struct tc_action_net *tn, u32
> index, struct nlattr *est,
>         }
>
>         p->idrinfo = idrinfo;
> +       __module_get(ops->owner);
>         p->ops = ops;
>         *a = p;
>         return 0;
> @@ -1035,13 +1036,6 @@ struct tc_action *tcf_action_init_1(struct net
> *net, struct tcf_proto *tp,
>         if (!name)
>                 a->hw_stats = hw_stats;
>
> -       /* module count goes up only when brand new policy is created
> -        * if it exists and is only bound to in a_o->init() then
> -        * ACT_P_CREATED is not returned (a zero is).
> -        */
> -       if (err != ACT_P_CREATED)
> -               module_put(a_o->owner);
> -
>         return a;
>
>  err_out:
> @@ -1100,7 +1094,8 @@ int tcf_action_init(struct net *net, struct
> tcf_proto *tp, struct nlattr *nla,
>         tcf_idr_insert_many(actions);
>
>         *attr_size = tcf_action_full_attrs_size(sz);
> -       return i - 1;
> +       err = i - 1;
> +       goto err_mod:
>
>  err:
>         tcf_action_destroy(actions, bind);
>
> The idea is on the higher level we hold refcnt when loading module and
> put it back _unconditionally_ when returning, and hold a refcnt only when
> we create an action and conditionally put it back when an error happens.
> With pseudo code, it is something like this:
>
> load_ops() // module refcnt +1
> init_actions(); // module refcnt +1 only when create a new one
> if (err)
>   // module refcnt -1 when we delete one
>   module_put();
> module_put(); // module refcnt -1
>
> This looks much easier to track. What do you think?
>
> Thanks!

Indeed, your suggestion looks more straightforward. The only thing we
need to mind is that action->init() callbacks assume that caller
releases the module even after calling tcf_idr_create(), so we also need
to modify tcf_idr_release() (used by error handlers in action->init()
implementations) to release the module.

I'll run some tests tomorrow to verify that I'm not missing anything
else.

Regards,
Vlad

[PATCH RFC 3/4] tc-testing: add simple action test to verify batch add cleanup

[Vlad Buslov]

Verify cleanup of failed actions batch add where second action in batch
fails after successful init of first action.

Signed-off-by: Vlad Buslov <vladbu@nvidia.com>
---
 .../tc-testing/tc-tests/actions/simple.json   | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/simple.json b/tools/testing/selftests/tc-testing/tc-tests/actions/simple.json
index e15f708b0fa4..d5bcbb919dcc 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/simple.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/simple.json
@@ -175,5 +175,35 @@
         "teardown": [
             "$TC actions flush action simple"
         ]
+    },
+    {
+        "id": "8d07",
+        "name": "Verify cleanup of failed actions batch add",
+        "category": [
+            "actions",
+            "simple"
+        ],
+        "setup": [
+            [
+                "$TC actions flush action simple",
+                0,
+                1,
+                255
+            ],
+            "$TC actions add action simple sdata \"2\" index 2",
+            [
+                "$TC actions add action simple sdata \"1\" index 1 action simple sdata \"2\" index 2",
+                255
+            ],
+            "$TC actions flush action simple"
+        ],
+        "cmdUnderTest": "$TC actions add action simple sdata \"2\" index 2",
+        "expExitCode": "0",
+        "verifyCmd": "$TC actions list action simple",
+        "matchPattern": "action order [0-9]*: Simple <2>.*index 2 ref",
+        "matchCount": "1",
+        "teardown": [
+            "$TC actions flush action simple"
+        ]
     }
 ]
-- 
2.29.2

[PATCH RFC 4/4] tc-testing: add simple action test to verify batch change cleanup

[Vlad Buslov]

Verify cleanup of failed actions batch change where second action in batch
fails after successful init of first action.

Signed-off-by: Vlad Buslov <vladbu@nvidia.com>
---
 .../tc-testing/tc-tests/actions/simple.json   | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/simple.json b/tools/testing/selftests/tc-testing/tc-tests/actions/simple.json
index d5bcbb919dcc..e0c5f060ccb9 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/simple.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/simple.json
@@ -205,5 +205,34 @@
         "teardown": [
             "$TC actions flush action simple"
         ]
+    },
+    {
+        "id": "a68a",
+        "name": "Verify cleanup of failed actions batch change",
+        "category": [
+            "actions",
+            "simple"
+        ],
+        "setup": [
+            [
+                "$TC actions flush action simple",
+                0,
+                1,
+                255
+            ],
+            [
+                "$TC actions change action simple sdata \"1\" index 1 action simple sdata \"2\" goto chain 42 index 2",
+                255
+            ],
+            "$TC actions flush action simple"
+        ],
+        "cmdUnderTest": "$TC actions add action simple sdata \"1\" index 1",
+        "expExitCode": "0",
+        "verifyCmd": "$TC actions list action simple",
+        "matchPattern": "action order [0-9]*: Simple <1>.*index 1 ref",
+        "matchCount": "1",
+        "teardown": [
+            "$TC actions flush action simple"
+        ]
     }
 ]
-- 
2.29.2