* [PATCH v2 net 1/2] net sched actions: fix access to uninitialized data @ 2017-04-18 10:13 Wolfgang Bumiller 2017-04-18 10:13 ` [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier Wolfgang Bumiller ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Wolfgang Bumiller @ 2017-04-18 10:13 UTC (permalink / raw) To: netdev; +Cc: Jamal Hadi Salim, David S. Miller, Cong Wang 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier 2017-04-18 10:13 [PATCH v2 net 1/2] net sched actions: fix access to uninitialized data Wolfgang Bumiller @ 2017-04-18 10:13 ` Wolfgang Bumiller 2017-04-18 13:03 ` Jamal Hadi Salim 2017-04-18 17:03 ` Cong Wang 2017-04-18 12:51 ` [PATCH v2 net 1/2] net sched actions: fix access to uninitialized data Jamal Hadi Salim 2017-04-19 9:02 ` Daniel Borkmann 2 siblings, 2 replies; 15+ messages in thread From: Wolfgang Bumiller @ 2017-04-18 10:13 UTC (permalink / raw) To: netdev; +Cc: Jamal Hadi Salim, David S. Miller, Cong Wang 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier 2017-04-18 10:13 ` [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier Wolfgang Bumiller @ 2017-04-18 13:03 ` Jamal Hadi Salim 2017-04-18 17:03 ` Cong Wang 1 sibling, 0 replies; 15+ messages in thread From: Jamal Hadi Salim @ 2017-04-18 13:03 UTC (permalink / raw) To: Wolfgang Bumiller, netdev; +Cc: David S. Miller, Cong Wang, Roman Mashak 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier 2017-04-18 10:13 ` [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier Wolfgang Bumiller 2017-04-18 13:03 ` Jamal Hadi Salim @ 2017-04-18 17:03 ` Cong Wang 2017-04-19 2:21 ` Jamal Hadi Salim 1 sibling, 1 reply; 15+ messages in thread From: Cong Wang @ 2017-04-18 17:03 UTC (permalink / raw) To: Wolfgang Bumiller Cc: Linux Kernel Network Developers, Jamal Hadi Salim, David S. Miller 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier 2017-04-18 17:03 ` Cong Wang @ 2017-04-19 2:21 ` Jamal Hadi Salim 2017-04-19 6:23 ` Cong Wang 0 siblings, 1 reply; 15+ messages in thread From: Jamal Hadi Salim @ 2017-04-19 2:21 UTC (permalink / raw) To: Cong Wang, Wolfgang Bumiller Cc: Linux Kernel Network Developers, David S. Miller 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier 2017-04-19 2:21 ` Jamal Hadi Salim @ 2017-04-19 6:23 ` Cong Wang 2017-04-19 8:09 ` Wolfgang Bumiller 0 siblings, 1 reply; 15+ messages in thread From: Cong Wang @ 2017-04-19 6:23 UTC (permalink / raw) To: Jamal Hadi Salim Cc: Wolfgang Bumiller, Linux Kernel Network Developers, David S. Miller 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: ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier 2017-04-19 6:23 ` Cong Wang @ 2017-04-19 8:09 ` Wolfgang Bumiller 2017-04-19 11:32 ` Jamal Hadi Salim 0 siblings, 1 reply; 15+ messages in thread From: Wolfgang Bumiller @ 2017-04-19 8:09 UTC (permalink / raw) To: Jamal Hadi Salim, Cong Wang Cc: Linux Kernel Network Developers, David S. Miller > 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: ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier 2017-04-19 8:09 ` Wolfgang Bumiller @ 2017-04-19 11:32 ` Jamal Hadi Salim 2017-04-19 15:03 ` Wolfgang Bumiller 2017-04-19 17:25 ` Cong Wang 0 siblings, 2 replies; 15+ messages in thread From: Jamal Hadi Salim @ 2017-04-19 11:32 UTC (permalink / raw) To: Wolfgang Bumiller, Cong Wang Cc: Linux Kernel Network Developers, David S. Miller 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier 2017-04-19 11:32 ` Jamal Hadi Salim @ 2017-04-19 15:03 ` Wolfgang Bumiller 2017-04-19 21:32 ` Cong Wang 2017-04-20 10:24 ` Jamal Hadi Salim 2017-04-19 17:25 ` Cong Wang 1 sibling, 2 replies; 15+ messages in thread From: Wolfgang Bumiller @ 2017-04-19 15:03 UTC (permalink / raw) To: Jamal Hadi Salim, Cong Wang Cc: Linux Kernel Network Developers, David S. Miller > 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); } ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier 2017-04-19 15:03 ` Wolfgang Bumiller @ 2017-04-19 21:32 ` Cong Wang 2017-04-20 10:24 ` Jamal Hadi Salim 1 sibling, 0 replies; 15+ messages in thread From: Cong Wang @ 2017-04-19 21:32 UTC (permalink / raw) To: Wolfgang Bumiller Cc: Jamal Hadi Salim, Linux Kernel Network Developers, David S. Miller 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier 2017-04-19 15:03 ` Wolfgang Bumiller 2017-04-19 21:32 ` Cong Wang @ 2017-04-20 10:24 ` Jamal Hadi Salim 1 sibling, 0 replies; 15+ messages in thread From: Jamal Hadi Salim @ 2017-04-20 10:24 UTC (permalink / raw) To: Wolfgang Bumiller, Cong Wang Cc: Linux Kernel Network Developers, David S. Miller 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier 2017-04-19 11:32 ` Jamal Hadi Salim 2017-04-19 15:03 ` Wolfgang Bumiller @ 2017-04-19 17:25 ` Cong Wang 2017-04-20 10:28 ` Jamal Hadi Salim 1 sibling, 1 reply; 15+ messages in thread From: Cong Wang @ 2017-04-19 17:25 UTC (permalink / raw) To: Jamal Hadi Salim Cc: Wolfgang Bumiller, Linux Kernel Network Developers, David S. Miller 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier 2017-04-19 17:25 ` Cong Wang @ 2017-04-20 10:28 ` Jamal Hadi Salim 0 siblings, 0 replies; 15+ messages in thread From: Jamal Hadi Salim @ 2017-04-20 10:28 UTC (permalink / raw) To: Cong Wang Cc: Wolfgang Bumiller, Linux Kernel Network Developers, David S. Miller, Lucas Bates, Roman Mashak 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 net 1/2] net sched actions: fix access to uninitialized data 2017-04-18 10:13 [PATCH v2 net 1/2] net sched actions: fix access to uninitialized data Wolfgang Bumiller 2017-04-18 10:13 ` [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier Wolfgang Bumiller @ 2017-04-18 12:51 ` Jamal Hadi Salim 2017-04-19 9:02 ` Daniel Borkmann 2 siblings, 0 replies; 15+ messages in thread From: Jamal Hadi Salim @ 2017-04-18 12:51 UTC (permalink / raw) To: Wolfgang Bumiller, netdev; +Cc: David S. Miller, Cong Wang 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 net 1/2] net sched actions: fix access to uninitialized data 2017-04-18 10:13 [PATCH v2 net 1/2] net sched actions: fix access to uninitialized data Wolfgang Bumiller 2017-04-18 10:13 ` [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier Wolfgang Bumiller 2017-04-18 12:51 ` [PATCH v2 net 1/2] net sched actions: fix access to uninitialized data Jamal Hadi Salim @ 2017-04-19 9:02 ` Daniel Borkmann 2 siblings, 0 replies; 15+ messages in thread From: Daniel Borkmann @ 2017-04-19 9:02 UTC (permalink / raw) To: Wolfgang Bumiller, netdev; +Cc: Jamal Hadi Salim, David S. Miller, Cong Wang 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-04-20 10:28 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-18 10:13 [PATCH v2 net 1/2] net sched actions: fix access to uninitialized data Wolfgang Bumiller 2017-04-18 10:13 ` [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier Wolfgang Bumiller 2017-04-18 13:03 ` Jamal Hadi Salim 2017-04-18 17:03 ` Cong Wang 2017-04-19 2:21 ` Jamal Hadi Salim 2017-04-19 6:23 ` Cong Wang 2017-04-19 8:09 ` Wolfgang Bumiller 2017-04-19 11:32 ` Jamal Hadi Salim 2017-04-19 15:03 ` Wolfgang Bumiller 2017-04-19 21:32 ` Cong Wang 2017-04-20 10:24 ` Jamal Hadi Salim 2017-04-19 17:25 ` Cong Wang 2017-04-20 10:28 ` Jamal Hadi Salim 2017-04-18 12:51 ` [PATCH v2 net 1/2] net sched actions: fix access to uninitialized data Jamal Hadi Salim 2017-04-19 9:02 ` Daniel Borkmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).