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: <ygnh1rbrbtyp.fsf@nvidia.com>
Date:   Sat, 3 Apr 2021 12:25:34 +0300
From:   Vlad Buslov <vladbu@...dia.com>
To:     Cong Wang <xiyou.wangcong@...il.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 Sat 03 Apr 2021 at 01:13, Cong Wang <xiyou.wangcong@...il.com> wrote:
> 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?

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@...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?

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.