netdev - Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <861dba23-1244-ee57-a980-a6dceffa2793@mojatatu.com>
Date:   Wed, 19 Apr 2017 07:32:38 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Wolfgang Bumiller <w.bumiller@...xmox.com>,
        Cong Wang <xiyou.wangcong@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount
 earlier

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