On Sat, 19 Jun 2021 at 15:35, D. Hugh Redelmeier <h...@mimosa.com> 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: > > 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". > Here's some more (extracted) context: if (endpoint_is_specified(st->st_mobike_local_endpoint)) { char *n = jam_str(text_said, SAMIGTOT_BUF, "initiator migrate kernel SA "); } else { char *n = jam_str(text_said, SAMIGTOT_BUF, "responder migrate kernel SA "); so the code is detailing with a polarity reversal, But wait there's more: sa.spi = proto_info->our_spi; sa.spi = proto_info->attrs.spi; sa.spi = proto_info->our_spi; sa.spi = proto_info->attrs.spi; sa.dst.new_address = endpoint_address(st->st_mobike_local_endpoint); sa.src.new_address = endpoint_address(st->st_mobike_local_endpoint); sa.src.new_address = endpoint_address(st->st_mobike_remote_endpoint); sa.dst.new_address = endpoint_address(st->st_mobike_remote_endpoint); So now we've got a double polarity reversal with an SPI twist. 11 points! The set_text_said() calls seem to match .new_address, so I suspect it is logging the old address. > 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. > Even better, debug logging: char reqid_buf[ULTOT_BUF + 32]; endpoint_buf ra; snprintf(reqid_buf, sizeof(reqid_buf), ":%u to %s reqid=%u %s", old_port, str_endpoint(&new_endpoint, &ra), sa.reqid, enum_name(&netkey_sa_dir_names, dir)); add_str(text_said, SAMIGTOT_BUF, text_said, reqid_buf); dbg("%s", text_said); What about dropping set_text_said() and then dump everything using a single call? Hmm, SAMIGTOT_BUF takes a critical hit, and both add_str() and set_text_said() suffer damage. Personally, I'd be looking at how create_xfrm_migrate_sa() and migrate_xfrm_sa() interact. > 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@lists.libreswan.org > https://lists.libreswan.org/mailman/listinfo/swan-dev >
_______________________________________________ Swan-dev mailing list Swan-dev@lists.libreswan.org https://lists.libreswan.org/mailman/listinfo/swan-dev