Hi Kumar, Thanks for the patch!
On Sun 28 Mar 2021 at 09:45, Kumar Kartikeya Dwivedi <mem...@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 <mem...@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])) {