PATCH: IPSEC acquire in presence of multiple managers

PATCH: IPSEC acquire in presence of multiple managers

[jamal]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]


Herbert/Dave,

Acquire should be supported by both pfkey and netlink.
However, it stops to send acquire message from the kernel on first
success.
It is possible that one or the other manager maybe passively monitoring
and needs to see those messages.

Thanks to Masahide NAKAMURA <nakam@linux-ipv6.org>, Shinta Sugimoto
<shinta.sugimoto@ericsson.com> for both identifying the issue and
validating the fix.

If you want i can put this with other NLMSGALIGN one liner i mentioned
earlier.

cheers,
jamal

[-- Attachment #2: acq_p --]
[-- Type: text/plain, Size: 708 bytes --]

--- a/net/xfrm/xfrm_state.c	2005/03/25 17:44:43	1.1
+++ b/net/xfrmxfrm_state.c	2005/03/25 18:55:33
@@ -774,16 +774,20 @@
 		wake_up(&km_waitq);
 }
 
+/*
+ * We send to all registered managers regardless of failure
+ * We are happy with one success
+*/
 int km_query(struct xfrm_state *x, struct xfrm_tmpl *t, struct xfrm_policy *pol)
 {
-	int err = -EINVAL;
+	int err = -EINVAL, acqret = -EINVAL;
 	struct xfrm_mgr *km;
 
 	read_lock(&xfrm_km_lock);
 	list_for_each_entry(km, &xfrm_km_list, list) {
-		err = km->acquire(x, t, pol, XFRM_POLICY_OUT);
-		if (!err)
-			break;
+		acqret = km->acquire(x, t, pol, XFRM_POLICY_OUT);
+		if (!acqret)
+			err = acqret;
 	}
 	read_unlock(&xfrm_km_lock);
 	return err;

Re: PATCH: IPSEC acquire in presence of multiple managers

[Herbert Xu]

On Fri, Mar 25, 2005 at 07:12:07PM -0500, jamal wrote:
> 
> Acquire should be supported by both pfkey and netlink.
> However, it stops to send acquire message from the kernel on first
> success.
> It is possible that one or the other manager maybe passively monitoring
> and needs to see those messages.

Yes that's a good catch.

One problem though is that if theal real KM is dead but the passive
monitor is still there then the kernel will have to wait for the
larval states to time out.

It can happen without the patch too if the KM dies after the message
is delivered.  This will make it slightly more likely.

I guess that's something we'll just have to live with.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: PATCH: IPSEC acquire in presence of multiple managers

[jamal]

On Fri, 2005-03-25 at 19:30, Herbert Xu wrote:

> One problem though is that if theal real KM is dead but the passive
> monitor is still there then the kernel will have to wait for the
> larval states to time out.
> 
> It can happen without the patch too if the KM dies after the message
> is delivered.  This will make it slightly more likely.
> 

Agreed. 

> I guess that's something we'll just have to live with.

Well its useful even if we could just run "ip mon" to look at acquires
going across.

If i understood correctly pfkey: the kernel can be told when a KM is
about to die or just came back up using an empty acquire message by the
KM. I dont think we support this at the moment from looking at the code.
It seems that we dont support any acquires from userspace to kernel
which in theory could be triggered by some apps (I saw OSPF;->) trying 
to get a SA. Is this common behavior?

cheers,
jamal

Re: PATCH: IPSEC acquire in presence of multiple managers

[Herbert Xu]

On Fri, Mar 25, 2005 at 07:54:31PM -0500, jamal wrote:
>
> It seems that we dont support any acquires from userspace to kernel

I haven't checked af_key but netlink does support that.  All you have
to do is send messages to the correct multicast group.

Of course whether any of the KMs actually deal with it is a different
story :)
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: PATCH: IPSEC acquire in presence of multiple managers

[jamal]

On Fri, 2005-03-25 at 19:58, Herbert Xu wrote:
> On Fri, Mar 25, 2005 at 07:54:31PM -0500, jamal wrote:
> >
> > It seems that we dont support any acquires from userspace to kernel
> 
> I haven't checked af_key but netlink does support that.  All you have
> to do is send messages to the correct multicast group.
> 
> Of course whether any of the KMs actually deal with it is a different
> story :)

What i have seen being described is as follows:

user space app --> kernel
  acquire with all necessary parameters
kernel --> XFRMGRP_ACQURE
  acquire as it would right now with an outbound packet
some KM -->kernel
  SA ADD/UPD
Kernel ---> XFRMGRP_SA (we are working on this, app hears announce) 
  SA ADD/UPD event 

now the SA is in the kernel user space app could point to it via
some SPD like classifier. 

cheers,
jamal

Re: PATCH: IPSEC acquire in presence of multiple managers

[Herbert Xu]

jamal <hadi@cyberus.ca> wrote:
> 
> What i have seen being described is as follows:
> 
> user space app --> kernel
>  acquire with all necessary parameters
> kernel --> XFRMGRP_ACQURE
>  acquire as it would right now with an outbound packet

You're right.  Neither xfrm_user/af_key is doing this at the moment.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: PATCH: IPSEC acquire in presence of multiple managers

[jamal]

You could say i am obssesed by this aquire message - I dont know why;->

I noticed in the absence of a responsive KM, the acquires are sent
forever. Is it 30 seconds and may be degenerating to 60 seconds?
In the meantime my ping is sitting there not giving me back the prompt.
I suspect this is so as to make it reliable and maybe aggrevated by the
fact i can now passively monitor with ip xfrm mon.

Shouldnt there be a _configurable_ timer and number of retries?
all attempts of reliability at least put an upper limit.
Perhaps the km states could be extended a little? i.e instead of
just ACQUIRING maybe some intermidiate states are needed (sort of like
neighbor discovery or ARP). And when it looks hopeless you just stop.

Is there a standard maybe that defines such behavior?

cheers,
jamal

Re: PATCH: IPSEC acquire in presence of multiple managers

[Herbert Xu]

On Sat, Mar 26, 2005 at 01:41:27PM -0500, jamal wrote:
> 
> I noticed in the absence of a responsive KM, the acquires are sent
> forever. Is it 30 seconds and may be degenerating to 60 seconds?
> In the meantime my ping is sitting there not giving me back the prompt.
> I suspect this is so as to make it reliable and maybe aggrevated by the
> fact i can now passively monitor with ip xfrm mon.

It's only sitting there forever because ping is configured to send
packets forever by default.

Try ping -c 1 and things will probably be different.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: PATCH: IPSEC acquire in presence of multiple managers

[Herbert Xu]

On Sun, Mar 27, 2005 at 05:42:17AM +1000, herbert wrote:
> 
> It's only sitting there forever because ping is configured to send
> packets forever by default.
> 
> Try ping -c 1 and things will probably be different.

Actually it won't.  It will try it forever when the larval state
expires.  When we fix the xfrm state resolution, we can add a
knob for this case.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: PATCH: IPSEC acquire in presence of multiple managers

[jamal]

On Sat, 2005-03-26 at 14:45, Herbert Xu wrote:
> On Sun, Mar 27, 2005 at 05:42:17AM +1000, herbert wrote:
> > 
> > It's only sitting there forever because ping is configured to send
> > packets forever by default.
> > 
> > Try ping -c 1 and things will probably be different.
> 
> Actually it won't.  It will try it forever when the larval state
> expires.  When we fix the xfrm state resolution, we can add a
> knob for this case.

When you get that and need someone to test just post it and i will.

cheers,
jamal

Re: PATCH: IPSEC acquire in presence of multiple managers

[Patrick McHardy]

Herbert Xu wrote:
> On Fri, Mar 25, 2005 at 07:54:31PM -0500, jamal wrote:
> 
>>It seems that we dont support any acquires from userspace to kernel
> 
> 
> I haven't checked af_key but netlink does support that.  All you have
> to do is send messages to the correct multicast group.
> 
> Of course whether any of the KMs actually deal with it is a different
> story :)

af_key implements the second part of RFC2367 §3.1.6, canceling
an acquire request by sending an acquire message to the kernel with
the same sequence number as the initial acquire request. It doesn't
support the third part, acting as KM for userspace.

Regards
Patrick

Resend: Re: PATCH: IPSEC acquire in presence of multiple managers

[jamal]

[-- Attachment #1: Type: text/plain, Size: 112 bytes --]


Apologies, The last patch had some a glitch in the filename. Dave please
apply this one instead

cheers,
jamal

[-- Attachment #2: acq_p --]
[-- Type: text/plain, Size: 709 bytes --]

--- a/net/xfrm/xfrm_state.c	2005/03/25 17:44:43	1.1
+++ b/net/xfrm/xfrm_state.c	2005/03/25 18:55:33
@@ -774,16 +774,20 @@
 		wake_up(&km_waitq);
 }
 
+/*
+ * We send to all registered managers regardless of failure
+ * We are happy with one success
+*/
 int km_query(struct xfrm_state *x, struct xfrm_tmpl *t, struct xfrm_policy *pol)
 {
-	int err = -EINVAL;
+	int err = -EINVAL, acqret = -EINVAL;
 	struct xfrm_mgr *km;
 
 	read_lock(&xfrm_km_lock);
 	list_for_each_entry(km, &xfrm_km_list, list) {
-		err = km->acquire(x, t, pol, XFRM_POLICY_OUT);
-		if (!err)
-			break;
+		acqret = km->acquire(x, t, pol, XFRM_POLICY_OUT);
+		if (!acqret)
+			err = acqret;
 	}
 	read_unlock(&xfrm_km_lock);
 	return err;

Re: Resend: Re: PATCH: IPSEC acquire in presence of multiple managers

[Herbert Xu]

On Sat, Mar 26, 2005 at 01:35:31PM -0500, jamal wrote:
>
>  int km_query(struct xfrm_state *x, struct xfrm_tmpl *t, struct xfrm_policy *pol)
>  {
> -	int err = -EINVAL;
> +	int err = -EINVAL, acqret = -EINVAL;

BTW, you don't need to initialise acqret here since it's only used
inside the loop and is always assigned unconditionally..

> +		acqret = km->acquire(x, t, pol, XFRM_POLICY_OUT);
> +		if (!acqret)
> +			err = acqret;

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Resend: Re: PATCH: IPSEC acquire in presence of multiple managers

[jamal]

On Sat, 2005-03-26 at 14:27, Herbert Xu wrote:
> On Sat, Mar 26, 2005 at 01:35:31PM -0500, jamal wrote:
> >
> >  int km_query(struct xfrm_state *x, struct xfrm_tmpl *t, struct xfrm_policy *pol)
> >  {
> > -	int err = -EINVAL;
> > +	int err = -EINVAL, acqret = -EINVAL;
> 
> BTW, you don't need to initialise acqret here since it's only used
> inside the loop and is always assigned unconditionally..

fair enough. You seem to have no major issues with the patch0 that i
sent. I can submit that instead to Dave with the few tweaks + this
change. Sounds good?

cheers,
jamal

Re: Resend: Re: PATCH: IPSEC acquire in presence of multiple managers

[Herbert Xu]

On Sat, Mar 26, 2005 at 03:13:15PM -0500, jamal wrote:
> 
> fair enough. You seem to have no major issues with the patch0 that i
> sent. I can submit that instead to Dave with the few tweaks + this
> change. Sounds good?

That's fine by me.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Resend: Re: PATCH: IPSEC acquire in presence of multiple managers

[jamal]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

On Sun, 2005-03-27 at 03:21, Herbert Xu wrote:
> On Sat, Mar 26, 2005 at 03:13:15PM -0500, jamal wrote:
> > 
> > fair enough. You seem to have no major issues with the patch0 that i
> > sent. I can submit that instead to Dave with the few tweaks + this
> > change. Sounds good?
> 

Hrm - Ive made a few changes since we last spoke (changed the cb
structure i defined) and i dont want the patch to be huge. 

So Dave if you are reading this apply the reattached patch and i will
work against it from now on.

cheers,
jamal

[-- Attachment #2: acq_p --]
[-- Type: text/plain, Size: 709 bytes --]

--- a/net/xfrm/xfrm_state.c	2005/03/25 17:44:43	1.1
+++ b/net/xfrm/xfrm_state.c	2005/03/25 18:55:33
@@ -774,16 +774,20 @@
 		wake_up(&km_waitq);
 }
 
+/*
+ * We send to all registered managers regardless of failure
+ * We are happy with one success
+*/
 int km_query(struct xfrm_state *x, struct xfrm_tmpl *t, struct xfrm_policy *pol)
 {
-	int err = -EINVAL;
+	int err = -EINVAL, acqret = -EINVAL;
 	struct xfrm_mgr *km;
 
 	read_lock(&xfrm_km_lock);
 	list_for_each_entry(km, &xfrm_km_list, list) {
-		err = km->acquire(x, t, pol, XFRM_POLICY_OUT);
-		if (!err)
-			break;
+		acqret = km->acquire(x, t, pol, XFRM_POLICY_OUT);
+		if (!acqret)
+			err = acqret;
 	}
 	read_unlock(&xfrm_km_lock);
 	return err;

Re: Resend: Re: PATCH: IPSEC acquire in presence of multiple managers

[David S. Miller]

On 26 Mar 2005 13:35:31 -0500
jamal <hadi@cyberus.ca> wrote:

> Apologies, The last patch had some a glitch in the filename. Dave please
> apply this one instead

Doesn't apply, in the current tree km_query() is marked static.

Please regenerate your patch and sorry for not getting to this
sooner.

Re: Resend: Re: PATCH: IPSEC acquire in presence of multiple managers

[jamal]

On Fri, 2005-04-01 at 00:13, David S. Miller wrote:
> On 26 Mar 2005 13:35:31 -0500
> jamal <hadi@cyberus.ca> wrote:
> 
> > Apologies, The last patch had some a glitch in the filename. Dave please
> > apply this one instead
> 
> Doesn't apply, in the current tree km_query() is marked static.
> 
> Please regenerate your patch and sorry for not getting to this
> sooner.

Dave, I am combining this with the other event patch that is under
discussion right now which i will end up sending to you.
If you want it separate i could do that.

cheers,
jamal