[<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
Powered by blists - more mailing lists