[v2,net,2/2] net sched actions: decrement module refcount earlier

> On April 19, 2017 at 8:23 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> 
> 
> On Tue, Apr 18, 2017 at 7:21 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > Indeed. Allocate the cookie before init? That way, we fail early
> > and dont need to worry about restoring anything.
> 
> No, a->act_cookie needs an action pointer first. ;)
> 
> > In the case of a replace, do you really want to call tcf_hash_release?
> >
> 
> Good point. Probably no, we already call it inside ->init().

Right, doing this (before your change, and after patching tc to allow
sending overlong cookies):

    # tc actions add action ok index 500
    # tc actions change action ok index 500 cookie 112233445566778899aabb
    RTNETLINK answers: Invalid argument
    We have an error talking to the kernel

results in the action disappearing (`tc actions ls action gact` is empty)

And doing this in a loop keeps bumping the module refcount:

    # lsmod |grep act_gact
    act_gact               16384  8023

If I guard the tcf_hash_release() the way you suggested the action
doesn't disappear with the `change` command, and the module refcount
doesn't grow either.

Tested with the adapted version below (since we still need a second
variable due to err changing):

On 17-04-19 04:09 AM, Wolfgang Bumiller wrote:
>

> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 8cc883c063f0..c7e5e437b847 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -555,6 +555,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
>  	struct nlattr *tb[TCA_ACT_MAX + 1];
>  	struct nlattr *kind;
>  	int err;
> +	bool created;
>
>  	if (name == NULL) {
>  		err = nla_parse_nested(tb, TCA_ACT_MAX, nla, NULL);
> @@ -603,20 +604,19 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
>  		err = a_o->init(net, nla, est, &a, ovr, bind);
>  	if (err < 0)
>  		goto err_mod;
> +	created = err == ACT_P_CREATED;
>
>  	if (name == NULL && tb[TCA_ACT_COOKIE]) {
>  		int cklen = nla_len(tb[TCA_ACT_COOKIE]);
>
>  		if (cklen > TC_COOKIE_MAX_SIZE) {
>  			err = -EINVAL;
> -			tcf_hash_release(a, bind);
> -			goto err_mod;
> +			goto err_release;
>  		}
>
>  		if (nla_memdup_cookie(a, tb) < 0) {
>  			err = -ENOMEM;
> -			tcf_hash_release(a, bind);
> -			goto err_mod;
> +			goto err_release;
>  		}
>  	}
>
> @@ -624,11 +624,14 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
>  	 * if it exists and is only bound to in a_o->init() then
>  	 * ACT_P_CREATED is not returned (a zero is).
>  	 */
> -	if (err != ACT_P_CREATED)
> +	if (!created)
>  		module_put(a_o->owner);
>
>  	return a;
>
> +err_release:
> +	if (created)
> +		tcf_hash_release(a, bind);
>  err_mod:
>  	module_put(a_o->owner);
>  err_out:
>

This solves one issue, but I am afraid the issue Cong mentioned is a
possibility still.
Lets say user did a replace and tried to also replace the cookie
in that transaction. The init() succeeds but the cookie allocation
fails. To be correct we'll have to undo the replace i.e something
like uninit() which will restore back the old values.
This is very complex and unnecessary.

My suggestion:
If we move the cookie allocation before init we can save it and
only when init succeeds do we attach it to the action, otherwise
we free it on error path.

cheers,
jamal

On Wed, Apr 19, 2017 at 4:32 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> This solves one issue, but I am afraid the issue Cong mentioned is a
> possibility still.
> Lets say user did a replace and tried to also replace the cookie
> in that transaction. The init() succeeds but the cookie allocation
> fails. To be correct we'll have to undo the replace i.e something
> like uninit() which will restore back the old values.
> This is very complex and unnecessary.

It is not complex once we move to RCU completely, replacement
is merely a pointer assignment, rollback is same.

It is necessary too according to the rules of RCU, as I said before, the
current code is broken, we can't modify an existing action with RCU,
we have to make a copy. I do have plan to make actions truly RCU,
but I have to redesign the action API's first.

On 17-04-19 01:25 PM, Cong Wang wrote:
> On Wed, Apr 19, 2017 at 4:32 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> This solves one issue, but I am afraid the issue Cong mentioned is a
>> possibility still.
>> Lets say user did a replace and tried to also replace the cookie
>> in that transaction. The init() succeeds but the cookie allocation
>> fails. To be correct we'll have to undo the replace i.e something
>> like uninit() which will restore back the old values.
>> This is very complex and unnecessary.
>
> It is not complex once we move to RCU completely, replacement
> is merely a pointer assignment, rollback is same.
>

Nice to hear.

> It is necessary too according to the rules of RCU, as I said before, the
> current code is broken, we can't modify an existing action with RCU,
> we have to make a copy. I do have plan to make actions truly RCU,
> but I have to redesign the action API's first.

Ok ;->
Lucas: Lets work to get those test suites in.

cheers,
jamal