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@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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])) {