| Message ID | 20210331164012.28653-2-vladbu@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Action initalization fixes | expand |
On Wed, Mar 31, 2021 at 9:41 AM Vlad Buslov <vladbu@nvidia.com> wrote: > > Action init code increments reference counter when it changes an action. > This is the desired behavior for cls API which needs to obtain action > reference for every classifier that points to action. However, act API just > needs to change the action and releases the reference before returning. > This sequence breaks when the requested action doesn't exist, which causes > act API init code to create new action with specified index, but action is > still released before returning and is deleted (unless it was referenced > concurrently by cls API). Please also add a summary of how you fix it. From what I understand, it seems you just skip the refcnt put of successful cases? One comment below. > > Fixes: cae422f379f3 ("net: sched: use reference counting action init") > Reported-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > Signed-off-by: Vlad Buslov <vladbu@nvidia.com> > --- > include/net/act_api.h | 5 +++-- > net/sched/act_api.c | 27 +++++++++++++++++---------- > net/sched/cls_api.c | 9 +++++---- > 3 files changed, 25 insertions(+), 16 deletions(-) > > diff --git a/include/net/act_api.h b/include/net/act_api.h > index 2bf3092ae7ec..312f0f6554a0 100644 > --- a/include/net/act_api.h > +++ b/include/net/act_api.h > @@ -185,7 +185,7 @@ 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, > struct nlattr *est, char *name, int ovr, int bind, > - struct tc_action *actions[], size_t *attr_size, > + struct tc_action *actions[], int init_res[], size_t *attr_size, > bool rtnl_held, struct netlink_ext_ack *extack); > struct tc_action_ops *tc_action_load_ops(char *name, struct nlattr *nla, > bool rtnl_held, > @@ -193,7 +193,8 @@ struct tc_action_ops *tc_action_load_ops(char *name, struct nlattr *nla, > struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, > struct nlattr *nla, struct nlattr *est, > char *name, int ovr, int bind, > - struct tc_action_ops *ops, bool rtnl_held, > + struct tc_action_ops *a_o, int *init_res, > + bool rtnl_held, > struct netlink_ext_ack *extack); > int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[], int bind, > int ref, bool terse); > diff --git a/net/sched/act_api.c b/net/sched/act_api.c > index b919826939e0..eb20a75796d5 100644 > --- a/net/sched/act_api.c > +++ b/net/sched/act_api.c > @@ -777,8 +777,11 @@ static int tcf_action_put(struct tc_action *p) > return __tcf_action_put(p, false); > } > > -/* Put all actions in this array, skip those NULL's. */ > -static void tcf_action_put_many(struct tc_action *actions[]) > +/* Put all actions in this array, skip those NULL's. If cond array is provided > + * by caller, then only put actions that match. > + */ > +static void tcf_action_put_many(struct tc_action *actions[], int *cond, > + int match) > { > int i; > > @@ -786,7 +789,7 @@ static void tcf_action_put_many(struct tc_action *actions[]) > struct tc_action *a = actions[i]; > const struct tc_action_ops *ops; > > - if (!a) > + if (!a || (cond && cond[i] != match)) This looks a bit odd. How about passing an array of action pointers which only contains those that need to be put? Thanks.
On Sat 03 Apr 2021 at 01:13, Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Wed, Mar 31, 2021 at 9:41 AM Vlad Buslov <vladbu@nvidia.com> wrote: >> >> Action init code increments reference counter when it changes an action. >> This is the desired behavior for cls API which needs to obtain action >> reference for every classifier that points to action. However, act API just >> needs to change the action and releases the reference before returning. >> This sequence breaks when the requested action doesn't exist, which causes >> act API init code to create new action with specified index, but action is >> still released before returning and is deleted (unless it was referenced >> concurrently by cls API). > > Please also add a summary of how you fix it. From what I understand, > it seems you just skip the refcnt put of successful cases? Oops, I didn't regenerate patches after amending the commit message. This should include the following paragraph: Extend tcf_action_init() to accept 'init_res' array and initialize it with action->ops->init() result. Refactor tcf_action_put_many() to also accept such array and only put actions for which init result match provided value. Modify tcf_action_add() to only put actions with init_res==0 instead of unconditionally putting all actions when user set NLM_F_REPLACE netlink message flag. > > One comment below. > >> >> Fixes: cae422f379f3 ("net: sched: use reference counting action init") >> Reported-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> >> Signed-off-by: Vlad Buslov <vladbu@nvidia.com> >> --- >> include/net/act_api.h | 5 +++-- >> net/sched/act_api.c | 27 +++++++++++++++++---------- >> net/sched/cls_api.c | 9 +++++---- >> 3 files changed, 25 insertions(+), 16 deletions(-) >> >> diff --git a/include/net/act_api.h b/include/net/act_api.h >> index 2bf3092ae7ec..312f0f6554a0 100644 >> --- a/include/net/act_api.h >> +++ b/include/net/act_api.h >> @@ -185,7 +185,7 @@ 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, >> struct nlattr *est, char *name, int ovr, int bind, >> - struct tc_action *actions[], size_t *attr_size, >> + struct tc_action *actions[], int init_res[], size_t *attr_size, >> bool rtnl_held, struct netlink_ext_ack *extack); >> struct tc_action_ops *tc_action_load_ops(char *name, struct nlattr *nla, >> bool rtnl_held, >> @@ -193,7 +193,8 @@ struct tc_action_ops *tc_action_load_ops(char *name, struct nlattr *nla, >> struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, >> struct nlattr *nla, struct nlattr *est, >> char *name, int ovr, int bind, >> - struct tc_action_ops *ops, bool rtnl_held, >> + struct tc_action_ops *a_o, int *init_res, >> + bool rtnl_held, >> struct netlink_ext_ack *extack); >> int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[], int bind, >> int ref, bool terse); >> diff --git a/net/sched/act_api.c b/net/sched/act_api.c >> index b919826939e0..eb20a75796d5 100644 >> --- a/net/sched/act_api.c >> +++ b/net/sched/act_api.c >> @@ -777,8 +777,11 @@ static int tcf_action_put(struct tc_action *p) >> return __tcf_action_put(p, false); >> } >> >> -/* Put all actions in this array, skip those NULL's. */ >> -static void tcf_action_put_many(struct tc_action *actions[]) >> +/* Put all actions in this array, skip those NULL's. If cond array is provided >> + * by caller, then only put actions that match. >> + */ >> +static void tcf_action_put_many(struct tc_action *actions[], int *cond, >> + int match) >> { >> int i; >> >> @@ -786,7 +789,7 @@ static void tcf_action_put_many(struct tc_action *actions[]) >> struct tc_action *a = actions[i]; >> const struct tc_action_ops *ops; >> >> - if (!a) >> + if (!a || (cond && cond[i] != match)) > > This looks a bit odd. How about passing an array of action pointers which > only contains those that need to be put? I wanted to make it extensible with cond array instead of make every user manually filter the action array before calling tcf_action_put_many(). But I guess there is currently no need for that and just extending tcf_action_add() with a loop to zero-out the pointers for newly created actions will be clearer. Will change it in V2. > > Thanks.
diff --git a/include/net/act_api.h b/include/net/act_api.h index 2bf3092ae7ec..312f0f6554a0 100644 --- a/include/net/act_api.h +++ b/include/net/act_api.h @@ -185,7 +185,7 @@ 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, struct nlattr *est, char *name, int ovr, int bind, - struct tc_action *actions[], size_t *attr_size, + struct tc_action *actions[], int init_res[], size_t *attr_size, bool rtnl_held, struct netlink_ext_ack *extack); struct tc_action_ops *tc_action_load_ops(char *name, struct nlattr *nla, bool rtnl_held, @@ -193,7 +193,8 @@ struct tc_action_ops *tc_action_load_ops(char *name, struct nlattr *nla, struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, struct nlattr *nla, struct nlattr *est, char *name, int ovr, int bind, - struct tc_action_ops *ops, bool rtnl_held, + struct tc_action_ops *a_o, int *init_res, + bool rtnl_held, struct netlink_ext_ack *extack); int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[], int bind, int ref, bool terse); diff --git a/net/sched/act_api.c b/net/sched/act_api.c index b919826939e0..eb20a75796d5 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -777,8 +777,11 @@ static int tcf_action_put(struct tc_action *p) return __tcf_action_put(p, false); } -/* Put all actions in this array, skip those NULL's. */ -static void tcf_action_put_many(struct tc_action *actions[]) +/* Put all actions in this array, skip those NULL's. If cond array is provided + * by caller, then only put actions that match. + */ +static void tcf_action_put_many(struct tc_action *actions[], int *cond, + int match) { int i; @@ -786,7 +789,7 @@ static void tcf_action_put_many(struct tc_action *actions[]) struct tc_action *a = actions[i]; const struct tc_action_ops *ops; - if (!a) + if (!a || (cond && cond[i] != match)) continue; ops = a->ops; if (tcf_action_put(a)) @@ -992,7 +995,8 @@ struct tc_action_ops *tc_action_load_ops(char *name, struct nlattr *nla, struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, struct nlattr *nla, struct nlattr *est, char *name, int ovr, int bind, - struct tc_action_ops *a_o, bool rtnl_held, + struct tc_action_ops *a_o, int *init_res, + bool rtnl_held, struct netlink_ext_ack *extack) { struct nla_bitfield32 flags = { 0, 0 }; @@ -1028,6 +1032,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, } if (err < 0) goto err_out; + *init_res = err; if (!name && tb[TCA_ACT_COOKIE]) tcf_set_action_cookie(&a->act_cookie, cookie); @@ -1056,7 +1061,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla, struct nlattr *est, char *name, int ovr, int bind, - struct tc_action *actions[], size_t *attr_size, + struct tc_action *actions[], int init_res[], size_t *attr_size, bool rtnl_held, struct netlink_ext_ack *extack) { struct tc_action_ops *ops[TCA_ACT_MAX_PRIO] = {}; @@ -1084,7 +1089,8 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla, for (i = 1; i <= TCA_ACT_MAX_PRIO && tb[i]; i++) { act = tcf_action_init_1(net, tp, tb[i], est, name, ovr, bind, - ops[i - 1], rtnl_held, extack); + ops[i - 1], &init_res[i - 1], rtnl_held, + extack); if (IS_ERR(act)) { err = PTR_ERR(act); goto err; @@ -1462,7 +1468,7 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n, return 0; } err: - tcf_action_put_many(actions); + tcf_action_put_many(actions, NULL, 0); return ret; } @@ -1499,10 +1505,11 @@ static int tcf_action_add(struct net *net, struct nlattr *nla, size_t attr_size = 0; int loop, ret; struct tc_action *actions[TCA_ACT_MAX_PRIO] = {}; + int init_res[TCA_ACT_MAX_PRIO] = {}; for (loop = 0; loop < 10; loop++) { ret = tcf_action_init(net, NULL, nla, NULL, NULL, ovr, 0, - actions, &attr_size, true, extack); + actions, init_res, &attr_size, true, extack); if (ret != -EAGAIN) break; } @@ -1510,8 +1517,8 @@ static int tcf_action_add(struct net *net, struct nlattr *nla, if (ret < 0) return ret; ret = tcf_add_notify(net, n, actions, portid, attr_size, extack); - if (ovr) - tcf_action_put_many(actions); + /* Only put existing actions that were changed by init (res==0). */ + tcf_action_put_many(actions, init_res, 0); return ret; } diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index d3db70865d66..f7425bb9fc3d 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -3040,6 +3040,7 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb, { #ifdef CONFIG_NET_CLS_ACT { + int init_res[TCA_ACT_MAX_PRIO] = {}; struct tc_action *act; size_t attr_size = 0; @@ -3051,8 +3052,8 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb, return PTR_ERR(a_o); act = tcf_action_init_1(net, tp, tb[exts->police], rate_tlv, "police", ovr, - TCA_ACT_BIND, a_o, rtnl_held, - extack); + TCA_ACT_BIND, a_o, init_res, + rtnl_held, extack); if (IS_ERR(act)) { module_put(a_o->owner); return PTR_ERR(act); @@ -3067,8 +3068,8 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb, err = tcf_action_init(net, tp, tb[exts->action], rate_tlv, NULL, ovr, TCA_ACT_BIND, - exts->actions, &attr_size, - rtnl_held, extack); + exts->actions, init_res, + &attr_size, rtnl_held, extack); if (err < 0) return err; exts->nr_actions = err;
Action init code increments reference counter when it changes an action. This is the desired behavior for cls API which needs to obtain action reference for every classifier that points to action. However, act API just needs to change the action and releases the reference before returning. This sequence breaks when the requested action doesn't exist, which causes act API init code to create new action with specified index, but action is still released before returning and is deleted (unless it was referenced concurrently by cls API). Fixes: cae422f379f3 ("net: sched: use reference counting action init") Reported-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> Signed-off-by: Vlad Buslov <vladbu@nvidia.com> --- include/net/act_api.h | 5 +++-- net/sched/act_api.c | 27 +++++++++++++++++---------- net/sched/cls_api.c | 9 +++++---- 3 files changed, 25 insertions(+), 16 deletions(-)