[PATCH] xfrm: Fix double dst_release() in xfrm_lookup() -EREMOTE case

[PATCH] xfrm: Fix double dst_release() in xfrm_lookup() -EREMOTE case

[Mark Brown]

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(-)

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;
 	}
-- 
1.7.0.3

Re: [PATCH] xfrm: Fix double dst_release() in xfrm_lookup() -EREMOTE case

[Timo Teräs]

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

Re: [PATCH] xfrm: Fix double dst_release() in xfrm_lookup() -EREMOTE case

[Mark Brown]

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.