<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, 19 Jun 2021 at 15:35, D. Hugh Redelmeier <<a href="mailto:hugh@mimosa.com" target="_blank">hugh@mimosa.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I don't understand this code but I'm "improving" it, based on symmetry.<br>
I've hit a snag.  Advice welcome!<br>
<br>
There are four calls to text_said.  Here's what they look like, with<br>
details removed:<br></blockquote><div><br></div></div><div class="gmail_quote"><br></div><div class="gmail_quote"><br></div><div class="gmail_quote"><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I'm a little surprised and puzzled that the second args to<br>
set_text_said calls go<br>
        dst<br>
        src<br>
        src<br>
        dst<br>
My intuition says that these should each be "dst".<br></blockquote></div><div class="gmail_quote"><br></div><div class="gmail_quote">Here's some more (extracted) context:<br></div><div class="gmail_quote"><div><div class="gmail_quote"><br></div><div class="gmail_quote">        if (endpoint_is_specified(st->st_mobike_local_endpoint)) {</div><div class="gmail_quote"><div>                char *n = jam_str(text_said, SAMIGTOT_BUF, "initiator migrate kernel SA ");</div><div>      } else {<br></div><div>                char *n = jam_str(text_said, SAMIGTOT_BUF, "responder migrate kernel SA ");<br></div><div><br></div><div>so the code is detailing with a polarity reversal,  But wait there's more:<br></div><div><br></div><div>                        sa.spi = proto_info->our_spi;</div><div>                        sa.spi = proto_info->attrs.spi;</div><div>                        sa.spi = proto_info->our_spi;</div><div>                        sa.spi = proto_info->attrs.spi;</div><div><br></div><div>                        sa.dst.new_address = endpoint_address(st->st_mobike_local_endpoint);<br></div>                        sa.src.new_address = endpoint_address(st->st_mobike_local_endpoint);</div><div class="gmail_quote">                        sa.src.new_address = endpoint_address(st->st_mobike_remote_endpoint);</div><div class="gmail_quote">                        sa.dst.new_address = endpoint_address(st->st_mobike_remote_endpoint);</div><div class="gmail_quote"><br></div><div class="gmail_quote">So now we've got a double polarity reversal with an SPI twist.  11 points!</div><div class="gmail_quote"><br></div><div class="gmail_quote">The set_text_said() calls seem to match .new_address, so I suspect it is logging the old address.</div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Even if that's not correct, it feels as if the either the first pair<br>
or the second pair is reversed.<br>
<br>
Note: the text_said only appears in logging so being wrong has no<br>
serious consequences.<br></blockquote><div><br></div><div>Even better, debug logging:<br></div><div><br></div><div>        char reqid_buf[ULTOT_BUF + 32];<br>        endpoint_buf ra;<br>        snprintf(reqid_buf, sizeof(reqid_buf), ":%u to %s reqid=%u %s",<br>                        old_port,<br>                 str_endpoint(&new_endpoint, &ra),<br>                        sa.reqid,<br>                        enum_name(&netkey_sa_dir_names, dir));<br>        add_str(text_said, SAMIGTOT_BUF, text_said, reqid_buf);<br>        dbg("%s", text_said);<br></div><div><br></div><div> 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.<br></div><div>Personally, I'd be looking at how create_xfrm_migrate_sa() and migrate_xfrm_sa() interact.<br></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Observations:<br>
<br>
If !(dir == XFRM_POLICY_IN || dir == XFRM_POLICY_FWD)<br>
then dir == XFRM_POLICY_OUT.<br>
<br>
Kernel IPSec SA's are unidirectional.<br>
The SAID includes the destination address of packets carried by the SA.<br>
I think that in each case, the destination address is "dst".<br>
<br>
There are currently three calls to this function, all within one <br>
statement of the function netlink_migrate_sa:<br>
        return<br>
                create_xfrm_migrate_sa(st, XFRM_POLICY_OUT, &sa, mig_said) &&<br>
                migrate_xfrm_sa(&sa, st->st_logger) &&<br>
<br>
                create_xfrm_migrate_sa(st, XFRM_POLICY_IN, &sa, mig_said) &&<br>
                migrate_xfrm_sa(&sa, st->st_logger) &&<br>
<br>
                create_xfrm_migrate_sa(st, XFRM_POLICY_FWD, &sa, mig_said) &&<br>
                migrate_xfrm_sa(&sa, st->st_logger);<br>
<br>
<br>
dir is the second argument of create_xfrm_migrate_sa.  Clearly it can<br>
only have one of three values: XFRM_POLICY_OUT, XFRM_POLICY_IN,<br>
XFRM_POLICY_FWD.  So the test of dir could be simplified to<br>
        dir != XFRM_POLICY_OUT<br>
<br>
Each IPSec SA is unidirectional.  That's why they come in pairs, one<br>
for each direction.<br>
<br>
The SAID is a triple: destination IP, SPI, protocol (eg. AH or ESP).<br>
<br>
The test dir == XFRM_POLICY_IN || dir == XFRM_POLICY_FWD must be<br>
telling us whether we're dealing with an inbound SA.  If it's true, it<br>
must be inbound.<br>
_______________________________________________<br>
Swan-dev mailing list<br>
<a href="mailto:Swan-dev@lists.libreswan.org" target="_blank">Swan-dev@lists.libreswan.org</a><br>
<a href="https://lists.libreswan.org/mailman/listinfo/swan-dev" rel="noreferrer" target="_blank">https://lists.libreswan.org/mailman/listinfo/swan-dev</a><br>
</blockquote></div></div>