Re: [PATCH] IPSec fixes

> > > 	as for key_sp_unlink(), i don't think the patch is correct.
> > > 	even if you do not call key_sp_unlink() in key_spdflush(), spd entries
> > > 	will get unlink'ed in key_timehandler().  therefore the end result
> > > 	will be the same.
> > 
> > No ! calling key_sp_unlink() from key_spdflush() will result in an
> > _extra_ call of key_freesp() and thus refcnt will be decremented
> > though it shouldn't.
> > This will result in a refcnt being 0 too early and with valid
> > pointers to that secpolicy and will further lead to "Memory accessed
> > and/or modified after free" situations somewhen after the first and
> > all successive flushes of the SPD.
> > Each part of the code checks for the state == .._DEAD when getting an
> > sp from sptree so the comment above key_spdflush() is correct. Only
> > mark the sp as dead.
> > 
> > Hope this explains the problem a bit better.
> 
> 	the refcnt decremented in key_sp_unlink() is for the link from
> 	sptree[].  i guess this is the proper fix.  does it fix your situation?

	oops.

itojun


Index: key.c
===================================================================
RCS file: /cvsroot/kame/kame/kame/sys/netkey/key.c,v
retrieving revision 1.324
diff -u -r1.324 key.c
--- key.c	14 Jan 2004 04:10:24 -0000	1.324
+++ key.c	14 Jan 2004 08:45:36 -0000
_at__at_ -2092,9 +2092,9 _at__at_
 	newsp->lifetime = lft ? lft->sadb_lifetime_addtime : 0;
 	newsp->validtime = lft ? lft->sadb_lifetime_usetime : 0;
 
-	newsp->refcnt = 1;	/* do not reclaim until I say I do */
 	newsp->state = IPSEC_SPSTATE_ALIVE;
 	LIST_INSERT_TAIL(&sptree[newsp->dir], newsp, secpolicy, chain);
+	newsp->refcnt++;
 
 	/* delete the entry in spacqtree */
 	if (mhp->msg->sadb_msg_type == SADB_X_SPDUPDATE &&
_at__at_ -8080,9 +8080,10 _at__at_
 {
 
 	/* remove from SP index */
-	if (__LIST_CHAINED(sp))
+	if (__LIST_CHAINED(sp)) {
 		LIST_REMOVE(sp, chain);
-	key_freesp(sp);
+		key_freesp(sp);
+	}
 }
 
 /* XXX too much? */