| Message ID | 20170418101322.27666-2-w.bumiller@proxmox.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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.
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
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;
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(-)