[Swan-dev] xfrmi work conflict

[Paul Wouters]

On Fri, 18 Sep 2020, Antony Antony wrote:

>> Note that ikev2-xfrmi-14-fwmark still fails. I think you expect that
>> because the test is in WIP. The fwmark that is commited in test output
>> is fwmark 0x1000000 but the test actually only uses fwmark 0x1. So I am
>> not sure what the idea is.
>
> from a quick look add
> https://github.com/antonyantony/libreswan/commit/799dfa935aef3d366021bfea6004766359921b1e
> I think you missed this commit. May be the commit message was misleading.

Thanks, I did miss the changes there. I probably didn't look beyond the
update to TESTLIST, assuming that would be its own commit and not mixed
with others. I pushed the change. Thanks.

> thanks for the quick  merge.
> I was getting confused with too many rapid changes in the code and not able
> to catch up with testruns on my branch.

Sorry that things have been moving fast in the last few weeks. We are
really urgently trying to get 4.0 out the door. Unfortunately, it seems
I'm the only one looking at the test cases that are failing to identiy
release blockers, write fixes, etc and moving this forward. It has been
a very slow number of weeks for me with this work.

> I have an UNTESTED fix to the merge and your commit f965623b7. See the
> attached patch.

It seems you did not see my comment on this: https://github.com/antonyantony/libreswan/commit/092475c8adb14c5c136d246c2924b5e792ab6940#commitcomment-42412007

> I am too shocked to test it at the moment. May be I will
> test it later! May be there are more places xfrm_if_id zero is assumed "no"
> that need fixing. In hindsight I should have used yn_no instead of 0 every
> where!

No. I think you are mixing up the configuration keyword ipsec-interface= values
with the connection's if_id value?

> A minor nit on commit meesages. git object reference url in commit message
> will disappear and commit message would look confusing. I discourage
> transient url in commit messsage for the future.

I tried the take in commits unmodified where possible so the author
atribute would be correct. For those that needed small changes, I wanted
to link back to the original author as best I could, which is why I used
this method. In the past, I had either used the original author's name
or my own author's name and neither were well received. I'm trying to
avoid the "linux kernel" issue of needing to artisanally craft and
re-craft email messages for patches before they are applied, as that
would take weeks or months to resolve. I don't know how to pull in your
code in a way that you approve.

> I also have non echnical issue with f965623b7. I am personally shocked by
> this commit.

I think you mean another commit related to ipsec0? Commit f965623b7
is a commit by you ?

> I feel as it is PLUTO_XFRMI_REMAP_IF_ID_ZERO is a horrible hack and my
> predication is it is more pain than what it is worth.

The "worth" is that it became clear that for people to move from KLIPS
to XFRMi, they needed an interface name ipsec0. This turned out to be
vitally imported for people to even try and migrate away from KLIPS. So
the worth is very high.

I agree it is a hack. It is a hack upon a bad enum hack as well. I was
strongly opposed to using an enum for both strings "yes" and "no" and
for integers. I already had to bandaid a libipsec passert() to make this
work to begin with. It was your insistence on ipsec-inteface=yes working
that required the enum hack. I ensured that the libipseconf keyword
values yn_no/yn_yes are constrained to the ipsecconf code, and do not
leak into pluto itself.

As for the actual mapping of ipsec0 name to an if_id that is not 0,
there is not much choice if we want to support the ipsec0 name without
letting users fully set their own names - which you wanted to avoid and
why we ended up on the ipsec-inteface= option. I am not invested in the
way of this hack and the specific PLUTO_XFRMI_REMAP_IF_ID_ZERO number.
If there is a better way to doing the remapping, I am fine with it. But
a remap of some kind has to be there becuse if_id == 0 cannot be used.
I did test it, just to make sure behaviour of not setting if_id as
netlink attribute acted the same as setting it to 0 and passing it to
the kernel.

> To me this commit and the merge look poorly tested.

I changed ikev2-xfrmi-02 specifically to use ipsec0 and ensure we keep
testing it in the future. If there are side effects and there is a
better remapping method, I'm happy to do this. It would likely be for
4.1 unless you have something ready this weekend.

> May be there is a need
> for quick resolution under time pressure. That is why I am sending untested
> patch for review and further testing than commiting it.

I don't see the change of yn_no for 0 to be needed or useful. I
explained in the above linked commit comment and above in this email.
Please clarify if you think this is a bug and explain what would go
wrong. Note again that UINT32_MAX is not used to set the if_id. It
does not leave the parser/whack_message section of the code. The
ipsec-interface=no goes via whack message xfrm_if_id UINT32_MAX to
disabled, while ipsec-interface=0 goes via xfrm_if_id 0 to the remap
in connections.c so that kernel_xfrm* code doesn't need to be aware
of anything other than the "name remap" for the special ipsec0 name.


> Now the commit f965623b7 is in main I feel the table turned around. I have
> to do the work disproving while I feel you should have tested more and/or
> asked feedback before commiting it.

> In an ideal world my preference is revert f965623b7, take closer look at
> supporting ipsec0, add test cases discuss it before adding support for
> ipsec0.  Git revert may be harder now, atleast revert that feture!

I did add a test case, and that passes. All other unchanged test
cases that passed, still passed. I did do my testing. If I broke more
complicated setups, which is surely possible, then those scenario's had
no test cases.

You don't have to disprove my patch for me. If you have a better
solution, it can be updated. But postponing "ipsec0" support for 4.0
is the worst scenario in my view and we also cannot postpone 4.0 anymore
either as we have 5 different groups bugging us almost daily to see the
things they need in a full release (f34 s390x compile failure, TCP
support, rawhide, OSP left/rightikeport, PLUTO_BYTES_ fixup, whack
--rereadcerts deployments)

Paul