[Openswan dev] Patch for review

[David McCullough]

Jivin Tuomo Soini lays it down ...
> Does this break klips?

Almost certainly,  but I haven't tried it yet :-)

> It seem to work perfectly on netkey.
> 
> This is not complete fix yet, Real fixing requires removing checking of
> initiate_ondemand_body return codes, This just hardcodes return code so
> it works and returns code to the state it was before all my experiments.

Just restating that I haven't tried this patch,  but it seems unlikely to
work for KLIPS.  The bug we had with klips was that if we didn't do anything
with the tunnel in initiate_ondemand,  then the kernel installed bare_shunt
would stay and cause problems.  Always removing the bare_shnut is just as
likely to cause problems later when we expect it to be there.  The klips ops
are very unforgiving,  if you call update,  it has to exist,  if you call
create it must not exist,  if you call delete,  it must exist.

Would it be possible for you to explain the bug you are trying to fix
when running with netkey ?  Is it for demand tunnels or not ? What is going
wrong and when ?

Comments below on how the patch can easily be made klips safe if that is the
right thing to do.

> commit 42e9ba4cd79187d0fe32ae6c1e29cf7e2ef7f916
> Author: Tuomo Soini <tis at foobar.fi>
> Date:   Thu Apr 22 12:42:43 2010 +0300
> 
>     This commit reverts changes which did fix spurious tunnel rekeyings
>     because of netkey acquires. But unlike those work-arounds this
>     change removes reason for those commits by always returning with 0 from
>     initiate_ondemand_body. And those reverted changes broke on-demand tunneling
>     by %hold state.
>     
>     If we return with non-0 value bare shunts are left behind and never
>     cleaned away causing invalid hold eroutes on netkey.
> 
> diff --git a/programs/pluto/initiate.c b/programs/pluto/initiate.c
> index e2e1c96..9d828ac 100644
> --- a/programs/pluto/initiate.c
> +++ b/programs/pluto/initiate.c
> @@ -717,7 +717,7 @@ initiate_ondemand_body(struct find_oppo_bundle *b
>      int hisport;
>      char demandbuf[256];
>      bool loggedit = FALSE;
> -    int work = 1; /* assume we did some */
> +    int work = 0; /* This is ugly fix, we just always return 0 - Tuomo */

How about:

	/* on klips/mast assume we will do something something */
	work = (kern_interface == USE_KLIPS || kern_interface == USE_MASTKLIPS);

>      
>      /* What connection shall we use?
>       * First try for one that explicitly handles the clients.
> @@ -780,15 +780,19 @@ initiate_ondemand_body(struct find_oppo_bundle *b
>  
>  	if(c->kind == CK_INSTANCE)
>  	{
> +	    char cib[CONN_INST_BUF];
>  	    /* there is already an instance being negotiated, do nothing */
> -	    return 0;
> +	    openswan_log("rekeying existing instance \"%s\"%s, due to acquire"
> +			 , c->name
> +			 , (fmt_conn_instance(c, cib), cib));
> +
> +	    /*
> +	     * we used to return here, but rekeying is a better choice. If we
> +	     * got the acquire, it is because something turned stuff into a
> +	     * %trap, or something got deleted, perhaps due to an expiry.
> +	     */

		work = 0; /* klips/mast need to know we are doing nothing */

>  	}
>  
> -	if(c->kind == CK_PERMANENT)
> -	{
> -	    /* there is already a tunnel, do nothing */
> -	    return 0;

Restore this, with the work = 0 and for netkey ot will be a NOOP.

		work = 0; /* klips/mast need to know we are doing nothing */

> -	}
>  	/* otherwise, there is some kind of static conn that can handle
>  	 * this connection, so we initiate it */
>  

Cheers,
Davidm

-- 
David McCullough,      david_mccullough at mcafee.com,  Ph:+61 734352815
McAfee - SnapGear      http://www.mcafee.com         http://www.uCdot.org