netdev - Re: [PATCH RFC 1/4] net: sched: fix action overwrite reference counting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM_iQpX85rHgUMvwQskEFqa+MRtv5Y_+ZbMnds9xxmaKP=qiOg@mail.gmail.com>
Date:   Fri, 2 Apr 2021 15:13:02 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Vlad Buslov <vladbu@...dia.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Kumar Kartikeya Dwivedi <memxor@...il.com>,
        David Miller <davem@...emloft.net>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Jakub Kicinski <kuba@...nel.org>,
        Toke Høiland-Jørgensen <toke@...hat.com>
Subject: Re: [PATCH RFC 1/4] net: sched: fix action overwrite reference counting

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