Message ID | 462700507.23.1492589388406@webmail.proxmox.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
On 17-04-19 04:09 AM, Wolfgang Bumiller wrote: > > diff --git a/net/sched/act_api.c b/net/sched/act_api.c > index 8cc883c063f0..c7e5e437b847 100644 > --- a/net/sched/act_api.c > +++ b/net/sched/act_api.c > @@ -555,6 +555,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla, > struct nlattr *tb[TCA_ACT_MAX + 1]; > struct nlattr *kind; > int err; > + bool created; > > if (name == NULL) { > err = nla_parse_nested(tb, TCA_ACT_MAX, nla, NULL); > @@ -603,20 +604,19 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla, > err = a_o->init(net, nla, est, &a, ovr, bind); > if (err < 0) > goto err_mod; > + created = err == ACT_P_CREATED; > > if (name == NULL && tb[TCA_ACT_COOKIE]) { > int cklen = nla_len(tb[TCA_ACT_COOKIE]); > > if (cklen > TC_COOKIE_MAX_SIZE) { > err = -EINVAL; > - tcf_hash_release(a, bind); > - goto err_mod; > + goto err_release; > } > > if (nla_memdup_cookie(a, tb) < 0) { > err = -ENOMEM; > - tcf_hash_release(a, bind); > - goto err_mod; > + goto err_release; > } > } > > @@ -624,11 +624,14 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla, > * 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) > + if (!created) > module_put(a_o->owner); > > return a; > > +err_release: > + if (created) > + tcf_hash_release(a, bind); > err_mod: > module_put(a_o->owner); > err_out: > This solves one issue, but I am afraid the issue Cong mentioned is a possibility still. Lets say user did a replace and tried to also replace the cookie in that transaction. The init() succeeds but the cookie allocation fails. To be correct we'll have to undo the replace i.e something like uninit() which will restore back the old values. This is very complex and unnecessary. My suggestion: If we move the cookie allocation before init we can save it and only when init succeeds do we attach it to the action, otherwise we free it on error path. cheers, jamal
On Wed, Apr 19, 2017 at 4:32 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > This solves one issue, but I am afraid the issue Cong mentioned is a > possibility still. > Lets say user did a replace and tried to also replace the cookie > in that transaction. The init() succeeds but the cookie allocation > fails. To be correct we'll have to undo the replace i.e something > like uninit() which will restore back the old values. > This is very complex and unnecessary. It is not complex once we move to RCU completely, replacement is merely a pointer assignment, rollback is same. It is necessary too according to the rules of RCU, as I said before, the current code is broken, we can't modify an existing action with RCU, we have to make a copy. I do have plan to make actions truly RCU, but I have to redesign the action API's first.
On 17-04-19 01:25 PM, Cong Wang wrote: > On Wed, Apr 19, 2017 at 4:32 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> This solves one issue, but I am afraid the issue Cong mentioned is a >> possibility still. >> Lets say user did a replace and tried to also replace the cookie >> in that transaction. The init() succeeds but the cookie allocation >> fails. To be correct we'll have to undo the replace i.e something >> like uninit() which will restore back the old values. >> This is very complex and unnecessary. > > It is not complex once we move to RCU completely, replacement > is merely a pointer assignment, rollback is same. > Nice to hear. > It is necessary too according to the rules of RCU, as I said before, the > current code is broken, we can't modify an existing action with RCU, > we have to make a copy. I do have plan to make actions truly RCU, > but I have to redesign the action API's first. Ok ;-> Lucas: Lets work to get those test suites in. cheers, jamal
diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 8cc883c063f0..c7e5e437b847 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -555,6 +555,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla, struct nlattr *tb[TCA_ACT_MAX + 1]; struct nlattr *kind; int err; + bool created; if (name == NULL) { err = nla_parse_nested(tb, TCA_ACT_MAX, nla, NULL); @@ -603,20 +604,19 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla, err = a_o->init(net, nla, est, &a, ovr, bind); if (err < 0) goto err_mod; + created = err == ACT_P_CREATED; if (name == NULL && tb[TCA_ACT_COOKIE]) { int cklen = nla_len(tb[TCA_ACT_COOKIE]); if (cklen > TC_COOKIE_MAX_SIZE) { err = -EINVAL; - tcf_hash_release(a, bind); - goto err_mod; + goto err_release; } if (nla_memdup_cookie(a, tb) < 0) { err = -ENOMEM; - tcf_hash_release(a, bind); - goto err_mod; + goto err_release; } } @@ -624,11 +624,14 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla, * 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) + if (!created) module_put(a_o->owner); return a; +err_release: + if (created) + tcf_hash_release(a, bind); err_mod: module_put(a_o->owner); err_out: