[Swan-dev] text_said() calls in create_xfrm_migrate_sa() puzzle me

[Antony Antony]

Hugh,

you spotted a bug in debug output.

I think the idea is to log <spi>@<old addres> <new address> reqid=<reqid>.
either dst or src would change. I also recollect trying to log the ports 
when there is encap.

debug output is in:
https://testing.libreswan.org/v4.4-483-g292ec75828-main/ikev2-mobike-05-gcm/OUTPUT/north.pluto.log.gz
Jun 19 16:14:55.733085: | initiator migrate kernel SA esp.626c3e9e at 192.1.3.33:500 to 192.1.8.22:500 reqid=16389 XFRM_OUT
Jun 19 16:14:55.733219: | TCP/NAT: encap type 0(none)
Jun 19 16:14:55.733283: | initiator migrate kernel SA esp.792cc8f at 192.1.3.33:500 to 192.1.8.22:500 reqid=16389 XFRM_IN
Jun 19 16:14:55.733376: | TCP/NAT: encap type 0(none)
Jun 19 16:14:55.733450: | initiator migrate kernel SA esp.792cc8f at 192.1.3.33:500 to 192.1.8.22:500 reqid=16389 XFRM_FWD

IP and ports should match output in "ip xfrm" both state and policy.

https://testing.libreswan.org/v4.4-483-g292ec75828-main/ikev2-mobike-05-gcm/OUTPUT/north.console.verbose.txt

-antony

On Sat, Jun 19, 2021 at 03:34:56PM -0400, D. Hugh Redelmeier wrote:
> I don't understand this code but I'm "improving" it, based on symmetry.
> I've hit a snag.  Advice welcome!
> 
> There are four calls to text_said.  Here's what they look like, with
> details removed:
> 
> 	if (endpoint_is_specified(st->st_mobike_local_endpoint)) {
> 		if (dir == XFRM_POLICY_IN || dir == XFRM_POLICY_FWD) {
> 			src = &c->spd.that.host_addr;
> 			dst = &c->spd.this.host_addr;
> 			set_text_said(n, dst, sa.spi, proto);
> 		} else {
> 			src = &c->spd.this.host_addr;
> 			dst = &c->spd.that.host_addr;
> 			set_text_said(n, src, sa.spi, proto);
> 		}
> 	} else {
> 		if (dir == XFRM_POLICY_IN || dir == XFRM_POLICY_FWD) {
> 			src = &c->spd.that.host_addr;
> 			dst = &c->spd.this.host_addr;
> 			set_text_said(n, src, sa.spi, proto);
> 		} else {
> 			src = &c->spd.this.host_addr;
> 			dst = &c->spd.that.host_addr;
> 			set_text_said(n, dst, sa.spi, proto);
> 			}
> 		}
> 	}
> 
> I'm a little surprised and puzzled that the second args to
> set_text_said calls go
> 	dst
> 	src
> 	src
> 	dst
> My intuition says that these should each be "dst".
> 
> Even if that's not correct, it feels as if the either the first pair
> or the second pair is reversed.
> 
> Note: the text_said only appears in logging so being wrong has no
> serious consequences.
> 
> Observations:
> 
> If !(dir == XFRM_POLICY_IN || dir == XFRM_POLICY_FWD)
> then dir == XFRM_POLICY_OUT.
> 
> Kernel IPSec SA's are unidirectional.
> The SAID includes the destination address of packets carried by the SA.
> I think that in each case, the destination address is "dst".
> 
> There are currently three calls to this function, all within one 
> statement of the function netlink_migrate_sa:
> 	return
> 		create_xfrm_migrate_sa(st, XFRM_POLICY_OUT, &sa, mig_said) &&
> 		migrate_xfrm_sa(&sa, st->st_logger) &&
> 
> 		create_xfrm_migrate_sa(st, XFRM_POLICY_IN, &sa, mig_said) &&
> 		migrate_xfrm_sa(&sa, st->st_logger) &&
> 
> 		create_xfrm_migrate_sa(st, XFRM_POLICY_FWD, &sa, mig_said) &&
> 		migrate_xfrm_sa(&sa, st->st_logger);
> 
> 
> dir is the second argument of create_xfrm_migrate_sa.  Clearly it can
> only have one of three values: XFRM_POLICY_OUT, XFRM_POLICY_IN,
> XFRM_POLICY_FWD.  So the test of dir could be simplified to
> 	dir != XFRM_POLICY_OUT
> 
> Each IPSec SA is unidirectional.  That's why they come in pairs, one
> for each direction.
> 
> The SAID is a triple: destination IP, SPI, protocol (eg. AH or ESP).
> 
> The test dir == XFRM_POLICY_IN || dir == XFRM_POLICY_FWD must be
> telling us whether we're dealing with an inbound SA.  If it's true, it
> must be inbound.
> _______________________________________________
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev