Re: [PATCH net-next v3] net: sched: bump refcount for new action in ACT replace mode

[Vlad Buslov]

On Wed 31 Mar 2021 at 11:37, Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> 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 while notifying userspace of success.
>
> 	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.
>
> We also remember the new actions that we took an additional reference on,
> and relinquish the temporal reference during rollback on failure.
>
> 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@xxxxxxxxx>
> ---
> Changelog:
>
> v2 -> v3
> Cleanup new action on rollback after raising refcount (Cong)
>
> v1 -> v2
> Remove erroneous tcf_action_put_many call in tcf_exts_validate (Vlad)
> Isolate refcount bump to ACT API in replace mode
> ---
>  include/net/act_api.h |  2 +-
>  net/sched/act_api.c   | 24 ++++++++++++++++++++++--
>  net/sched/cls_api.c   |  2 +-
>  3 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index 2bf3092ae7ec..a01ff5fa641e 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -194,7 +194,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>  				    struct nlattr *nla, struct nlattr *est,
>  				    char *name, int ovr, int bind,
>  				    struct tc_action_ops *ops, bool rtnl_held,
> -				    struct netlink_ext_ack *extack);
> +				    struct netlink_ext_ack *extack, bool *ref);
>  int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[], int bind,
>  		    int ref, bool terse);
>  int tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int, int);
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index b919826939e0..718bc197b9a7 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -993,7 +993,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
>  				    struct nlattr *nla, struct nlattr *est,
>  				    char *name, int ovr, int bind,
>  				    struct tc_action_ops *a_o, bool rtnl_held,
> -				    struct netlink_ext_ack *extack)
> +				    struct netlink_ext_ack *extack, bool *ref)
>  {
>  	struct nla_bitfield32 flags = { 0, 0 };
>  	u8 hw_stats = TCA_ACT_HW_STATS_ANY;
> @@ -1042,6 +1042,13 @@ 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) {
> +		if (ref)
> +			*ref = true;
> +
> +		refcount_set(&a->tcfa_refcnt, 2);
> +	}
> +

I wonder if we are being too clever here with all the refcnt
manipulations. Maybe it is better to just expose to the caller whether
existing action has been overwritten or a new one has been created and
let the user decide if they want to keep the reference? I'll send a RFC
to gather feedback from maintainers.

>  	return a;
>
>  err_out:
> @@ -1062,10 +1069,13 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
>  	struct tc_action_ops *ops[TCA_ACT_MAX_PRIO] = {};
>  	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
>  	struct tc_action *act;
> +	u32 new_actions = 0;
>  	size_t sz = 0;
>  	int err;
>  	int i;
>
> +	BUILD_BUG_ON(TCA_ACT_MAX_PRIO > sizeof(new_actions) * 8);
> +
>  	err = nla_parse_nested_deprecated(tb, TCA_ACT_MAX_PRIO, nla, NULL,
>  					  extack);
>  	if (err < 0)
> @@ -1083,8 +1093,9 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
>  	}
>
>  	for (i = 1; i <= TCA_ACT_MAX_PRIO && tb[i]; i++) {
> +		bool ovr_new = false;
>  		act = tcf_action_init_1(net, tp, tb[i], est, name, ovr, bind,
> -					ops[i - 1], rtnl_held, extack);
> +					ops[i - 1], rtnl_held, extack, &ovr_new);
>  		if (IS_ERR(act)) {
>  			err = PTR_ERR(act);
>  			goto err;
> @@ -1092,6 +1103,10 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
>  		sz += tcf_action_fill_size(act);
>  		/* Start from index 0 */
>  		actions[i - 1] = act;
> +
> +		/* remember new actions that we take a reference on */
> +		if (ovr_new)
> +			new_actions |= 1UL << (i - 1);
>  	}
>
>  	/* We have to commit them all together, because if any error happened in
> @@ -1103,6 +1118,11 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
>  	return i - 1;
>
>  err:
> +	/* reset the reference back to 1 in case of error */
> +	for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) {
> +		if (new_actions & (1UL << i))
> +			refcount_set(&actions[i]->tcfa_refcnt, 1);
> +	}
>  	tcf_action_destroy(actions, bind);
>  err_mod:
>  	for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index d3db70865d66..4f4a7355b1e1 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3052,7 +3052,7 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
>  			act = tcf_action_init_1(net, tp, tb[exts->police],
>  						rate_tlv, "police", ovr,
>  						TCA_ACT_BIND, a_o, rtnl_held,
> -						extack);
> +						extack, NULL);
>  			if (IS_ERR(act)) {
>  				module_put(a_o->owner);
>  				return PTR_ERR(act);