[PATCH v2 net 1/2] net sched actions: fix access to uninitialized data

[PATCH v2 net 1/2] net sched actions: fix access to uninitialized data

[Wolfgang Bumiller]

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
(same as v1)

 net/sched/act_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index b70aa57319ea..8cc883c063f0 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -604,7 +604,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 	if (err < 0)
 		goto err_mod;
 
-	if (tb[TCA_ACT_COOKIE]) {
+	if (name == NULL && tb[TCA_ACT_COOKIE]) {
 		int cklen = nla_len(tb[TCA_ACT_COOKIE]);
 
 		if (cklen > TC_COOKIE_MAX_SIZE) {
-- 
2.11.0

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

[Wolfgang Bumiller]

Whether the reference count has to be decremented depends
on whether the policy was created. If TCA_ACT_COOKIE is
passed and an error occurs there, the same condition still
has to be honored.

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
---

I did not include the Acked-bys here because I've noticed that this
is still wrong. After reading a bit more and doing more tests with
different filters I realized that the `name != NULL` case is specific
to the police filter only. For the other filters this patch here breaks
refcounting in the error case (I included it for reference only).
I'm thinking the first patch should be enough. (I've tested forcing the
other filters into the error path *without* this patch and couldn't
produce crashes or reference count problems (while with this patch
applied it was leaking reference counts on creation (which makes sense
considering tcf_hash_release is used and the ACT_P_CREATED case will
keep repeating)). (Whereas without both patches simply looking through
creating and deleting a policing filter pretty much always resulted in
crashes with various different backtraces.)

(I'd still like to clarify the comment in the code btw.)

 net/sched/act_api.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 8cc883c063f0..4493f1b9d22b 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -604,28 +604,30 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 	if (err < 0)
 		goto err_mod;
 
+	/* The module count should only go up when a brand new policy was
+	 * created, if it exists and is only bound to in a_o->init() then
+	 * ACT_P_CREATED is not returned (a zero is), and we need to roll back
+	 * the bump caused by tc_lookup_action_n().
+	 */
+	if (err != ACT_P_CREATED)
+		module_put(a_o->owner);
+
 	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_out;
 		}
 
 		if (nla_memdup_cookie(a, tb) < 0) {
 			err = -ENOMEM;
 			tcf_hash_release(a, bind);
-			goto err_mod;
+			goto err_out;
 		}
 	}
 
-	/* module count goes up only when brand new policy is created
-	 * 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)
-		module_put(a_o->owner);
 
 	return a;
 
-- 
2.11.0

Re: [PATCH v2 net 1/2] net sched actions: fix access to uninitialized data

[Jamal Hadi Salim]

On 17-04-18 06:13 AM, Wolfgang Bumiller wrote:
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
> (same as v1)
>
>  net/sched/act_api.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index b70aa57319ea..8cc883c063f0 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -604,7 +604,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
>  	if (err < 0)
>  		goto err_mod;
>
> -	if (tb[TCA_ACT_COOKIE]) {
> +	if (name == NULL && tb[TCA_ACT_COOKIE]) {
>  		int cklen = nla_len(tb[TCA_ACT_COOKIE]);
>
>  		if (cklen > TC_COOKIE_MAX_SIZE) {
>

This one looks good.

cheers,
jamal

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

[Jamal Hadi Salim]

On 17-04-18 06:13 AM, Wolfgang Bumiller wrote:
> Whether the reference count has to be decremented depends
> on whether the policy was created. If TCA_ACT_COOKIE is
> passed and an error occurs there, the same condition still
> has to be honored.
>
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
>
> I did not include the Acked-bys here because I've noticed that this
> is still wrong. After reading a bit more and doing more tests with
> different filters I realized that the `name != NULL` case is specific
> to the police filter only. For the other filters this patch here breaks
> refcounting in the error case (I included it for reference only).
> I'm thinking the first patch should be enough. (I've tested forcing the
> other filters into the error path *without* this patch and couldn't
> produce crashes or reference count problems (while with this patch
> applied it was leaking reference counts on creation (which makes sense
> considering tcf_hash_release is used and the ACT_P_CREATED case will
> keep repeating)). (Whereas without both patches simply looking through
> creating and deleting a policing filter pretty much always resulted in
> crashes with various different backtraces.)
>

The error path for cookie failure still needs handling.
In retrospect:
I think you should leave the module_put() to the end,
it protects any unloading while the action is being
processed.
It may be worth remembering a_o->init() results and
conditionally calling tcf_hash_release() based on whether
a creation happened or not. I think your original patch had
a similar idea without the extra variable I am suggesting.

cheers,
jamal

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

[Cong Wang]

On Tue, Apr 18, 2017 at 3:13 AM, Wolfgang Bumiller
<w.bumiller@proxmox.com> wrote:
> Whether the reference count has to be decremented depends
> on whether the policy was created. If TCA_ACT_COOKIE is
> passed and an error occurs there, the same condition still
> has to be honored.
>
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
>
> I did not include the Acked-bys here because I've noticed that this
> is still wrong. After reading a bit more and doing more tests with
> different filters I realized that the `name != NULL` case is specific
> to the police filter only. For the other filters this patch here breaks
> refcounting in the error case (I included it for reference only).


police action...That is why I said we may need a TCA_POLICE_COOKIE.

> I'm thinking the first patch should be enough. (I've tested forcing the
> other filters into the error path *without* this patch and couldn't
> produce crashes or reference count problems (while with this patch
> applied it was leaking reference counts on creation (which makes sense
> considering tcf_hash_release is used and the ACT_P_CREATED case will
> keep repeating)). (Whereas without both patches simply looking through
> creating and deleting a policing filter pretty much always resulted in
> crashes with various different backtraces.)
>

The action API's suck here.

The idea is we should rollback everything when cookie setup fails.

Taking another look, it seems the current code (without this patch) is
correct:

1) When ->init() returns ACT_P_CREATED, we should rollback both
act creation and module refcnt, the former is already taken care by
tcf_hash_release(), the latter is at err_mod.

2) When ->init() returns !ACT_P_CREATED, we should rollback the
the modification to the existing action and module refcnt, the former is
impossible with current code (because we don't do copy and update)
so we only do tcf_hash_release(), module refcnt needs to rollback
like normal path.

Ideally, these action API's should handle it nicely, exposing the
module_put()/module_get() is ugly and confusing.

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

[Jamal Hadi Salim]

On 17-04-18 01:03 PM, Cong Wang wrote:
> On Tue, Apr 18, 2017 at 3:13 AM, Wolfgang Bumiller
> <w.bumiller@proxmox.com> wrote:

> police action...That is why I said we may need a TCA_POLICE_COOKIE.
>

Unless it is very old user space code (which wouldnt know what a
cookie is), dont think there's much use of direct policer access.

>> I'm thinking the first patch should be enough. (I've tested forcing the
>> other filters into the error path *without* this patch and couldn't
>> produce crashes or reference count problems (while with this patch
>> applied it was leaking reference counts on creation (which makes sense
>> considering tcf_hash_release is used and the ACT_P_CREATED case will
>> keep repeating)). (Whereas without both patches simply looking through
>> creating and deleting a policing filter pretty much always resulted in
>> crashes with various different backtraces.)
>>
>
> The action API's suck here.
>
> The idea is we should rollback everything when cookie setup fails.
>
> Taking another look, it seems the current code (without this patch) is
> correct:
>
> 1) When ->init() returns ACT_P_CREATED, we should rollback both
> act creation and module refcnt, the former is already taken care by
> tcf_hash_release(), the latter is at err_mod.
>
> 2) When ->init() returns !ACT_P_CREATED, we should rollback the
> the modification to the existing action and module refcnt, the former is
> impossible with current code (because we don't do copy and update)
> so we only do tcf_hash_release(), module refcnt needs to rollback
> like normal path.
>

Indeed. Allocate the cookie before init? That way, we fail early
and dont need to worry about restoring anything.
In the case of a replace, do you really want to call tcf_hash_release?

> Ideally, these action API's should handle it nicely, exposing the
> module_put()/module_get() is ugly and confusing.
>

lots of room for improvement.

cheers,
jamal

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

[Cong Wang]

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().

Something like this...

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 82b1d48d91cc..29ffc348fe2f 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -609,14 +609,12 @@ struct tc_action *tcf_action_init_1(struct net
*net, struct nlattr *nla,

                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;
                }
        }

@@ -629,6 +627,9 @@ struct tc_action *tcf_action_init_1(struct net
*net, struct nlattr *nla,

        return a;

+err_release:
+       if (err == ACT_P_CREATED)
+               tcf_hash_release(a, bind);
 err_mod:
        module_put(a_o->owner);
 err_out:

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

[Wolfgang Bumiller]

> 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):

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:

Re: [PATCH v2 net 1/2] net sched actions: fix access to uninitialized data

[Daniel Borkmann]

Hi Wolfgang,

On 04/18/2017 12:13 PM, Wolfgang Bumiller wrote:
[...]

Since this patch goes to -net tree and is a bug fix, please
also add a proper analysis into the commit log instead of
just an empty commit message.

A Fixes tag to your bug fix would also be appropriate, so
that this helps tracking follow-ups to the original commit
that introduced the code. For this one, should be:

Fixes: 1045ba77a596 ("net sched actions: Add support for user cookies")

> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Thanks,
Daniel

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

[Jamal Hadi Salim]

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

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

[Wolfgang Bumiller]

> On April 19, 2017 at 1:32 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> 
> 
> On 17-04-19 04:09 AM, Wolfgang Bumiller 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.
> 
> 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.

Shouldn't the old code have freed an old a->act_cookie as well before
replacing it then? nla_memdup_cookie() starts off by assigning a->act_cookie.

(I've been running this loop for a while now:
# while : ; do tc actions change action ok index 500 cookie $i; let i++; done
and memory usage *seems* to be growing faster with the loop running - still
slow, but visible. (I stopped most but not all background processes in this
VM))

I don't see the growth with the change below (replacing both patches).
(although given the freeing of the old act_cookie pointer I wonder if
this needs additional locking?)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index b70aa57319ea..e05b924618a0 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -529,20 +529,20 @@ int tcf_action_dump(struct sk_buff *skb, struct list_head *actions,
 	return err;
 }
 
-static int nla_memdup_cookie(struct tc_action *a, struct nlattr **tb)
+static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb)
 {
-	a->act_cookie = kzalloc(sizeof(*a->act_cookie), GFP_KERNEL);
-	if (!a->act_cookie)
-		return -ENOMEM;
+	struct tc_cookie *c = kzalloc(sizeof(*c), GFP_KERNEL);
+	if (!c)
+		return NULL;
 
-	a->act_cookie->data = nla_memdup(tb[TCA_ACT_COOKIE], GFP_KERNEL);
-	if (!a->act_cookie->data) {
-		kfree(a->act_cookie);
-		return -ENOMEM;
+	c->data = nla_memdup(tb[TCA_ACT_COOKIE], GFP_KERNEL);
+	if (!c->data) {
+		kfree(c);
+		return NULL;
 	}
-	a->act_cookie->len = nla_len(tb[TCA_ACT_COOKIE]);
+	c->len = nla_len(tb[TCA_ACT_COOKIE]);
 
-	return 0;
+	return c;
 }
 
 struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
@@ -551,6 +551,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 {
 	struct tc_action *a;
 	struct tc_action_ops *a_o;
+	struct tc_cookie *cookie = NULL;
 	char act_name[IFNAMSIZ];
 	struct nlattr *tb[TCA_ACT_MAX + 1];
 	struct nlattr *kind;
@@ -566,6 +567,18 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 			goto err_out;
 		if (nla_strlcpy(act_name, kind, IFNAMSIZ) >= IFNAMSIZ)
 			goto err_out;
+		if (tb[TCA_ACT_COOKIE]) {
+			int cklen = nla_len(tb[TCA_ACT_COOKIE]);
+
+			if (cklen > TC_COOKIE_MAX_SIZE)
+				goto err_out;
+
+			cookie = nla_memdup_cookie(tb);
+			if (!cookie) {
+				err = -ENOMEM;
+				goto err_out;
+			}
+		}
 	} else {
 		err = -EINVAL;
 		if (strlcpy(act_name, name, IFNAMSIZ) >= IFNAMSIZ)
@@ -604,20 +617,12 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 	if (err < 0)
 		goto err_mod;
 
-	if (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;
-		}
-
-		if (nla_memdup_cookie(a, tb) < 0) {
-			err = -ENOMEM;
-			tcf_hash_release(a, bind);
-			goto err_mod;
+	if (name == NULL && tb[TCA_ACT_COOKIE]) {
+		if (a->act_cookie) {
+			kfree(a->act_cookie->data);
+			kfree(a->act_cookie);
 		}
+		a->act_cookie = cookie;
 	}
 
 	/* module count goes up only when brand new policy is created
@@ -632,6 +637,10 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
 err_mod:
 	module_put(a_o->owner);
 err_out:
+	if (cookie) {
+		kfree(cookie->data);
+		kfree(cookie);
+	}
 	return ERR_PTR(err);
 }

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

[Cong Wang]

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.

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

[Cong Wang]

On Wed, Apr 19, 2017 at 8:03 AM, Wolfgang Bumiller
<w.bumiller@proxmox.com> wrote:
>> On April 19, 2017 at 1:32 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> 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.
>
> Shouldn't the old code have freed an old a->act_cookie as well before
> replacing it then? nla_memdup_cookie() starts off by assigning a->act_cookie.
>

Yeah. Looks even better, we can totally avoid rollback the action modification
rollback on failure.

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

[Jamal Hadi Salim]

On 17-04-19 11:03 AM, Wolfgang Bumiller wrote:
>> On April 19, 2017 at 1:32 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>
>>
>> On 17-04-19 04:09 AM, Wolfgang Bumiller 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.
>>
>> 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.
>
> Shouldn't the old code have freed an old a->act_cookie as well before
> replacing it then? nla_memdup_cookie() starts off by assigning a->act_cookie.
>
> (I've been running this loop for a while now:
> # while : ; do tc actions change action ok index 500 cookie $i; let i++; done
> and memory usage *seems* to be growing faster with the loop running - still
> slow, but visible. (I stopped most but not all background processes in this
> VM))
>
> I don't see the growth with the change below (replacing both patches).
> (although given the freeing of the old act_cookie pointer I wonder if
> this needs additional locking?)
>

I think we are safe. The cookie should not be touched by any datapath
code.

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

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

[Jamal Hadi Salim]

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