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] [day] [month] [year] [list]

Message-ID: <d361fc0a-96e9-0ebb-54cb-596ff5832036@mojatatu.com>
Date:   Sun, 22 Jan 2017 07:56:20 -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-20 01:20 AM, Cong Wang wrote:
> On Wed, Jan 18, 2017 at 3:33 AM, Jamal Hadi Salim <jhs@...atatu.com> wrote:
>> On 17-01-17 01:17 PM, Cong Wang wrote:
>>>

>> 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.
>
> Hmm, tcf_action_destroy() should not touch the refcnt at all in this case,
> right? Since the refcnt here is not for readers in kernel code but for
> user-space. We mix the use of this refcnt, which leads to problems.
>
> Your patch is not correct either for DEL, tcf_action_destroy() is not
> needed to call again after tcf_del_notify() fails, right? Probably
> it is not needed at all:

Cong,
Please proceed to separate del from get. The trickery is biting us.
Also - run those tests i had in my patch. There is a difference
between the bound vs not-bound use cases.

cheers,
jamal