netdev - Re: [PATCH net 1/1] net sched actions: fix refcnt when GETing of action after bind

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: <7a55a129-1e96-80f7-373d-1ff6a95b5269@mojatatu.com>
Date:   Wed, 18 Jan 2017 06:33:52 -0500
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 net 1/1] net sched actions: fix refcnt when GETing of
 action after bind

On 17-01-17 01:17 PM, Cong Wang wrote:
> On Sun, Jan 15, 2017 at 7:14 AM, Jamal Hadi Salim <jhs@...atatu.com> wrote:
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index 2095c83..e10456ef6f 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -900,8 +900,6 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
>>                         goto err;
>>                 }
>>                 act->order = i;
>> -               if (event == RTM_GETACTION)
>> -                       act->tcfa_refcnt++;
>>                 list_add_tail(&act->list, &actions);
>>         }
>>
>> @@ -914,7 +912,8 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
>>                 return ret;
>>         }
>>  err:
>> -       tcf_action_destroy(&actions, 0);
>> +       if (event != RTM_GETACTION)
>> +               tcf_action_destroy(&actions, 0);
>
> Why this check for RTM_GETACTION? It does not make sense
> at least for the error case, that is, when tcf_action_get_1() fails
> in the middle of the loop, all the previous ones should be destroyed
> no matter it's GETACTION or DELACTION...
>
> Also, for the non-error case of GETACTION, they should be
> destroyed too after dumping to user-space, otherwise there is no
> way to use them since 'actions' is local to that function.
>
> I thought in commit aecc5cefc389 you grab that refcnt on purpose.
>

I did.
The issue there (after your original patch) was destroy() would
decrement the refcount to zero and a GET was essentially translated
to a DEL. Incrementing the refcount earlier protected against that
assuming destroy was going to decrement it.
However, when an action is bound the destroy() doesnt decrement
the refcnt. So the refcnt keeps going up forever (and therefore
deleting fails in the future). So we cant use destroy() as is.

We need another function to cover the scenario you are
describing. I had thought of using cleanup_a() for GET and leave
the refcount alone. But that is insufficient incase the module
refcounts went up. Maybe it is better to refactor get/del
to have different code paths (instead of the clever code reuse).


cheers,
jamal