netdev - Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier

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:   Tue, 18 Apr 2017 22:21:12 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Cong Wang <xiyou.wangcong@...il.com>,
        Wolfgang Bumiller <w.bumiller@...xmox.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount
 earlier

On 17-04-18 01:03 PM, Cong Wang wrote:
> On Tue, Apr 18, 2017 at 3:13 AM, Wolfgang Bumiller
> <w.bumiller@...xmox.com> wrote:

> police action...That is why I said we may need a TCA_POLICE_COOKIE.
>

Unless it is very old user space code (which wouldnt know what a
cookie is), dont think there's much use of direct policer access.

>> I'm thinking the first patch should be enough. (I've tested forcing the
>> other filters into the error path *without* this patch and couldn't
>> produce crashes or reference count problems (while with this patch
>> applied it was leaking reference counts on creation (which makes sense
>> considering tcf_hash_release is used and the ACT_P_CREATED case will
>> keep repeating)). (Whereas without both patches simply looking through
>> creating and deleting a policing filter pretty much always resulted in
>> crashes with various different backtraces.)
>>
>
> The action API's suck here.
>
> The idea is we should rollback everything when cookie setup fails.
>
> Taking another look, it seems the current code (without this patch) is
> correct:
>
> 1) When ->init() returns ACT_P_CREATED, we should rollback both
> act creation and module refcnt, the former is already taken care by
> tcf_hash_release(), the latter is at err_mod.
>
> 2) When ->init() returns !ACT_P_CREATED, we should rollback the
> the modification to the existing action and module refcnt, the former is
> impossible with current code (because we don't do copy and update)
> so we only do tcf_hash_release(), module refcnt needs to rollback
> like normal path.
>

Indeed. Allocate the cookie before init? That way, we fail early
and dont need to worry about restoring anything.
In the case of a replace, do you really want to call tcf_hash_release?

> Ideally, these action API's should handle it nicely, exposing the
> module_put()/module_get() is ugly and confusing.
>

lots of room for improvement.

cheers,
jamal