[PATCH 1/1] net: sched: extend lifetime of new action in replace mode

[PATCH 1/1] net: sched: extend lifetime of new action in replace mode

[Kumar Kartikeya Dwivedi]

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

Re: [PATCH 1/1] net: sched: extend lifetime of new action in replace mode

[Vlad Buslov]

Hi Kumar,

Thanks for the patch!

On Sun 28 Mar 2021 at 09:45, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> 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.

Could you describe the reproduction in more details? Looking at the code
it seems that there are two ways actions are overwritten/deleted:

1. Directly through action API, which is still serialized by rtnl lock.

2. Classifier API, which doesn't use rtnl lock anymore and can execute
concurrently.

Actions created by path 2 also have their bind count incremented which
prevents them from being deleted by path 1 and cls API can only deleted
them together with classifier that points to them.

>
> 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)

So here you only take additional reference on newly created action...

> +		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)

... but here you always release the reference if ovr is set, even when
overwriting existing action.

> +		tcf_action_put_many(exts->actions);

So, what happens here is actions were 'deleted' concurrently (their
tcfa_refcnt decremented by 1)? tcf_action_put_many() will decrement
refcnt again, it will reach 0, actions get actually deleted and
tcf_exts_validate() returns with non-error code, but exts->actions
pointing to freed memory? Doesn't look like the patches fixes the
described issue, unless I'm missing something.

>  #else
>  	if ((exts->action && tb[exts->action]) ||
>  	    (exts->police && tb[exts->police])) {

Re: [PATCH 1/1] net: sched: extend lifetime of new action in replace mode

[Kumar Kartikeya Dwivedi]

On Mon, Mar 29, 2021 at 02:35:12PM IST, Vlad Buslov wrote:
> it seems that there are two ways actions are overwritten/deleted:
>
> 1. Directly through action API, which is still serialized by rtnl lock.
>
> 2. Classifier API, which doesn't use rtnl lock anymore and can execute
> concurrently.
>
> Actions created by path 2 also have their bind count incremented which
> prevents them from being deleted by path 1 and cls API can only deleted
> them together with classifier that points to them.
>
> [...]
> So, what happens here is actions were 'deleted' concurrently (their
> tcfa_refcnt decremented by 1)? tcf_action_put_many() will decrement
> refcnt again, it will reach 0, actions get actually deleted and
> tcf_exts_validate() returns with non-error code, but exts->actions
> pointing to freed memory? Doesn't look like the patches fixes the
> described issue, unless I'm missing something.
>

Thanks for the review and comments.

You are absolutely right. This patch was totally broken. Your feedback however
was quite helpful in understanding the code. I sent a v2, please lmk if it's
correct (also with a hopefully thorough description of the problem & solution).

--
Kartikeya