On Thu 08 Apr 2021 at 02:50, Cong Wang [off-list ref] wrote:
On Wed, Apr 7, 2021 at 8:36 AM Vlad Buslov [off-list ref] wrote:
quoted
Action init code increments reference counter when it changes an action.
This is the desired behavior for cls API which needs to obtain action
reference for every classifier that points to action. However, act API just
needs to change the action and releases the reference before returning.
This sequence breaks when the requested action doesn't exist, which causes
act API init code to create new action with specified index, but action is
still released before returning and is deleted (unless it was referenced
concurrently by cls API).
Reproduction:
$ sudo tc actions ls action gact
$ sudo tc actions change action gact drop index 1
$ sudo tc actions ls action gact
I didn't know 'change' could actually create an action when
it does not exist. So it sets NLM_F_REPLACE, how could it
replace a non-existing one? Is this the right behavior or is it too
late to change even if it is not?
Origins of setting ovr based on NLM_F_REPLACE are lost since this code
goes back to Linus' Linux-2.6.12-rc2 commit. Jamal, do you know if this
is the expected behavior or just something unintended?
quoted
Extend tcf_action_init() to accept 'init_res' array and initialize it with
action->ops->init() result. In tcf_action_add() remove pointers to created
actions from actions array before passing it to tcf_action_put_many().
In my last comments, I actually meant whether we can avoid this
'init_res[]' array. Since here you want to check whether an action
returned by tcf_action_init_1() is a new one or an existing one, how
about checking its refcnt? Something like:
act = tcf_action_init_1(...);
if (IS_ERR(act)) {
err = PTR_ERR(act);
goto err;
}
if (refcount_read(&act->tcfa_refcnt) == 1) {
// we know this is a newly allocated one
}
Thanks.
Hmm, I don't think this would work in general case. Consider following
cases:
1. Action existed during init as filter action(refcnt=1), init overwrote
it setting refcnt=2, by the time we got to checking tcfa_refcnt filter
has been deleted (refcnt=1) so code will incorrectly assume that it has
created the action.
2. We need this check in tcf_action_add() to release the refcnt in case
of overwriting existing actions, but by that time actions are already
accessible though idr, so even in case when new action has been created
(refcnt=1) it could already been referenced by concurrently created
filter (refcnt=2).
Regards,
Vlad