| Message ID | 20210328064555.93365-1-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/1] net: sched: extend lifetime of new action in replace mode | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | fail | 1 blamed authors not CCed: marcelo.leitner@gmail.com; 1 maintainers not CCed: marcelo.leitner@gmail.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 134 this patch: 134 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 33 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 223 this patch: 223 |
| netdev/header_inline | success | Link |
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])) {
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
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])) {
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(-)