lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 23 Jul 2014 15:09:38 -0700 (PDT) From: David Miller <davem@...emloft.net> To: eric.dumazet@...il.com Cc: steffen.klassert@...unet.com, herbert@...dor.apana.org.au, netdev@...r.kernel.org Subject: Re: [PATCH 1/2] xfrm: Fix refcount imbalance in xfrm_lookup From: Eric Dumazet <eric.dumazet@...il.com> Date: Wed, 23 Jul 2014 19:21:13 +0200 > On Wed, 2014-07-23 at 10:18 +0200, Steffen Klassert wrote: >> xfrm_lookup must return a dst_entry with a refcount for the caller. >> Git commit 1a1ccc96abb ("xfrm: Remove caching of xfrm_policy_sk_bundles") >> removed this refcount for the socket policy case accidentally. >> This patch restores it and sets DST_NOCACHE flag to make sure >> that the dst_entry is freed when the refcount becomes null. >> >> Fixes: 1a1ccc96abb ("xfrm: Remove caching of xfrm_policy_sk_bundles") >> Signed-off-by: Steffen Klassert <steffen.klassert@...unet.com> >> --- >> net/xfrm/xfrm_policy.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c >> index a8ef510..0525d78 100644 >> --- a/net/xfrm/xfrm_policy.c >> +++ b/net/xfrm/xfrm_policy.c >> @@ -2097,6 +2097,8 @@ struct dst_entry *xfrm_lookup(struct net *net, struct dst_entry *dst_orig, >> goto no_transform; >> } >> >> + dst_hold(&xdst->u.dst); >> + xdst->u.dst.flags |= DST_NOCACHE; >> route = xdst->route; >> } >> } > > > Hmm... are you sure ? > > Before 1a1ccc96abb we did a dst_hold() because we kept the dst in > xfrm_policy_sk_bundles. So keeping a reference count was mandatory. > > But after, I don't understand why it is needed at all. It should be needed. We create a new bundle here, from scratch, based upon the socket policy. Such new bundles make routes with dst refcount == 0. All callers expect that the route given back to them is either the original 'dst' passed into xfrm_lookup() unmolested, or a new 'dst' having a refcount taken for this caller. That's why all call sites expect that they can just go "dst_release(dst)" and these objects it works. So, if we returned 'dst' with refcount == 0 it would not work. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists