On Wed, Mar 31, 2021 at 9:41 AM Vlad Buslov <vla...@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 <mem...@gmail.com>
> Signed-off-by: Vlad Buslov <vla...@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.
