diff mbox series

[v2,1/1] net: sched: bump refcount for new action in ACT replace mode

Message ID 20210329225322.143135-1-memxor@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [v2,1/1] net: sched: bump refcount for new action in ACT replace mode | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Guessed tree name to be net-next
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cc_maintainers fail 2 blamed authors not CCed: vladbu@mellanox.com marcelo.leitner@gmail.com; 11 maintainers not CCed: yhs@fb.com marcelo.leitner@gmail.com kpsingh@kernel.org daniel@iogearbox.net andrii@kernel.org bpf@vger.kernel.org kafai@fb.com ast@kernel.org john.fastabend@gmail.com songliubraving@fb.com vladbu@mellanox.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 1 this patch: 1
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 9 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 1 this patch: 1
netdev/header_inline success Link

Commit Message

Kumar Kartikeya Dwivedi March 29, 2021, 10:53 p.m. UTC
Currently, action creation using ACT API in replace mode is buggy.
When invoking for non-existent action index 42,

	tc action replace action bpf obj foo.o sec <xyz> index 42

kernel creates the action, fills up the netlink response, and then just
deletes the action after notifying userspace.

	tc action show action bpf

doesn't list the action.

This happens due to the following sequence when ovr = 1 (replace mode)
is enabled:

tcf_idr_check_alloc is used to atomically check and either obtain
reference for existing action at index, or reserve the index slot using
a dummy entry (ERR_PTR(-EBUSY)).

This is necessary as pointers to these actions will be held after
dropping the idrinfo lock, so bumping the reference count is necessary
as we need to insert the actions, and notify userspace by dumping their
attributes. Finally, we drop the reference we took using the
tcf_action_put_many call in tcf_action_add. However, for the case where
a new action is created due to free index, its refcount remains one.
This when paired with the put_many call leads to the kernel setting up
the action, notifying userspace of its creation, and then tearing it
down. For existing actions, the refcount is still held so they remain
unaffected.

Fortunately due to rtnl_lock serialization requirement, such an action
with refcount == 1 will not be concurrently deleted by anything else, at
best CLS API can move its refcount up and down by binding to it after it
has been published from tcf_idr_insert_many. Since refcount is atleast
one until put_many call, CLS API cannot delete it. Also __tcf_action_put
release path already ensures deterministic outcome (either new action
will be created or existing action will be reused in case CLS API tries
to bind to action concurrently) due to idr lock serialization.

We fix this by making refcount of newly created actions as 2 in ACT API
replace mode. A relaxed store will suffice as visibility is ensured only
after the tcf_idr_insert_many call.

Note that in case of creation or overwriting using CLS API only (i.e.
bind = 1), overwriting existing action object is not allowed, and any
such request is silently ignored (without error).

The refcount bump that occurs in tcf_idr_check_alloc call there for
existing action will pair with tcf_exts_destroy call made from the
owner module for the same action. In case of action creation, there
is no existing action, so no tcf_exts_destroy callback happens.

This means no code changes for CLS API.

Fixes: cae422f379f3 ("net: sched: use reference counting action init")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
Changelog:

v1 -> v2
Remove erroneous tcf_action_put_many call in tcf_exts_validate (Vlad)
Isolate refcount bump to ACT API in replace mode
---
 net/sched/act_api.c | 3 +++
 1 file changed, 3 insertions(+)

--
2.30.2

Comments

Vlad Buslov March 30, 2021, 10:32 a.m. UTC | #1
On Tue 30 Mar 2021 at 01:53, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> Currently, action creation using ACT API in replace mode is buggy.
> When invoking for non-existent action index 42,
>
> 	tc action replace action bpf obj foo.o sec <xyz> index 42
>
> kernel creates the action, fills up the netlink response, and then just
> deletes the action after notifying userspace.
>
> 	tc action show action bpf
>
> doesn't list the action.

Okay, I understand the issue now. I'll also add a tdc test for it.

>
> This happens due to the following sequence when ovr = 1 (replace mode)
> is enabled:
>
> tcf_idr_check_alloc is used to atomically check and either obtain
> reference for existing action at index, or reserve the index slot using
> a dummy entry (ERR_PTR(-EBUSY)).
>
> This is necessary as pointers to these actions will be held after
> dropping the idrinfo lock, so bumping the reference count is necessary
> as we need to insert the actions, and notify userspace by dumping their
> attributes. Finally, we drop the reference we took using the
> tcf_action_put_many call in tcf_action_add. However, for the case where
> a new action is created due to free index, its refcount remains one.
> This when paired with the put_many call leads to the kernel setting up
> the action, notifying userspace of its creation, and then tearing it
> down. For existing actions, the refcount is still held so they remain
> unaffected.
>
> Fortunately due to rtnl_lock serialization requirement, such an action
> with refcount == 1 will not be concurrently deleted by anything else, at
> best CLS API can move its refcount up and down by binding to it after it
> has been published from tcf_idr_insert_many. Since refcount is atleast
> one until put_many call, CLS API cannot delete it. Also __tcf_action_put
> release path already ensures deterministic outcome (either new action
> will be created or existing action will be reused in case CLS API tries
> to bind to action concurrently) due to idr lock serialization.
>
> We fix this by making refcount of newly created actions as 2 in ACT API
> replace mode. A relaxed store will suffice as visibility is ensured only
> after the tcf_idr_insert_many call.
>
> Note that in case of creation or overwriting using CLS API only (i.e.
> bind = 1), overwriting existing action object is not allowed, and any
> such request is silently ignored (without error).
>
> The refcount bump that occurs in tcf_idr_check_alloc call there for
> existing action will pair with tcf_exts_destroy call made from the
> owner module for the same action. In case of action creation, there
> is no existing action, so no tcf_exts_destroy callback happens.
>
> This means no code changes for CLS API.
>
> Fixes: cae422f379f3 ("net: sched: use reference counting action init")
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
> Changelog:
>
> v1 -> v2
> Remove erroneous tcf_action_put_many call in tcf_exts_validate (Vlad)
> Isolate refcount bump to ACT API in replace mode
> ---
>  net/sched/act_api.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index b919826939e0..43cceb924976 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1042,6 +1042,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>  	if (err != ACT_P_CREATED)
>  		module_put(a_o->owner);
>
> +	if (!bind && ovr && err == ACT_P_CREATED)
> +		refcount_set(&a->tcfa_refcnt, 2);
> +
>  	return a;
>
>  err_out:

Reviewed-and-tested-by: Vlad Buslov <vladbu@nvidia.com>
Cong Wang March 31, 2021, 4:40 a.m. UTC | #2
On Mon, Mar 29, 2021 at 3:55 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index b919826939e0..43cceb924976 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1042,6 +1042,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>         if (err != ACT_P_CREATED)
>                 module_put(a_o->owner);
>
> +       if (!bind && ovr && err == ACT_P_CREATED)
> +               refcount_set(&a->tcfa_refcnt, 2);
> +

Hmm, if we set the refcnt to 2 here, how could tcf_action_destroy()
destroy them when we rollback from a failure in the middle of the loop
in tcf_action_init()?

Thanks.
Vlad Buslov March 31, 2021, 7:59 a.m. UTC | #3
On Wed 31 Mar 2021 at 07:40, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Mar 29, 2021 at 3:55 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index b919826939e0..43cceb924976 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -1042,6 +1042,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>>         if (err != ACT_P_CREATED)
>>                 module_put(a_o->owner);
>>
>> +       if (!bind && ovr && err == ACT_P_CREATED)
>> +               refcount_set(&a->tcfa_refcnt, 2);
>> +
>
> Hmm, if we set the refcnt to 2 here, how could tcf_action_destroy()
> destroy them when we rollback from a failure in the middle of the loop
> in tcf_action_init()?
>
> Thanks.

Hmm, you might be right. Also, the error handling code in
tcf_action_init() looks incorrect:

err:
	tcf_action_destroy(actions, bind);
err_mod:
	for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
		if (ops[i])
			module_put(ops[i]->owner);
	}
	return err;

It looks like here the modules for all actions that successfully
completed their init has already been release by either
tcf_action_init_1() on action overwrite or by tcf_action_destroy() on
action create. I'll try to come up with tests that validate these corner
cases.

Regards,
Vlad
Kumar Kartikeya Dwivedi March 31, 2021, 8:38 a.m. UTC | #4
On Wed, Mar 31, 2021 at 10:10:45AM IST, Cong Wang wrote:
> On Mon, Mar 29, 2021 at 3:55 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> > diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> > index b919826939e0..43cceb924976 100644
> > --- a/net/sched/act_api.c
> > +++ b/net/sched/act_api.c
> > @@ -1042,6 +1042,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> >         if (err != ACT_P_CREATED)
> >                 module_put(a_o->owner);
> >
> > +       if (!bind && ovr && err == ACT_P_CREATED)
> > +               refcount_set(&a->tcfa_refcnt, 2);
> > +
>
> Hmm, if we set the refcnt to 2 here, how could tcf_action_destroy()
> destroy them when we rollback from a failure in the middle of the loop
> in tcf_action_init()?
>

You are right, it wouldn't. I sent a new version with a fix. PTAL.

> Thanks.

--
Kartikeya
diff mbox series

Patch

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index b919826939e0..43cceb924976 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1042,6 +1042,9 @@  struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	if (err != ACT_P_CREATED)
 		module_put(a_o->owner);

+	if (!bind && ovr && err == ACT_P_CREATED)
+		refcount_set(&a->tcfa_refcnt, 2);
+
 	return a;

 err_out: