| Message ID | 1526308035-12484-7-git-send-email-vladbu@mellanox.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | Modify action API for implementing lockless actions | expand |
Mon, May 14, 2018 at 04:27:07PM CEST, vladbu@mellanox.com wrote: >Implement helper function to delete action using new action ops delete >function implemented by each action for lockless execution. Reading this sentense for 4 times. I still don't understand what you say :( > >Implement action put function that releases reference to action and frees When you write "Implement action put function" it is cryptic. Just say "Implement function __tcf_action_put()". >it if necessary. Refactor action deletion code to use new put function and >not to rely on rtnl lock. Remove rtnl lock assertions that are no longer >needed. [...]
Mon, May 14, 2018 at 04:27:07PM CEST, vladbu@mellanox.com wrote: [...] >+static int tcf_action_del_1(struct net *net, char *kind, u32 index, >+ struct netlink_ext_ack *extack) >+{ >+ const struct tc_action_ops *ops; >+ int err = -EINVAL; >+ >+ ops = tc_lookup_action_n(kind); >+ if (!ops) { How this can happen? Apparently you hold reference to the module since you put it couple lines below. In that case, I don't really understand why you need to lookup and just don't use "ops" pointer you have in tcf_action_delete(). >+ NL_SET_ERR_MSG(extack, "Specified TC action not found"); >+ goto err_out; >+ } >+ >+ if (ops->delete) >+ err = ops->delete(net, index); >+ >+ module_put(ops->owner); >+err_out: >+ return err; >+} >+ > static int tca_action_flush(struct net *net, struct nlattr *nla, > struct nlmsghdr *n, u32 portid, > struct netlink_ext_ack *extack) >@@ -1052,6 +1088,36 @@ static int tca_action_flush(struct net *net, struct nlattr *nla, > return err; > } > >+static int tcf_action_delete(struct net *net, struct list_head *actions, >+ struct netlink_ext_ack *extack) >+{ >+ int ret; >+ struct tc_action *a, *tmp; >+ char kind[IFNAMSIZ]; >+ u32 act_index; >+ >+ list_for_each_entry_safe(a, tmp, actions, list) { >+ const struct tc_action_ops *ops = a->ops; >+ >+ /* Actions can be deleted concurrently >+ * so we must save their type and id to search again >+ * after reference is released. >+ */ >+ strncpy(kind, a->ops->kind, sizeof(kind) - 1); >+ act_index = a->tcfa_index; >+ >+ list_del(&a->list); >+ if (tcf_action_put(a)) >+ module_put(ops->owner); >+ >+ /* now do the delete */ >+ ret = tcf_action_del_1(net, kind, act_index, extack); >+ if (ret < 0) >+ return ret; >+ } >+ return 0; >+} >+ [...]
On Mon 14 May 2018 at 16:47, Jiri Pirko <jiri@resnulli.us> wrote: > Mon, May 14, 2018 at 04:27:07PM CEST, vladbu@mellanox.com wrote: > > [...] > > >>+static int tcf_action_del_1(struct net *net, char *kind, u32 index, >>+ struct netlink_ext_ack *extack) >>+{ >>+ const struct tc_action_ops *ops; >>+ int err = -EINVAL; >>+ >>+ ops = tc_lookup_action_n(kind); >>+ if (!ops) { > > How this can happen? Apparently you hold reference to the module since > you put it couple lines below. In that case, I don't really understand > why you need to lookup and just don't use "ops" pointer you have in > tcf_action_delete(). The problem is that I cant really delete action while holding reference to it. I can try to decrement reference twice, however that might result race condition if another task tries to delete that action at the same time. Imagine situation: 1. Action is in actions idr, refcount==1 2. Task tries to delete action, takes reference while working with it, refcount==2 3. Another task tries to delete same action, takes reference, refcount==3 4. First task decrements refcount twice, refcount==1 5. At the same time second task decrements refcount twice, refcount==-1 My solution is to save action index, but release the reference. Then try to lookup action again and delete it if it is still in idr. (was not concurrently deleted) Now same potential race condition with this patch: 1. Action is in actions idr, refcount==1 2. Task tries to delete action, takes reference while working with it, refcount==2 3. Another task tries to delete same action, takes reference, refcount==3 4. First task releases reference and deletes actions from idr, which results another refcount decrement, refcount==1 5. At the same time second task releases reference to action, refcount==0, action is deleted 6. When task tries to lookup-delete action from idr by index, action is not found. This case is handled correctly by code and no negative overflow of refcount happens. > > >>+ NL_SET_ERR_MSG(extack, "Specified TC action not found"); >>+ goto err_out; >>+ } >>+ >>+ if (ops->delete) >>+ err = ops->delete(net, index); >>+ >>+ module_put(ops->owner); >>+err_out: >>+ return err; >>+} >>+ >> static int tca_action_flush(struct net *net, struct nlattr *nla, >> struct nlmsghdr *n, u32 portid, >> struct netlink_ext_ack *extack) >>@@ -1052,6 +1088,36 @@ static int tca_action_flush(struct net *net, struct nlattr *nla, >> return err; >> } >> >>+static int tcf_action_delete(struct net *net, struct list_head *actions, >>+ struct netlink_ext_ack *extack) >>+{ >>+ int ret; >>+ struct tc_action *a, *tmp; >>+ char kind[IFNAMSIZ]; >>+ u32 act_index; >>+ >>+ list_for_each_entry_safe(a, tmp, actions, list) { >>+ const struct tc_action_ops *ops = a->ops; >>+ >>+ /* Actions can be deleted concurrently >>+ * so we must save their type and id to search again >>+ * after reference is released. >>+ */ >>+ strncpy(kind, a->ops->kind, sizeof(kind) - 1); >>+ act_index = a->tcfa_index; >>+ >>+ list_del(&a->list); >>+ if (tcf_action_put(a)) >>+ module_put(ops->owner); >>+ >>+ /* now do the delete */ >>+ ret = tcf_action_del_1(net, kind, act_index, extack); >>+ if (ret < 0) >>+ return ret; >>+ } >>+ return 0; >>+} >>+ > > [...]
Mon, May 14, 2018 at 09:07:06PM CEST, vladbu@mellanox.com wrote: > >On Mon 14 May 2018 at 16:47, Jiri Pirko <jiri@resnulli.us> wrote: >> Mon, May 14, 2018 at 04:27:07PM CEST, vladbu@mellanox.com wrote: >> >> [...] >> >> >>>+static int tcf_action_del_1(struct net *net, char *kind, u32 index, >>>+ struct netlink_ext_ack *extack) >>>+{ >>>+ const struct tc_action_ops *ops; >>>+ int err = -EINVAL; >>>+ >>>+ ops = tc_lookup_action_n(kind); >>>+ if (!ops) { >> >> How this can happen? Apparently you hold reference to the module since >> you put it couple lines below. In that case, I don't really understand >> why you need to lookup and just don't use "ops" pointer you have in >> tcf_action_delete(). > >The problem is that I cant really delete action while holding reference Wait a sec. I was talking about a "module reference" (module_put()) >to it. I can try to decrement reference twice, however that might result >race condition if another task tries to delete that action at the same >time. > >Imagine situation: >1. Action is in actions idr, refcount==1 >2. Task tries to delete action, takes reference while working with it, >refcount==2 >3. Another task tries to delete same action, takes reference, >refcount==3 >4. First task decrements refcount twice, refcount==1 >5. At the same time second task decrements refcount twice, refcount==-1 > >My solution is to save action index, but release the reference. Then try >to lookup action again and delete it if it is still in idr. (was not >concurrently deleted) > >Now same potential race condition with this patch: >1. Action is in actions idr, refcount==1 >2. Task tries to delete action, takes reference while working with it, >refcount==2 >3. Another task tries to delete same action, takes reference, >refcount==3 >4. First task releases reference and deletes actions from idr, which >results another refcount decrement, refcount==1 >5. At the same time second task releases reference to action, >refcount==0, action is deleted >6. When task tries to lookup-delete action from idr by index, action is >not found. This case is handled correctly by code and no negative >overflow of refcount happens. > >> >> >>>+ NL_SET_ERR_MSG(extack, "Specified TC action not found"); >>>+ goto err_out; >>>+ } >>>+ >>>+ if (ops->delete) >>>+ err = ops->delete(net, index); >>>+ >>>+ module_put(ops->owner); >>>+err_out: >>>+ return err; >>>+} >>>+ >>> static int tca_action_flush(struct net *net, struct nlattr *nla, >>> struct nlmsghdr *n, u32 portid, >>> struct netlink_ext_ack *extack) >>>@@ -1052,6 +1088,36 @@ static int tca_action_flush(struct net *net, struct nlattr *nla, >>> return err; >>> } >>> >>>+static int tcf_action_delete(struct net *net, struct list_head *actions, >>>+ struct netlink_ext_ack *extack) >>>+{ >>>+ int ret; >>>+ struct tc_action *a, *tmp; >>>+ char kind[IFNAMSIZ]; >>>+ u32 act_index; >>>+ >>>+ list_for_each_entry_safe(a, tmp, actions, list) { >>>+ const struct tc_action_ops *ops = a->ops; >>>+ >>>+ /* Actions can be deleted concurrently >>>+ * so we must save their type and id to search again >>>+ * after reference is released. >>>+ */ >>>+ strncpy(kind, a->ops->kind, sizeof(kind) - 1); >>>+ act_index = a->tcfa_index; >>>+ >>>+ list_del(&a->list); >>>+ if (tcf_action_put(a)) >>>+ module_put(ops->owner); >>>+ >>>+ /* now do the delete */ >>>+ ret = tcf_action_del_1(net, kind, act_index, extack); >>>+ if (ret < 0) >>>+ return ret; >>>+ } >>>+ return 0; >>>+} >>>+ >> >> [...] >
On Tue 15 May 2018 at 09:03, Jiri Pirko <jiri@resnulli.us> wrote: > Mon, May 14, 2018 at 09:07:06PM CEST, vladbu@mellanox.com wrote: >> >>On Mon 14 May 2018 at 16:47, Jiri Pirko <jiri@resnulli.us> wrote: >>> Mon, May 14, 2018 at 04:27:07PM CEST, vladbu@mellanox.com wrote: >>> >>> [...] >>> >>> >>>>+static int tcf_action_del_1(struct net *net, char *kind, u32 index, >>>>+ struct netlink_ext_ack *extack) >>>>+{ >>>>+ const struct tc_action_ops *ops; >>>>+ int err = -EINVAL; >>>>+ >>>>+ ops = tc_lookup_action_n(kind); >>>>+ if (!ops) { >>> >>> How this can happen? Apparently you hold reference to the module since >>> you put it couple lines below. In that case, I don't really understand >>> why you need to lookup and just don't use "ops" pointer you have in >>> tcf_action_delete(). >> >>The problem is that I cant really delete action while holding reference > > Wait a sec. I was talking about a "module reference" (module_put()) I misunderstood your question. Yes, I guess I can just save return value of tcf_action_put to variable, continue using ops pointer and only call module_put after delete. > > >>to it. I can try to decrement reference twice, however that might result >>race condition if another task tries to delete that action at the same >>time. >> >>Imagine situation: >>1. Action is in actions idr, refcount==1 >>2. Task tries to delete action, takes reference while working with it, >>refcount==2 >>3. Another task tries to delete same action, takes reference, >>refcount==3 >>4. First task decrements refcount twice, refcount==1 >>5. At the same time second task decrements refcount twice, refcount==-1 >> >>My solution is to save action index, but release the reference. Then try >>to lookup action again and delete it if it is still in idr. (was not >>concurrently deleted) >> >>Now same potential race condition with this patch: >>1. Action is in actions idr, refcount==1 >>2. Task tries to delete action, takes reference while working with it, >>refcount==2 >>3. Another task tries to delete same action, takes reference, >>refcount==3 >>4. First task releases reference and deletes actions from idr, which >>results another refcount decrement, refcount==1 >>5. At the same time second task releases reference to action, >>refcount==0, action is deleted >>6. When task tries to lookup-delete action from idr by index, action is >>not found. This case is handled correctly by code and no negative >>overflow of refcount happens. >> >>> >>> >>>>+ NL_SET_ERR_MSG(extack, "Specified TC action not found"); >>>>+ goto err_out; >>>>+ } >>>>+ >>>>+ if (ops->delete) >>>>+ err = ops->delete(net, index); >>>>+ >>>>+ module_put(ops->owner); >>>>+err_out: >>>>+ return err; >>>>+} >>>>+ >>>> static int tca_action_flush(struct net *net, struct nlattr *nla, >>>> struct nlmsghdr *n, u32 portid, >>>> struct netlink_ext_ack *extack) >>>>@@ -1052,6 +1088,36 @@ static int tca_action_flush(struct net *net, struct nlattr *nla, >>>> return err; >>>> } >>>> >>>>+static int tcf_action_delete(struct net *net, struct list_head *actions, >>>>+ struct netlink_ext_ack *extack) >>>>+{ >>>>+ int ret; >>>>+ struct tc_action *a, *tmp; >>>>+ char kind[IFNAMSIZ]; >>>>+ u32 act_index; >>>>+ >>>>+ list_for_each_entry_safe(a, tmp, actions, list) { >>>>+ const struct tc_action_ops *ops = a->ops; >>>>+ >>>>+ /* Actions can be deleted concurrently >>>>+ * so we must save their type and id to search again >>>>+ * after reference is released. >>>>+ */ >>>>+ strncpy(kind, a->ops->kind, sizeof(kind) - 1); >>>>+ act_index = a->tcfa_index; >>>>+ >>>>+ list_del(&a->list); >>>>+ if (tcf_action_put(a)) >>>>+ module_put(ops->owner); >>>>+ >>>>+ /* now do the delete */ >>>>+ ret = tcf_action_del_1(net, kind, act_index, extack); >>>>+ if (ret < 0) >>>>+ return ret; >>>>+ } >>>>+ return 0; >>>>+} >>>>+ >>> >>> [...] >>
On Mon, May 14, 2018 at 05:27:07PM +0300, Vlad Buslov wrote: ... > @@ -1052,6 +1088,36 @@ static int tca_action_flush(struct net *net, struct nlattr *nla, > return err; > } > > +static int tcf_action_delete(struct net *net, struct list_head *actions, > + struct netlink_ext_ack *extack) > +{ > + int ret; Reverse christmass tree.. this line should be the last in variable declarations. > + struct tc_action *a, *tmp; > + char kind[IFNAMSIZ]; > + u32 act_index; > + > + list_for_each_entry_safe(a, tmp, actions, list) { > + const struct tc_action_ops *ops = a->ops; > + > + /* Actions can be deleted concurrently > + * so we must save their type and id to search again > + * after reference is released. > + */ > + strncpy(kind, a->ops->kind, sizeof(kind) - 1); This may be problematic. Why strncpy here? a->ops->kind is also of size IFNAMSIZ. If a->ops->kind is actually IFNAMSIZ-1 long, kind here won't be NULL terminated, as kind is not initialized and strncpy won't add the NULL. > + act_index = a->tcfa_index; > + > + list_del(&a->list); > + if (tcf_action_put(a)) > + module_put(ops->owner); > + > + /* now do the delete */ > + ret = tcf_action_del_1(net, kind, act_index, extack); > + if (ret < 0) > + return ret; > + } > + return 0; > +}
Sat, May 19, 2018 at 11:43:27PM CEST, marcelo.leitner@gmail.com wrote: >On Mon, May 14, 2018 at 05:27:07PM +0300, Vlad Buslov wrote: >... >> @@ -1052,6 +1088,36 @@ static int tca_action_flush(struct net *net, struct nlattr *nla, >> return err; >> } >> >> +static int tcf_action_delete(struct net *net, struct list_head *actions, >> + struct netlink_ext_ack *extack) >> +{ >> + int ret; > >Reverse christmass tree.. this line should be the last in variable >declarations. > >> + struct tc_action *a, *tmp; >> + char kind[IFNAMSIZ]; >> + u32 act_index; >> + >> + list_for_each_entry_safe(a, tmp, actions, list) { >> + const struct tc_action_ops *ops = a->ops; >> + >> + /* Actions can be deleted concurrently >> + * so we must save their type and id to search again >> + * after reference is released. >> + */ >> + strncpy(kind, a->ops->kind, sizeof(kind) - 1); > >This may be problematic. Why strncpy here? This is not necessary if Vlad is going to hold module referece, ops cannot disappear. > >a->ops->kind is also of size IFNAMSIZ. If a->ops->kind is actually >IFNAMSIZ-1 long, kind here won't be NULL terminated, as kind is not >initialized and strncpy won't add the NULL. > >> + act_index = a->tcfa_index; >> + >> + list_del(&a->list); >> + if (tcf_action_put(a)) >> + module_put(ops->owner); >> + >> + /* now do the delete */ >> + ret = tcf_action_del_1(net, kind, act_index, extack); >> + if (ret < 0) >> + return ret; >> + } >> + return 0; >> +}
On Sun 20 May 2018 at 06:22, Jiri Pirko <jiri@resnulli.us> wrote: > Sat, May 19, 2018 at 11:43:27PM CEST, marcelo.leitner@gmail.com wrote: >>On Mon, May 14, 2018 at 05:27:07PM +0300, Vlad Buslov wrote: >>... >>> @@ -1052,6 +1088,36 @@ static int tca_action_flush(struct net *net, struct nlattr *nla, >>> return err; >>> } >>> >>> +static int tcf_action_delete(struct net *net, struct list_head *actions, >>> + struct netlink_ext_ack *extack) >>> +{ >>> + int ret; >> >>Reverse christmass tree.. this line should be the last in variable >>declarations. >> >>> + struct tc_action *a, *tmp; >>> + char kind[IFNAMSIZ]; >>> + u32 act_index; >>> + >>> + list_for_each_entry_safe(a, tmp, actions, list) { >>> + const struct tc_action_ops *ops = a->ops; >>> + >>> + /* Actions can be deleted concurrently >>> + * so we must save their type and id to search again >>> + * after reference is released. >>> + */ >>> + strncpy(kind, a->ops->kind, sizeof(kind) - 1); >> >>This may be problematic. Why strncpy here? > > This is not necessary if Vlad is going to hold module referece, ops > cannot disappear. Yes, I've already refactored this code to reuse ops. > > >> >>a->ops->kind is also of size IFNAMSIZ. If a->ops->kind is actually >>IFNAMSIZ-1 long, kind here won't be NULL terminated, as kind is not >>initialized and strncpy won't add the NULL. >> >>> + act_index = a->tcfa_index; >>> + >>> + list_del(&a->list); >>> + if (tcf_action_put(a)) >>> + module_put(ops->owner); >>> + >>> + /* now do the delete */ >>> + ret = tcf_action_del_1(net, kind, act_index, extack); >>> + if (ret < 0) >>> + return ret; >>> + } >>> + return 0; >>> +}
diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 9459cce..d324a4c 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -90,21 +90,39 @@ static void free_tcf(struct tc_action *p) kfree(p); } -static void tcf_idr_remove(struct tcf_idrinfo *idrinfo, struct tc_action *p) +static void tcf_action_cleanup(struct tc_action *p) { - spin_lock_bh(&idrinfo->lock); - idr_remove(&idrinfo->action_idr, p->tcfa_index); - spin_unlock_bh(&idrinfo->lock); + if (p->ops->cleanup) + p->ops->cleanup(p); + gen_kill_estimator(&p->tcfa_rate_est); free_tcf(p); } +static int __tcf_action_put(struct tc_action *p, bool bind) +{ + struct tcf_idrinfo *idrinfo = p->idrinfo; + + if (refcount_dec_and_lock(&p->tcfa_refcnt, &idrinfo->lock)) { + if (bind) + atomic_dec(&p->tcfa_bindcnt); + idr_remove(&idrinfo->action_idr, p->tcfa_index); + spin_unlock(&idrinfo->lock); + + tcf_action_cleanup(p); + return 1; + } + + if (bind) + atomic_dec(&p->tcfa_bindcnt); + + return 0; +} + int __tcf_idr_release(struct tc_action *p, bool bind, bool strict) { int ret = 0; - ASSERT_RTNL(); - /* Release with strict==1 and bind==0 is only called through act API * interface (classifiers always bind). Only case when action with * positive reference count and zero bind count can exist is when it was @@ -118,18 +136,11 @@ int __tcf_idr_release(struct tc_action *p, bool bind, bool strict) * are acceptable. */ if (p) { - if (bind) - atomic_dec(&p->tcfa_bindcnt); - else if (strict && atomic_read(&p->tcfa_bindcnt) > 0) + if (!bind && strict && atomic_read(&p->tcfa_bindcnt) > 0) return -EPERM; - if (atomic_read(&p->tcfa_bindcnt) <= 0 && - refcount_dec_and_test(&p->tcfa_refcnt)) { - if (p->ops->cleanup) - p->ops->cleanup(p); - tcf_idr_remove(p->idrinfo, p); + if (__tcf_action_put(p, bind)) ret = ACT_P_DELETED; - } } return ret; @@ -576,6 +587,11 @@ int tcf_action_destroy(struct list_head *actions, int bind) return ret; } +static int tcf_action_put(struct tc_action *p) +{ + return __tcf_action_put(p, false); +} + int tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int bind, int ref) { @@ -975,6 +991,26 @@ static struct tc_action *tcf_action_get_1(struct net *net, struct nlattr *nla, return ERR_PTR(err); } +static int tcf_action_del_1(struct net *net, char *kind, u32 index, + struct netlink_ext_ack *extack) +{ + const struct tc_action_ops *ops; + int err = -EINVAL; + + ops = tc_lookup_action_n(kind); + if (!ops) { + NL_SET_ERR_MSG(extack, "Specified TC action not found"); + goto err_out; + } + + if (ops->delete) + err = ops->delete(net, index); + + module_put(ops->owner); +err_out: + return err; +} + static int tca_action_flush(struct net *net, struct nlattr *nla, struct nlmsghdr *n, u32 portid, struct netlink_ext_ack *extack) @@ -1052,6 +1088,36 @@ static int tca_action_flush(struct net *net, struct nlattr *nla, return err; } +static int tcf_action_delete(struct net *net, struct list_head *actions, + struct netlink_ext_ack *extack) +{ + int ret; + struct tc_action *a, *tmp; + char kind[IFNAMSIZ]; + u32 act_index; + + list_for_each_entry_safe(a, tmp, actions, list) { + const struct tc_action_ops *ops = a->ops; + + /* Actions can be deleted concurrently + * so we must save their type and id to search again + * after reference is released. + */ + strncpy(kind, a->ops->kind, sizeof(kind) - 1); + act_index = a->tcfa_index; + + list_del(&a->list); + if (tcf_action_put(a)) + module_put(ops->owner); + + /* now do the delete */ + ret = tcf_action_del_1(net, kind, act_index, extack); + if (ret < 0) + return ret; + } + return 0; +} + static int tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions, u32 portid, size_t attr_size, struct netlink_ext_ack *extack) @@ -1072,7 +1138,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions, } /* now do the delete */ - ret = tcf_action_destroy(actions, 0); + ret = tcf_action_delete(net, actions, extack); if (ret < 0) { NL_SET_ERR_MSG(extack, "Failed to delete TC action"); kfree_skb(skb); diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index bcba94b..00b88e5 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -1417,7 +1417,6 @@ void tcf_exts_destroy(struct tcf_exts *exts) #ifdef CONFIG_NET_CLS_ACT LIST_HEAD(actions); - ASSERT_RTNL(); tcf_exts_to_list(exts, &actions); tcf_action_destroy(&actions, TCA_ACT_UNBIND); kfree(exts->actions);
Implement helper function to delete action using new action ops delete function implemented by each action for lockless execution. Implement action put function that releases reference to action and frees it if necessary. Refactor action deletion code to use new put function and not to rely on rtnl lock. Remove rtnl lock assertions that are no longer needed. Signed-off-by: Vlad Buslov <vladbu@mellanox.com> --- net/sched/act_api.c | 98 ++++++++++++++++++++++++++++++++++++++++++++--------- net/sched/cls_api.c | 1 - 2 files changed, 82 insertions(+), 17 deletions(-)