Re: [PATCH]: double SAs are created when using AH and ESP together
From: Joy Latten [EMAIL PROTECTED] Date: Fri, 9 Mar 2007 17:14:54 -0600 I noticed that in xfrm_state_add we look for the larval SA in a few places without checking for protocol match. So when using both AH and ESP, whichever one gets added first, deletes the larval SA. It seems AH always gets added first and ESP is always the larval SA's protocol since the xfrm-tmpl has it first. Thus causing the additional km_query() Adding the check eliminates the double SA creation. I know this may not seem like a complete solution and I will continue to test and be on the lookout, but isn't having the check a good thing? So far I have tested SAs with just ESP, just AH and with both and all seems ok. Please let me know if this patch is ok. My kernel was 2.6.20-rc3-git3. Signed-off-by: Joy Latten [EMAIL PROTECTED] This patch looks good, I'll push this to Linus and -stable. Thanks! - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH]: double SAs are created when using AH and ESP together
On Tue, 2007-03-06 at 14:40 -0500, James Morris wrote:
On Tue, 6 Mar 2007, Joy Latten wrote:
I saw something similar to this some time ago when testing various
failure modes, and discused it with Herbert.
IIRC, there's a larval SA which is not torn down properly by Racoon once
the full SA is established, and the larval SA keeps resending until it
times out.
Ok, good to know.
I thought a bit more about this last night but am not
sure best way to fix it. Perhaps a way to keep larval
SA around until all SAs resulting from xfrm_vec[xfrm_nr]
are established... oh well, just thinking out loud... :-)
I think the solution, if this actually the problem, is for the userland
code to maintain the SAs.
Gotta agree. :-)
I noticed that in xfrm_state_add we look for the larval SA in a few
places without checking for protocol match. So when using both
AH and ESP, whichever one gets added first, deletes the larval SA.
It seems AH always gets added first and ESP is always the larval
SA's protocol since the xfrm-tmpl has it first. Thus causing the
additional km_query()
Adding the check eliminates the double SA creation.
I know this may not seem like a complete solution and I will
continue to test and be on the lookout, but isn't having the
check a good thing? So far I have tested SAs with just ESP, just AH
and with both and all seems ok.
Please let me know if this patch is ok.
My kernel was 2.6.20-rc3-git3.
Joy
Signed-off-by: Joy Latten [EMAIL PROTECTED]
diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_state.c
linux-2.6.20.patch/net/xfrm/xfrm_state.c
--- linux-2.6.20.orig/net/xfrm/xfrm_state.c 2007-03-08 17:39:14.0
-0600
+++ linux-2.6.20.patch/net/xfrm/xfrm_state.c2007-03-09 11:03:25.0
-0600
@@ -704,7 +704,8 @@ static struct xfrm_state *__find_acq_cor
x-props.mode != mode ||
x-props.family != family ||
x-km.state != XFRM_STATE_ACQ ||
- x-id.spi != 0)
+ x-id.spi != 0 ||
+ x-id.proto != proto)
continue;
switch (family) {
@@ -801,7 +802,8 @@ int xfrm_state_add(struct xfrm_state *x)
if (use_spi x-km.seq) {
x1 = __xfrm_find_acq_byseq(x-km.seq);
- if (x1 xfrm_addr_cmp(x1-id.daddr, x-id.daddr, family)) {
+ if (x1 ((x1-id.proto != x-id.proto) ||
+ xfrm_addr_cmp(x1-id.daddr, x-id.daddr, family))) {
xfrm_state_put(x1);
x1 = NULL;
}
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: double SAs are created when using AH and ESP together
On Fri, 2007-03-09 at 16:20 -0800, David Miller wrote: From: Joy Latten [EMAIL PROTECTED] Date: Fri, 9 Mar 2007 17:14:54 -0600 I noticed that in xfrm_state_add we look for the larval SA in a few places without checking for protocol match. So when using both AH and ESP, whichever one gets added first, deletes the larval SA. It seems AH always gets added first and ESP is always the larval SA's protocol since the xfrm-tmpl has it first. Thus causing the additional km_query() Adding the check eliminates the double SA creation. I know this may not seem like a complete solution and I will continue to test and be on the lookout, but isn't having the check a good thing? So far I have tested SAs with just ESP, just AH and with both and all seems ok. Please let me know if this patch is ok. My kernel was 2.6.20-rc3-git3. Signed-off-by: Joy Latten [EMAIL PROTECTED] Generally it looks OK, but I'm going to let this one sit for a while before I apply it so that other folks can review it too and spot any unintended consequences. In particular, I find it strance that we didn't check the protocol field all this time and I wonder whether that might be on purpose for some reason. At least the first hunk of this patch used to be checked back in net/ipv4/xfrm4_state.c in __xfrm4_find_acq and looks like it just was accidentally forgotten when there was a transition to using __find_acq_core http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=2770834c9f44afd1bfa13914c7285470775af657 Since Joy found this problem on a 2.6.18 kernel originally which was before this diff and had the proto check I'm guessing it is actually the second hunk which is more relevant to the problem. -Eric - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: double SAs are created when using AH and ESP together
On Fri, 2007-03-09 at 16:20 -0800, David Miller wrote: From: Joy Latten [EMAIL PROTECTED] Date: Fri, 9 Mar 2007 17:14:54 -0600 I noticed that in xfrm_state_add we look for the larval SA in a few places without checking for protocol match. So when using both AH and ESP, whichever one gets added first, deletes the larval SA. It seems AH always gets added first and ESP is always the larval SA's protocol since the xfrm-tmpl has it first. Thus causing the additional km_query() Adding the check eliminates the double SA creation. I know this may not seem like a complete solution and I will continue to test and be on the lookout, but isn't having the check a good thing? So far I have tested SAs with just ESP, just AH and with both and all seems ok. Please let me know if this patch is ok. My kernel was 2.6.20-rc3-git3. Signed-off-by: Joy Latten [EMAIL PROTECTED] Generally it looks OK, but I'm going to let this one sit for a while before I apply it so that other folks can review it too and spot any unintended consequences. In particular, I find it strance that we didn't check the protocol field all this time and I wonder whether that might be on purpose for some reason. Ok. And I'll continue to test, though most of my testing is done on lspp kernel. lspp kernel is an earlier version of upstream kernel and I noticed the old __xfrm4_find_acq() did include a check for the protocol, new __find_acq_core() doesn't. But neither checked for protocol after call to __xfrm_find_acq_byseq(). Joy - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: double SAs are created when using AH and ESP together
On Fri, 2007-03-09 at 19:54 -0500, Eric Paris wrote: On Fri, 2007-03-09 at 16:20 -0800, David Miller wrote: From: Joy Latten [EMAIL PROTECTED] Date: Fri, 9 Mar 2007 17:14:54 -0600 I noticed that in xfrm_state_add we look for the larval SA in a few places without checking for protocol match. So when using both AH and ESP, whichever one gets added first, deletes the larval SA. It seems AH always gets added first and ESP is always the larval SA's protocol since the xfrm-tmpl has it first. Thus causing the additional km_query() Adding the check eliminates the double SA creation. I know this may not seem like a complete solution and I will continue to test and be on the lookout, but isn't having the check a good thing? So far I have tested SAs with just ESP, just AH and with both and all seems ok. Please let me know if this patch is ok. My kernel was 2.6.20-rc3-git3. Signed-off-by: Joy Latten [EMAIL PROTECTED] Generally it looks OK, but I'm going to let this one sit for a while before I apply it so that other folks can review it too and spot any unintended consequences. In particular, I find it strance that we didn't check the protocol field all this time and I wonder whether that might be on purpose for some reason. At least the first hunk of this patch used to be checked back in net/ipv4/xfrm4_state.c in __xfrm4_find_acq and looks like it just was accidentally forgotten when there was a transition to using __find_acq_core http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=2770834c9f44afd1bfa13914c7285470775af657 Since Joy found this problem on a 2.6.18 kernel originally which was before this diff and had the proto check I'm guessing it is actually the second hunk which is more relevant to the problem. -Eric Don't mean to be a killjoy on a friday evening, but I just saw something else that I recall seeing a while back but forgot and so want to report now. While initiator is establishing SAs, the responder sends an ACQUIRE. I once got 3 sets of SAs. Now I only got 2 this time with above patch. This occurs when I start netperf to send streams of tcp and udp packets to test. This doesn't always happen. Just once in a while it seems... like a packet escaped and reached remote... (this is from lspp kernel) Initiator's log file: Mar 9 19:43:51 racoon: INFO: ISAKMP-SA established 9.3.192.210[500]-9.3.189.55[500] spi:dcdf529f9e7e1c7b:87400b8505e2f2aa Mar 9 19:43:52 racoon: INFO: initiate new phase 2 negotiation: 9.3.192.210[500]=9.3.189.55[500] Mar 9 19:43:53 racoon: INFO: IPsec-SA established: AH/Transport 9.3.189.55[0]-9.3.192.210[0] spi=193987518(0xb9003be) Mar 9 19:43:53 racoon: INFO: IPsec-SA established: ESP/Transport 9.3.189.55[0]-9.3.192.210[0] spi=137973961(0x83950c9) Mar 9 19:43:53 racoon: INFO: IPsec-SA established: AH/Transport 9.3.192.210[0]-9.3.189.55[0] spi=248476422(0xecf7306) Mar 9 19:43:53 racoon: INFO: IPsec-SA established: ESP/Transport 9.3.192.210[0]-9.3.189.55[0] spi=263395735(0xfb31997) Mar 9 19:43:53 racoon: INFO: respond new phase 2 negotiation: 9.3.192.210[500]=9.3.189.55[500] Mar 9 19:43:55 racoon: INFO: IPsec-SA established: AH/Transport 9.3.189.55[0]-9.3.192.210[0] spi=37664464(0x23eb6d0) Mar 9 19:43:55 racoon: INFO: IPsec-SA established: ESP/Transport 9.3.189.55[0]-9.3.192.210[0] spi=246992333(0xeb8cdcd) Mar 9 19:43:55 racoon: INFO: IPsec-SA established: AH/Transport 9.3.192.210[0]-9.3.189.55[0] spi=203651374(0xc23792e) Mar 9 19:43:55 racoon: INFO: IPsec-SA established: ESP/Transport 9.3.192.210[0]-9.3.189.55[0] spi=48093292(0x2ddd86c) -- Responder's log file: Mar 9 19:43:10 racoon: INFO: respond new phase 2 negotiation: 9.3.189.55[500]=9.3.192.210[500] Mar 9 19:43:11 racoon: INFO: IPsec-SA established: AH/Transport 9.3.192.210[0]-9.3.189.55[0] spi=248476422(0xecf7306) Mar 9 19:43:11 racoon: INFO: IPsec-SA established: ESP/Transport 9.3.192.210[0]-9.3.189.55[0] spi=263395735(0xfb31997) Mar 9 19:43:11 racoon: INFO: IPsec-SA established: AH/Transport 9.3.189.55[0]-9.3.192.210[0] spi=193987518(0xb9003be) Mar 9 19:43:11 racoon: INFO: security context doi: 1 Mar 9 19:43:11 racoon: INFO: security context algorithm: 1 Mar 9 19:43:11 racoon: INFO: security context length: 39 Mar 9 19:43:11 racoon: INFO: security context: root:sysadm_r:sysadm_t:s0-s15:c0.c1023 Mar 9 19:43:11 racoon: INFO: initiate new phase 2 negotiation: 9.3.189.55[500]=9.3.192.210[500] Mar 9 19:43:11 racoon: INFO: IPsec-SA established: ESP/Transport 9.3.189.55[0]-9.3.192.210[0] spi=137973961(0x83950c9) Mar 9 19:43:12 racoon: INFO: IPsec-SA established: AH/Transport 9.3.192.210[0]-9.3.189.55[0] spi=203651374(0xc23792e) Mar 9 19:43:12 racoon: INFO: IPsec-SA established: ESP/Transport 9.3.192.210[0]-9.3.189.55[0] spi=48093292(0x2ddd86c) Mar 9 19:43:12 racoon: INFO: IPsec-SA established:
