On 17-04-18 01:03 PM, Cong Wang wrote:
On Tue, Apr 18, 2017 at 3:13 AM, Wolfgang Bumiller
<w.bumil...@proxmox.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
