| Message ID | 1270729773-1758-1-git-send-email-broonie@opensource.wolfsonmicro.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
Mark Brown wrote: > Commit 80c802 ("xfrm: cache bundles instead of policies for outgoing > flows") changed __xfrm_policy() to call dst_release() when returning > -EREMOTE. In the case where this is called from xfrm_lookup() a double > call to dst_release() would result due to the existing dst_release() > there. Remove the dst_release() in xfrm_lookup(). > > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> > --- > > I'm not sure if this is correct or not - there may a reference been > taken earlier in __xfrm_lookup() that's being dropped but I didn't spot > it. This is not correct. The only case we return -EREMOTE from __xfrm_policy() is at: if (net->xfrm.sysctl_larval_drop) { /* EREMOTE tells the caller to generate * a one-shot blackhole route. */ dst_release(dst); xfrm_pols_put(pols, num_pols); XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTNOSTATES); return -EREMOTE; } It drops the inner xfrm_dst returned from flow cache lookup / resolver. xfrm_lookup() will drop the original dst (which is not xfrm_dst). This semantics is important because __xfrm_lookup() is also called from other places, that do other things when they get -EREMOTE. - TImo -- 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 Thu, Apr 08, 2010 at 03:39:38PM +0300, Timo Teräs wrote: > Mark Brown wrote: > >I'm not sure if this is correct or not - there may a reference been > >taken earlier in __xfrm_lookup() that's being dropped but I didn't spot > >it. > This is not correct. So I was correct when I said that there might've been a reference taken earlier :) > This semantics is important because __xfrm_lookup() is also called > from other places, that do other things when they get -EREMOTE. Right, it was the fact that this was the only place doing the free that made this unclear. Some comments might make this rather more obvious, the fact that the release was added to __xfrm_lookup() as part of the recent patch made it unclear if the release that was still there in xfrm_lookup() was still needed or an oversight. -- 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 7430ac2..f133036 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -1881,7 +1881,6 @@ int xfrm_lookup(struct net *net, struct dst_entry **dst_p, struct flowi *fl, int err = __xfrm_lookup(net, dst_p, fl, sk, flags); if (err == -EREMOTE) { - dst_release(*dst_p); *dst_p = NULL; err = -EAGAIN; }
Commit 80c802 ("xfrm: cache bundles instead of policies for outgoing flows") changed __xfrm_policy() to call dst_release() when returning -EREMOTE. In the case where this is called from xfrm_lookup() a double call to dst_release() would result due to the existing dst_release() there. Remove the dst_release() in xfrm_lookup(). Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> --- I'm not sure if this is correct or not - there may a reference been taken earlier in __xfrm_lookup() that's being dropped but I didn't spot it. net/xfrm/xfrm_policy.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)