netdev - Re: [PATCH v3 net 1/1] net sched actions: fix GETing actions

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 for Android: free password hash cracker in your pocket

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

Message-ID: <f24e1121-84c8-01d1-23cd-1288ba1402aa@mojatatu.com>
Date:   Tue, 13 Sep 2016 15:47:15 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH v3 net 1/1] net sched actions: fix GETing actions

On 16-09-13 12:20 PM, Cong Wang wrote:
> On Mon, Sep 12, 2016 at 4:07 PM, Jamal Hadi Salim <jhs@...atatu.com> wrote:
>> From: Jamal Hadi Salim <jhs@...atatu.com>
>>
>> With the batch changes that translated transient actions into
>> a temporary list lost in the translation was the fact that
>> tcf_action_destroy() will eventually delete the action from
>> the permanent location if the refcount is zero.
>>
>> Example of what broke:
>> ...add a gact action to drop
>> sudo $TC actions add action drop index 10
>> ...now retrieve it, looks good
>> sudo $TC actions get action gact index 10
>> ...retrieve it again and find it is gone!
>> sudo $TC actions get action gact index 10
>>
>> Fixes:
>> commit 22dc13c837c3 ("net_sched: convert tcf_exts from list to pointer array"),
>> commit 824a7e8863b3 ("net_sched: remove an unnecessary list_del()")
>> commit f07fed82ad79 ("net_sched: remove the leftover cleanup_a()")
>>
>> Signed-off-by: Jamal Hadi Salim <jhs@...atatu.com>
>> ---
>>  net/sched/act_api.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index d09d068..50720b1 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -592,6 +592,16 @@ err_out:
>>         return ERR_PTR(err);
>>  }
>>
>> +static void cleanup_a(struct list_head *actions, int ovr)
>> +{
>> +       struct tc_action *a;
>> +
>> +       list_for_each_entry(a, actions, list) {
>> +               if (ovr)
>> +                       a->tcfa_refcnt -= 1;
>> +       }
>> +}
>> +
>>  int tcf_action_init(struct net *net, struct nlattr *nla,
>>                                   struct nlattr *est, char *name, int ovr,
>>                                   int bind, struct list_head *actions)
>> @@ -612,8 +622,15 @@ int tcf_action_init(struct net *net, struct nlattr *nla,
>>                         goto err;
>>                 }
>>                 act->order = i;
>> +               if (ovr)
>> +                       act->tcfa_refcnt += 1;
>>                 list_add_tail(&act->list, actions);
>>         }
>> +
>> +       /* Remove the temp refcnt which was necessary to protect against
>> +        * destroying an existing action which was being replaced
>> +        */
>> +       cleanup_a(actions, ovr);
>>         return 0;
>
> I am still trying to understand this piece, so here you hold the refcnt
> for the same action used by the later iteration? Otherwise there is
> almost none user inbetween hold and release...
>
> The comment you add is not clear to me, we use RTNL/RCU to
> sync destroy and replace, so how could that happen?
>

I was worried about the destroy() hitting an error in that function.
If an action already existed and all we asked for was to
replace some attribute it would be deleted. It was the way the code was
before your changes so i just restored it to its original form.

cheers,
jamal