| Message ID | 20170413080648.bbiikladwevaribf@olga.proxmox.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Apr 13, 2017 at 1:06 AM, Wolfgang Bumiller <w.bumiller@proxmox.com> wrote: > On Wed, Apr 12, 2017 at 09:27:31PM -0700, Cong Wang wrote: >> Instead of duplicating code, you can add the check >> to the module_put() next to err_mod label? I mean: > > I just realized that with module_put() happening in both error and > success cases if `err != ACT_P_CREATED`, we could just move that code up > to above the TCA_ACT_COOKIE handling? Yes, even better. > Btw., the comment confused me a little at first as I thought it's about > what happens in ->init(). But reading the code I then noticed the module > count is increased in tc_lookup_action_n() (which calls try_module_get) > in this functions and it's about how this function itself is supposed > to affect the count - if I'm not mistaken. > => so I think it makes sense to deal with this earlier. Yes, the module reference count is not increased inside ->init(), it is because of the semantic of ->init(), it could create a new action or modify existing one, for the cast latter we need to rollback the refcount. Please feel free to update that comment to make it more clear, since you are already on it. ;) > > Otherwise I'd have to save `err != ACT_P_CREATED` in an additional > variable for the err_mod case since the cookie handling modifies `err`. > > What about this? (Since it's a separate issue not directly related to > patch 1 of the series I can send it as separate mail based on master if > you prefer - the diff below is based on master+patch1 for now.) > Looks good, this could also address Roman's comment. Please remove the RFC tag and resend the whole series. You can also add my: Acked-by: Cong Wang <xiyou.wangcong@gmail.com> Thanks.
On Thu, Apr 13, 2017 at 11:03:37AM -0700, Cong Wang wrote: > On Thu, Apr 13, 2017 at 1:06 AM, Wolfgang Bumiller > <w.bumiller@proxmox.com> wrote: > > On Wed, Apr 12, 2017 at 09:27:31PM -0700, Cong Wang wrote: > >> Instead of duplicating code, you can add the check > >> to the module_put() next to err_mod label? I mean: > > > > I just realized that with module_put() happening in both error and > > success cases if `err != ACT_P_CREATED`, we could just move that code up > > to above the TCA_ACT_COOKIE handling? > > Yes, even better. > > > Btw., the comment confused me a little at first as I thought it's about > > what happens in ->init(). But reading the code I then noticed the module > > count is increased in tc_lookup_action_n() (which calls try_module_get) > > in this functions and it's about how this function itself is supposed > > to affect the count - if I'm not mistaken. > > => so I think it makes sense to deal with this earlier. > > Yes, the module reference count is not increased inside ->init(), > it is because of the semantic of ->init(), it could create a new action > or modify existing one, for the cast latter we need to rollback the > refcount. Please feel free to update that comment to make it more > clear, since you are already on it. ;) Will do. > > > > > Otherwise I'd have to save `err != ACT_P_CREATED` in an additional > > variable for the err_mod case since the cookie handling modifies `err`. > > > > What about this? (Since it's a separate issue not directly related to > > patch 1 of the series I can send it as separate mail based on master if > > you prefer - the diff below is based on master+patch1 for now.) > > > > Looks good, this could also address Roman's comment. Please remove > the RFC tag and resend the whole series. > > You can also add my: > > Acked-by: Cong Wang <xiyou.wangcong@gmail.com> Before I do that - trying to wrap my head around the interdependencies here better to be thorough - I noticed that tcf_hash_release() can return ACT_P_DELETED. The ACT_P_CREATED case means tcf_hash_create() was used, in the other case the tc_action's ref & bind count is bumped by tcf_hash_check() and then also decremented by tcf_hash_release() if it existed, iow. kept at 1, but not always: It does always happen in act_police.c but in other files such as act_bpf.c or act_connmark.c if eg. bind is set they return without decrementing, so both ref&bind count are bumped when they return - the refcount logic isn't easy to follow for a newcomer. Now there are two uses of __tcf_hash_release() in act_api.c which check for a return value of ACT_P_DELETED, in which case they call module_put(). So I'm not sure exactly how the module and tc_action counts are related (and I usually like to understand my own patches ;-) ). Maybe I'm missing something obvious but I'm currently a bit confused as to whether the tcf_hash_release() call there is okay, or should have its return value checked or should depend on ->init()'s ACT_P_CREATED value as well?
On Fri, Apr 14, 2017 at 2:08 AM, Wolfgang Bumiller <w.bumiller@proxmox.com> wrote: > Before I do that - trying to wrap my head around the interdependencies > here better to be thorough - I noticed that tcf_hash_release() can > return ACT_P_DELETED. The ACT_P_CREATED case means tcf_hash_create() > was used, in the other case the tc_action's ref & bind count is bumped > by tcf_hash_check() and then also decremented by tcf_hash_release() if > it existed, iow. kept at 1, but not always: It does always happen in > act_police.c but in other files such as act_bpf.c or act_connmark.c if > eg. bind is set they return without decrementing, so both ref&bind count > are bumped when they return - the refcount logic isn't easy to follow > for a newcomer. Now there are two uses of __tcf_hash_release() in > act_api.c which check for a return value of ACT_P_DELETED, in which case > they call module_put(). That's the nasty part... IIRC, Jamal has fixed two bugs on action refcnt'ing. We really need to clean up the code. > So I'm not sure exactly how the module and tc_action counts are related > (and I usually like to understand my own patches ;-) ). Each action holds a refcnt to its module, each filter holds a refcnt to its bound or referenced (unbound) action. > Maybe I'm missing something obvious but I'm currently a bit confused as > to whether the tcf_hash_release() call there is okay, or should have its > return value checked or should depend on ->init()'s ACT_P_CREATED value > as well? > I think it's the same? If we have ACT_P_CREATED here, tcf_hash_release() will return ACT_P_DELETED for sure because the newly created action has refcnt==1? Thanks.
> On April 15, 2017 at 8:20 PM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > On Fri, Apr 14, 2017 at 2:08 AM, Wolfgang Bumiller > <w.bumiller@proxmox.com> wrote: > > Before I do that - trying to wrap my head around the interdependencies > > here better to be thorough - I noticed that tcf_hash_release() can > > return ACT_P_DELETED. The ACT_P_CREATED case means tcf_hash_create() > > was used, in the other case the tc_action's ref & bind count is bumped > > by tcf_hash_check() and then also decremented by tcf_hash_release() if > > it existed, iow. kept at 1, but not always: It does always happen in > > act_police.c but in other files such as act_bpf.c or act_connmark.c if > > eg. bind is set they return without decrementing, so both ref&bind count > > are bumped when they return - the refcount logic isn't easy to follow > > for a newcomer. Now there are two uses of __tcf_hash_release() in > > act_api.c which check for a return value of ACT_P_DELETED, in which case > > they call module_put(). > > > That's the nasty part... IIRC, Jamal has fixed two bugs on action refcnt'ing. > We really need to clean up the code. > > > So I'm not sure exactly how the module and tc_action counts are related > > (and I usually like to understand my own patches ;-) ). > > > Each action holds a refcnt to its module, each filter holds a refcnt to > its bound or referenced (unbound) action. > > > > Maybe I'm missing something obvious but I'm currently a bit confused as > > to whether the tcf_hash_release() call there is okay, or should have its > > return value checked or should depend on ->init()'s ACT_P_CREATED value > > as well? > > > > I think it's the same? If we have ACT_P_CREATED here, tcf_hash_release() > will return ACT_P_DELETED for sure because the newly created action has > refcnt==1? Makes sense on the one hand, but for ACT_P_DELETED both ref and bind count need to reach 0, so I'm still concerned that the different behaviors I mentioned above might be problematic if we use ACT_P_CREATED only. (It also means my patches still leak a count - which is probably still better than the previous underflow, but ultimately doesn't satisfy me.) Should I still resend it this way for the record with the Acked-bys? (Since given the fact that with unprivileged containers it's possible to trigger this access and potentially crash the kernel I strongly feel that some version of this should end up in the 4.11 release.)
Wolfgang, can you _please_ stop cc-ing lkml? Everybody you need to deal with is on netdev. On 17-04-15 02:48 PM, Wolfgang Bumiller wrote: > >> >>> So I'm not sure exactly how the module and tc_action counts are related >>> (and I usually like to understand my own patches ;-) ). >> >> Every time we add an action, refcnt goes up. Every time we bind an action to a filter the refcnt + bindcnt goes up. Reverse for everytime we delete a filter or action. The action does not get destroyed because the filter was deleted. Everytime the action gets its refcount bumped up (for either of the two reasons described above), the module ref needs incrementing. Everytime the action refcnt goes down the module ref needs to be decremented. Only when both refcnt and bind cnt go to at least zero do we really delete/destroy. >> Each action holds a refcnt to its module, each filter holds a refcnt to >> its bound or referenced (unbound) action. >> >> >>> Maybe I'm missing something obvious but I'm currently a bit confused as >>> to whether the tcf_hash_release() call there is okay, or should have its >>> return value checked or should depend on ->init()'s ACT_P_CREATED value >>> as well? >>> >> >> I think it's the same? If we have ACT_P_CREATED here, tcf_hash_release() >> will return ACT_P_DELETED for sure because the newly created action has >> refcnt==1? > > Makes sense on the one hand, but for ACT_P_DELETED both ref and bind > count need to reach 0, so I'm still concerned that the different behaviors > I mentioned above might be problematic if we use ACT_P_CREATED only. > (It also means my patches still leak a count - which is probably still > better than the previous underflow, but ultimately doesn't satisfy me.) > Should I still resend it this way for the record with the Acked-bys? > (Since given the fact that with unprivileged containers it's possible to > trigger this access and potentially crash the kernel I strongly feel that > some version of this should end up in the 4.11 release.) > I think that is an unrelated code path, current issue is on the create/replace code path. We need to separate delete from get - we just havent got around to it yet. Followup patches will make sense. Agree, this change should go in quickly.. Actually - come to think of it: You should probably leave the module put to the end instead of the beginning because it protects against another entry unloading the module while we are still processing. Just post the patch - we'll help reviewing it. cheers, jamal
On Sat, Apr 15, 2017 at 11:48 AM, Wolfgang Bumiller <w.bumiller@proxmox.com> wrote: > >> On April 15, 2017 at 8:20 PM Cong Wang <xiyou.wangcong@gmail.com> wrote: >> >> >> On Fri, Apr 14, 2017 at 2:08 AM, Wolfgang Bumiller >> <w.bumiller@proxmox.com> wrote: >> > Before I do that - trying to wrap my head around the interdependencies >> > here better to be thorough - I noticed that tcf_hash_release() can >> > return ACT_P_DELETED. The ACT_P_CREATED case means tcf_hash_create() >> > was used, in the other case the tc_action's ref & bind count is bumped >> > by tcf_hash_check() and then also decremented by tcf_hash_release() if >> > it existed, iow. kept at 1, but not always: It does always happen in >> > act_police.c but in other files such as act_bpf.c or act_connmark.c if >> > eg. bind is set they return without decrementing, so both ref&bind count >> > are bumped when they return - the refcount logic isn't easy to follow >> > for a newcomer. Now there are two uses of __tcf_hash_release() in >> > act_api.c which check for a return value of ACT_P_DELETED, in which case >> > they call module_put(). >> >> >> That's the nasty part... IIRC, Jamal has fixed two bugs on action refcnt'ing. >> We really need to clean up the code. >> >> > So I'm not sure exactly how the module and tc_action counts are related >> > (and I usually like to understand my own patches ;-) ). >> >> >> Each action holds a refcnt to its module, each filter holds a refcnt to >> its bound or referenced (unbound) action. >> >> >> > Maybe I'm missing something obvious but I'm currently a bit confused as >> > to whether the tcf_hash_release() call there is okay, or should have its >> > return value checked or should depend on ->init()'s ACT_P_CREATED value >> > as well? >> > >> >> I think it's the same? If we have ACT_P_CREATED here, tcf_hash_release() >> will return ACT_P_DELETED for sure because the newly created action has >> refcnt==1? > > Makes sense on the one hand, but for ACT_P_DELETED both ref and bind > count need to reach 0, so I'm still concerned that the different behaviors Bind refcnt is only used when it is bound to a filter and refcnt is always used, so either bind refcnt is 0 or it is same with refcnt. > I mentioned above might be problematic if we use ACT_P_CREATED only. > (It also means my patches still leak a count - which is probably still > better than the previous underflow, but ultimately doesn't satisfy me.) > Should I still resend it this way for the record with the Acked-bys? > (Since given the fact that with unprivileged containers it's possible to > trigger this access and potentially crash the kernel I strongly feel that > some version of this should end up in the 4.11 release.) I think so. Thanks.
diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 8cc883c063f0..7d920ef83a07 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -604,28 +604,29 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla, if (err < 0) goto err_mod; + /* 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); + 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;