Messages in this thread | | | From | Vlad Buslov <> | Subject | Re: [PATCH 1/1] net: sched: extend lifetime of new action in replace mode | Date | Mon, 29 Mar 2021 12:05:12 +0300 |
| |
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])) {
| |