| Message ID | 1375351716-3265-1-git-send-email-fan.du@windriver.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Aug 01, 2013 at 06:08:36PM +0800, Fan Du wrote: > Both policy timer and hold_timer need to be deleted when destroy policy > Bad mood today, maybe I'm wrong about this... > > Signed-off-by: Fan Du <fan.du@windriver.com> > --- > net/xfrm/xfrm_policy.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > index d8da6b8..f7078eb 100644 > --- a/net/xfrm/xfrm_policy.c > +++ b/net/xfrm/xfrm_policy.c > @@ -308,7 +308,7 @@ void xfrm_policy_destroy(struct xfrm_policy *policy) > { > BUG_ON(!policy->walk.dead); > > - if (del_timer(&policy->timer)) > + if (del_timer(&policy->timer) || del_timer(&policy->polq.hold_timer)) > BUG(); > The timers should be already deleted when xfrm_policy_destroy() is called. This is just to check if that really happened and to catch this bug if not. So it's not a bug fix, it just helps to catch a potential bug. I'll consider to take this into ipsec-next after some testing. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013年08月01日 20:01, Steffen Klassert wrote: > On Thu, Aug 01, 2013 at 06:08:36PM +0800, Fan Du wrote: >> Both policy timer and hold_timer need to be deleted when destroy policy >> Bad mood today, maybe I'm wrong about this... >> >> Signed-off-by: Fan Du<fan.du@windriver.com> >> --- >> net/xfrm/xfrm_policy.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c >> index d8da6b8..f7078eb 100644 >> --- a/net/xfrm/xfrm_policy.c >> +++ b/net/xfrm/xfrm_policy.c >> @@ -308,7 +308,7 @@ void xfrm_policy_destroy(struct xfrm_policy *policy) >> { >> BUG_ON(!policy->walk.dead); >> >> - if (del_timer(&policy->timer)) >> + if (del_timer(&policy->timer) || del_timer(&policy->polq.hold_timer)) >> BUG(); >> > > The timers should be already deleted when xfrm_policy_destroy() is > called. This is just to check if that really happened and to > catch this bug if not. So it's not a bug fix, it just helps to > catch a potential bug. I'll consider to take this into ipsec-next > after some testing. > On the contrary, please take a look at callers of xfrm_policy_destroy. Code flow is as below: xfrm_policy_alloc() /* setup timer/hold_timer here */ (1) /* do something with this policy, insert or something else */ goto err; /* bail out if bad things happens */ return ok err: xfrm_policy_destroy() /*So both timers in (1) need to be deleted */
On Fri, Aug 02, 2013 at 09:22:08AM +0800, Fan Du wrote: > > > On 2013年08月01日 20:01, Steffen Klassert wrote: > >On Thu, Aug 01, 2013 at 06:08:36PM +0800, Fan Du wrote: > >>Both policy timer and hold_timer need to be deleted when destroy policy > >>Bad mood today, maybe I'm wrong about this... > >> > >>Signed-off-by: Fan Du<fan.du@windriver.com> > >>--- > >> net/xfrm/xfrm_policy.c | 2 +- > >> 1 files changed, 1 insertions(+), 1 deletions(-) > >> > >>diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > >>index d8da6b8..f7078eb 100644 > >>--- a/net/xfrm/xfrm_policy.c > >>+++ b/net/xfrm/xfrm_policy.c > >>@@ -308,7 +308,7 @@ void xfrm_policy_destroy(struct xfrm_policy *policy) > >> { > >> BUG_ON(!policy->walk.dead); > >> > >>- if (del_timer(&policy->timer)) > >>+ if (del_timer(&policy->timer) || del_timer(&policy->polq.hold_timer)) > >> BUG(); > >> > > > >The timers should be already deleted when xfrm_policy_destroy() is > >called. This is just to check if that really happened and to > >catch this bug if not. So it's not a bug fix, it just helps to > >catch a potential bug. I'll consider to take this into ipsec-next > >after some testing. > > > > On the contrary, please take a look at callers of xfrm_policy_destroy. > Code flow is as below: > > xfrm_policy_alloc() /* setup timer/hold_timer here */ (1) > > /* do something with this policy, insert or something else */ > > goto err; /* bail out if bad things happens */ > > return ok > > err: > xfrm_policy_destroy() /*So both timers in (1) need to be deleted */ > setup_timer() does not arm the timer, it just initializes the timer. So the timer is not pending when xfrm_policy_destroy() is called on error. The del_timer() in xfrm_policy_destroy() is really just to catch a bug. That's also the reason why we call BUG() if we delete a pending timer. Anyway, applied to ipsec-next. Thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index d8da6b8..f7078eb 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -308,7 +308,7 @@ void xfrm_policy_destroy(struct xfrm_policy *policy) { BUG_ON(!policy->walk.dead); - if (del_timer(&policy->timer)) + if (del_timer(&policy->timer) || del_timer(&policy->polq.hold_timer)) BUG(); security_xfrm_policy_free(policy->security);
Both policy timer and hold_timer need to be deleted when destroy policy Bad mood today, maybe I'm wrong about this... Signed-off-by: Fan Du <fan.du@windriver.com> --- net/xfrm/xfrm_policy.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)