| Message ID | 20210331164012.28653-3-vladbu@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Action initalization fixes | 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 | success | Link |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| 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: 1 this patch: 1 |
| 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, 58 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
On Wed, Mar 31, 2021 at 9:41 AM Vlad Buslov <vladbu@nvidia.com> wrote: > > With recent changes that separated action module load from action > initialization tcf_action_init() function error handling code was modified > to manually release the loaded modules if loading/initialization of any > further action in same batch failed. For the case when all modules > successfully loaded and some of the actions were initialized before one of > them failed in init handler. In this case for all previous actions the > module will be released twice by the error handler: First time by the loop > that manually calls module_put() for all ops, and second time by the action > destroy code that puts the module after destroying the action. This is really strange. Isn't tc_action_load_ops() paired with module_put() under 'err_mod'? And the one in tcf_action_destroy() paired with tcf_action_init_1()? Is it the one below which causes the imbalance? 1038 /* module count goes up only when brand new policy is created 1039 * if it exists and is only bound to in a_o->init() then 1040 * ACT_P_CREATED is not returned (a zero is). 1041 */ 1042 if (err != ACT_P_CREATED) 1043 module_put(a_o->owner); 1044 Thanks.
On Sat 03 Apr 2021 at 02:14, Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Wed, Mar 31, 2021 at 9:41 AM Vlad Buslov <vladbu@nvidia.com> wrote: >> >> With recent changes that separated action module load from action >> initialization tcf_action_init() function error handling code was modified >> to manually release the loaded modules if loading/initialization of any >> further action in same batch failed. For the case when all modules >> successfully loaded and some of the actions were initialized before one of >> them failed in init handler. In this case for all previous actions the >> module will be released twice by the error handler: First time by the loop >> that manually calls module_put() for all ops, and second time by the action >> destroy code that puts the module after destroying the action. > > This is really strange. Isn't tc_action_load_ops() paired with module_put() > under 'err_mod'? And the one in tcf_action_destroy() paired with > tcf_action_init_1()? Is it the one below which causes the imbalance? > > 1038 /* module count goes up only when brand new policy is created > 1039 * if it exists and is only bound to in a_o->init() then > 1040 * ACT_P_CREATED is not returned (a zero is). > 1041 */ > 1042 if (err != ACT_P_CREATED) > 1043 module_put(a_o->owner); > 1044 This problem is not related to action change reference counting imbalance which is addressed in previous commit. The issue is that function tcf_action_init_1() doesn't take another reference to module. It expects caller to get the reference before calling init and "takes over" the reference in case of success (e.g. action instance now owns the reference which will be released when action instance is destroyed). So, the following happens in reproduction provided in commit message when executing "tc actions add action simple sdata \"1\" index 1 action simple sdata \"2\" index 2" command: 1. tcf_action_init() is called with batch of two actions of same type, no module references are held, 'actions' array is empty: act_simple refcnt balance = 0 actions[] = {} 2. tc_action_load_ops() is called for first action: act_simple refcnt balance = +1 actions[] = {} 3. tc_action_load_ops() is called for second action: act_simple refcnt balance = +2 actions[] = {} 4. tcf_action_init_1() called for first action, succeeds, action instance is assigned to 'actions' array: act_simple refcnt balance = +2 actions[] = { [0]=act1 } 5. tcf_action_init_1() fails for second action, 'actions' array not changed, goto err: act_simple refcnt balance = +2 actions[] = { [0]=act1 } 6. tcf_action_destroy() is called for 'actions' array, last reference to first action is released, tcf_action_destroy_1() calls module_put() for actions module: act_simple refcnt balance = +1 actions[] = {} 7. err_mod loop starts iterating over ops array, executes module_put() for first actions ops: act_simple refcnt balance = 0 actions[] = {} 7. err_mod loop executes module_put() for second actions ops: act_simple refcnt balance = -1 actions[] = {} The goal of my fix is to not unconditionally release the module reference for successfully initialized actions because this is already handled by action destroy code. Hope this explanation clarifies things. Regards, Vlad
On Sat, Apr 3, 2021 at 3:01 AM Vlad Buslov <vladbu@nvidia.com> wrote: > So, the following happens in reproduction provided in commit message > when executing "tc actions add action simple sdata \"1\" index 1 > action simple sdata \"2\" index 2" command: > > 1. tcf_action_init() is called with batch of two actions of same type, > no module references are held, 'actions' array is empty: > > act_simple refcnt balance = 0 > actions[] = {} > > 2. tc_action_load_ops() is called for first action: > > act_simple refcnt balance = +1 > actions[] = {} > > 3. tc_action_load_ops() is called for second action: > > act_simple refcnt balance = +2 > actions[] = {} > > 4. tcf_action_init_1() called for first action, succeeds, action > instance is assigned to 'actions' array: > > act_simple refcnt balance = +2 > actions[] = { [0]=act1 } > > 5. tcf_action_init_1() fails for second action, 'actions' array not > changed, goto err: > > act_simple refcnt balance = +2 > actions[] = { [0]=act1 } > > 6. tcf_action_destroy() is called for 'actions' array, last reference to > first action is released, tcf_action_destroy_1() calls module_put() for > actions module: > > act_simple refcnt balance = +1 > actions[] = {} > > 7. err_mod loop starts iterating over ops array, executes module_put() > for first actions ops: > > act_simple refcnt balance = 0 > actions[] = {} > > 7. err_mod loop executes module_put() for second actions ops: > > act_simple refcnt balance = -1 > actions[] = {} > > > The goal of my fix is to not unconditionally release the module > reference for successfully initialized actions because this is already > handled by action destroy code. Hope this explanation clarifies things. Great explanation! It seems harder and harder to understand the module refcnt here. How about we just take the refcnt when we successfully create an action? Something like this: diff --git a/net/sched/act_api.c b/net/sched/act_api.c index b919826939e0..075cc80480bf 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -493,6 +493,7 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est, } p->idrinfo = idrinfo; + __module_get(ops->owner); p->ops = ops; *a = p; return 0; @@ -1035,13 +1036,6 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, if (!name) a->hw_stats = hw_stats; - /* module count goes up only when brand new policy is created - * if it exists and is only bound to in a_o->init() then - * ACT_P_CREATED is not returned (a zero is). - */ - if (err != ACT_P_CREATED) - module_put(a_o->owner); - return a; err_out: @@ -1100,7 +1094,8 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla, tcf_idr_insert_many(actions); *attr_size = tcf_action_full_attrs_size(sz); - return i - 1; + err = i - 1; + goto err_mod: err: tcf_action_destroy(actions, bind); The idea is on the higher level we hold refcnt when loading module and put it back _unconditionally_ when returning, and hold a refcnt only when we create an action and conditionally put it back when an error happens. With pseudo code, it is something like this: load_ops() // module refcnt +1 init_actions(); // module refcnt +1 only when create a new one if (err) // module refcnt -1 when we delete one module_put(); module_put(); // module refcnt -1 This looks much easier to track. What do you think? Thanks!
On Tue 06 Apr 2021 at 01:56, Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Sat, Apr 3, 2021 at 3:01 AM Vlad Buslov <vladbu@nvidia.com> wrote: >> So, the following happens in reproduction provided in commit message >> when executing "tc actions add action simple sdata \"1\" index 1 >> action simple sdata \"2\" index 2" command: >> >> 1. tcf_action_init() is called with batch of two actions of same type, >> no module references are held, 'actions' array is empty: >> >> act_simple refcnt balance = 0 >> actions[] = {} >> >> 2. tc_action_load_ops() is called for first action: >> >> act_simple refcnt balance = +1 >> actions[] = {} >> >> 3. tc_action_load_ops() is called for second action: >> >> act_simple refcnt balance = +2 >> actions[] = {} >> >> 4. tcf_action_init_1() called for first action, succeeds, action >> instance is assigned to 'actions' array: >> >> act_simple refcnt balance = +2 >> actions[] = { [0]=act1 } >> >> 5. tcf_action_init_1() fails for second action, 'actions' array not >> changed, goto err: >> >> act_simple refcnt balance = +2 >> actions[] = { [0]=act1 } >> >> 6. tcf_action_destroy() is called for 'actions' array, last reference to >> first action is released, tcf_action_destroy_1() calls module_put() for >> actions module: >> >> act_simple refcnt balance = +1 >> actions[] = {} >> >> 7. err_mod loop starts iterating over ops array, executes module_put() >> for first actions ops: >> >> act_simple refcnt balance = 0 >> actions[] = {} >> >> 7. err_mod loop executes module_put() for second actions ops: >> >> act_simple refcnt balance = -1 >> actions[] = {} >> >> >> The goal of my fix is to not unconditionally release the module >> reference for successfully initialized actions because this is already >> handled by action destroy code. Hope this explanation clarifies things. > > Great explanation! It seems harder and harder to understand the > module refcnt here. How about we just take the refcnt when we > successfully create an action? Something like this: > > diff --git a/net/sched/act_api.c b/net/sched/act_api.c > index b919826939e0..075cc80480bf 100644 > --- a/net/sched/act_api.c > +++ b/net/sched/act_api.c > @@ -493,6 +493,7 @@ int tcf_idr_create(struct tc_action_net *tn, u32 > index, struct nlattr *est, > } > > p->idrinfo = idrinfo; > + __module_get(ops->owner); > p->ops = ops; > *a = p; > return 0; > @@ -1035,13 +1036,6 @@ struct tc_action *tcf_action_init_1(struct net > *net, struct tcf_proto *tp, > if (!name) > a->hw_stats = hw_stats; > > - /* module count goes up only when brand new policy is created > - * if it exists and is only bound to in a_o->init() then > - * ACT_P_CREATED is not returned (a zero is). > - */ > - if (err != ACT_P_CREATED) > - module_put(a_o->owner); > - > return a; > > err_out: > @@ -1100,7 +1094,8 @@ int tcf_action_init(struct net *net, struct > tcf_proto *tp, struct nlattr *nla, > tcf_idr_insert_many(actions); > > *attr_size = tcf_action_full_attrs_size(sz); > - return i - 1; > + err = i - 1; > + goto err_mod: > > err: > tcf_action_destroy(actions, bind); > > The idea is on the higher level we hold refcnt when loading module and > put it back _unconditionally_ when returning, and hold a refcnt only when > we create an action and conditionally put it back when an error happens. > With pseudo code, it is something like this: > > load_ops() // module refcnt +1 > init_actions(); // module refcnt +1 only when create a new one > if (err) > // module refcnt -1 when we delete one > module_put(); > module_put(); // module refcnt -1 > > This looks much easier to track. What do you think? > > Thanks! Indeed, your suggestion looks more straightforward. The only thing we need to mind is that action->init() callbacks assume that caller releases the module even after calling tcf_idr_create(), so we also need to modify tcf_idr_release() (used by error handlers in action->init() implementations) to release the module. I'll run some tests tomorrow to verify that I'm not missing anything else. Regards, Vlad
diff --git a/net/sched/act_api.c b/net/sched/act_api.c index eb20a75796d5..4ef556906e32 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -753,20 +753,28 @@ int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions, } EXPORT_SYMBOL(tcf_action_exec); -int tcf_action_destroy(struct tc_action *actions[], int bind) +static int tcf_action_destroy_1(struct tc_action *a, int bind) { const struct tc_action_ops *ops; + int ret; + + ops = a->ops; + ret = __tcf_idr_release(a, bind, true); + if (ret == ACT_P_DELETED) + module_put(ops->owner); + return ret; +} + +int tcf_action_destroy(struct tc_action *actions[], int bind) +{ struct tc_action *a; int ret = 0, i; for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) { a = actions[i]; actions[i] = NULL; - ops = a->ops; - ret = __tcf_idr_release(a, bind, true); - if (ret == ACT_P_DELETED) - module_put(ops->owner); - else if (ret < 0) + ret = tcf_action_destroy_1(a, bind); + if (ret < 0) return ret; } return ret; @@ -1082,7 +1090,7 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla, a_o = tc_action_load_ops(name, tb[i], rtnl_held, extack); if (IS_ERR(a_o)) { err = PTR_ERR(a_o); - goto err_mod; + goto err; } ops[i - 1] = a_o; } @@ -1109,11 +1117,13 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla, return i - 1; err: - tcf_action_destroy(actions, bind); -err_mod: for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { - if (ops[i]) + if (actions[i]) { + tcf_action_destroy_1(actions[i], bind); + actions[i] = NULL; + } else if (ops[i]) { module_put(ops[i]->owner); + } } return err; }
With recent changes that separated action module load from action initialization tcf_action_init() function error handling code was modified to manually release the loaded modules if loading/initialization of any further action in same batch failed. For the case when all modules successfully loaded and some of the actions were initialized before one of them failed in init handler. In this case for all previous actions the module will be released twice by the error handler: First time by the loop that manually calls module_put() for all ops, and second time by the action destroy code that puts the module after destroying the action. Reproduction: $ sudo tc actions add action simple sdata \"2\" index 2 $ sudo tc actions add action simple sdata \"1\" index 1 action simple sdata \"2\" index 2 RTNETLINK answers: File exists We have an error talking to the kernel $ sudo tc actions ls action simple total acts 1 action order 0: Simple <"2"> index 2 ref 1 bind 0 $ sudo tc actions flush action simple $ sudo tc actions ls action simple $ sudo tc actions add action simple sdata \"2\" index 2 Error: Failed to load TC action module. We have an error talking to the kernel $ lsmod | grep simple act_simple 20480 -1 Fix the issue by refactoring tcf_action_init() error handling code to properly account for the case of partially initialized action list and only put the module for actions that haven't been initialized. Fixes: d349f9976868 ("net_sched: fix RTNL deadlock again caused by request_module()") Signed-off-by: Vlad Buslov <vladbu@nvidia.com> --- net/sched/act_api.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-)