Re: [PATCH] IPSec fixes

On Wed, 14 Jan 2004, Jun-ichiro itojun Hagino wrote:

> > 	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.
>
> 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 */

this is ok as it is unneeded. the newsp (sp returned from key_msg2sp())
will have a refcnt set to 1.  This is the refcnt for the sptree.


>  	newsp->state = IPSEC_SPSTATE_ALIVE;
>  	LIST_INSERT_TAIL(&sptree[newsp->dir], newsp, secpolicy, chain);
> +	newsp->refcnt++;

this will give you an additional refcnt (being 2 at this point) which
you will decrement where/when ?


>  	/* 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? */

This check is nice but to my understanding entirely useless anyway.
__LIST_CHAINED checks for backward or forward pointer to be not NULL
but LIST_REMOVE will never NULL them (not with FreeBSD and not with
NetBSD) - please correct me if I am wrong.

So without s.th. like:
+               sp->chain.le_next = NULL;
+               sp->chain.le_prev = NULL;
the above should not make any difference from what has been there
before.

It shouldn't matter either as key_sp_unlink() should not be called
twice for the same sp. This made me thinking as I previously thought I
had found a path where this could happen but looking through the
source of KAME tree I cannot find it; I suspect I will not be able to
find it in the clean FreeBSD tree either.

This further means that you had been right and the unlink call from
spdflush seems ok but this does not align with my debugging results.
The last patch I had done that time had been:

--- cut ---
--- src-20031223-01/sys/netkey/key.c.orig       Wed Jan  7 20:54:44 2004
+++ src-20031223-01/sys/netkey/key.c    Wed Jan  7 21:04:17 2004
_at__at_ -2531,8 +2531,11 _at__at_ key_spdflush(so, m, mhp)
                if (sp->state == IPSEC_SPSTATE_DEAD)
                        continue;
                key_sp_dead(sp);
+#if 0  /* unlink will be done by someone else;
+          only mark it DEAD */
                key_sp_unlink(sp);
                sp = NULL;
+#endif
        }

        /* invalidate all cached SPD pointers on pcb */
--- cut ---

With this my last troubles had gone and I could not see any more
0xdeadcodd (0xdeadcode with a sp->refcnt--).

So I will again look into my debugging code, my debugging notes and
my debugging logs and see if I can reproduce the problem, and how -
meaning running through which path of the code makes me see 0xdeadc0dd.

I am sorry I haven't found the time for compiling and testing a new
kernel today.

You may perhaps want to wait before you commit your above patch.

-- 
Greetings

Bjoern A. Zeeb				bzeeb at Zabbadoz dot NeT
56 69 73 69 74				http://www.zabbadoz.net/