netdev - Re: [PATCH linux 2/2] net sched actions: fix refcount decrement on error

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]

Date:   Sun, 16 Apr 2017 10:30:04 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Wolfgang Bumiller <w.bumiller@...xmox.com>
Cc:     Cong Wang <xiyou.wangcong@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Roman Mashak <mrv@...atatu.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH linux 2/2] net sched actions: fix refcount decrement on
 error

Wolfgang, can you _please_ stop cc-ing lkml? Everybody you need to deal
with is on netdev.

On 17-04-15 02:48 PM, Wolfgang Bumiller wrote:
>

>>
>>> So I'm not sure exactly how the module and tc_action counts are related
>>> (and I usually like to understand my own patches ;-) ).
>>
>>

Every time we add an action, refcnt goes up. Every time we bind an
action to a filter the refcnt + bindcnt goes up.
Reverse for everytime we delete a filter or action. The action does
not get destroyed because the filter was deleted.
Everytime the action gets its refcount bumped up (for either of the
two reasons described above), the module ref needs incrementing.
Everytime the action refcnt goes down the module ref needs to be
decremented.

Only when both refcnt and bind cnt go to at least zero do we really
delete/destroy.

>> Each action holds a refcnt to its module, each filter holds a refcnt to
>> its bound or referenced (unbound) action.
>>
>>
>>> Maybe I'm missing something obvious but I'm currently a bit confused as
>>> to whether the tcf_hash_release() call there is okay, or should have its
>>> return value checked or should depend on ->init()'s ACT_P_CREATED value
>>> as well?
>>>
>>
>> I think it's the same? If we have ACT_P_CREATED here, tcf_hash_release()
>> will return ACT_P_DELETED for sure because the newly created action has
>> refcnt==1?
>
> Makes sense on the one hand, but for ACT_P_DELETED both ref and bind
> count need to reach 0, so I'm still concerned that the different behaviors
> I mentioned above might be problematic if we use ACT_P_CREATED only.
> (It also means my patches still leak a count - which is probably still
> better than the previous underflow, but ultimately doesn't satisfy me.)
> Should I still resend it this way for the record with the Acked-bys?
> (Since given the fact that with unprivileged containers it's possible to
> trigger this access and potentially crash the kernel I strongly feel that
> some version of this should end up in the 4.11 release.)
>

I think that is an unrelated code path, current issue is on the
create/replace code path. We need to separate delete from get - we
just havent got around to it yet. Followup patches will make sense.
Agree, this change should go in quickly..

Actually - come to think of it: You should probably leave the module
put to the end instead of the beginning because it protects against
another entry unloading the module while we are still processing.

Just post the patch - we'll help reviewing it.

cheers,
jamal