[Swan-dev] I think 229e2d24a4 needs to be reverted
antony at phenome.org
Wed May 8 10:02:57 UTC 2019
I am not commenting on 229e2d24a4, instead the line of argument you made.
your statement sounds a bit scary to me. If true it would mean we went back
to stone ages!
Why do you say - "stupid thing of IKEv2 using STATE_PARENT_I3 and
STATE_PARENT_R2 for both IKE and CHILD SA" ?
The states STATE_PARENT_I3 STATE_PARENT_R2 are not shared between IKE and
Child SA state since the following commits.
svm entry change e58685b2b8
Added constant fa3ecaa60
The terminal states of Child SA are STATE_V2_IPSEC_I STATE_V2_IPSEC_R.
You can also read this on our Wikipage.
I am bit skeptical about your assertion. May be worth checking the facts
before using this line of argument. The issue you spotted after 229e2d24a4
could be real, I haven't looked into it yet.
Note: My reaction is based on an older release 3.21-3.22. May be after the
code churn since 3.21, we are now calling both IKE and Child state
STATE_PARENT_I3 STATE_PARENT_R2. This would be sad:) However, looking at
ikev2.c it is less likely.
NOTE as far as I recollect - there one place where parent and child share
same state name - only on the initiator for STATE_PARENT_I2, it is only for
short while. Then it become STATE_V2_IPSEC_I. I haven trying to find a quiet
period, test results are stable for a week or so, to change this. See the
wiki page for the propsed change. May be you are confused because of this.
PS: no inline comments bellow.
On Tue, May 07, 2019 at 09:22:19PM -0400, Paul Wouters wrote:
> -#define IS_IKE_SA(st) ( ((st)->st_clonedfrom == SOS_NOBODY) && \
> - (IS_PHASE1((st)->st_state) || IS_PHASE15((st)->st_state) ||
> IS_PARENT_SA(st)) )
> +#define IS_IKE_SA(st) ((st)->st_clonedfrom == SOS_NOBODY)
> The idea here is that the normal case for checking IS_IKE_SA() depends
> only on the state it is in.
> Because we had that stupid thing of IKEv2 using STATE_PARENT_I3 and
> STATE_PARENT_R2 for both IKE and CHILD SA, we had to work around a
> lot of things, and the clonedfrom check was added. But this is _wrong_
> long term, because we want to clone states for IKE too. Cloning _should_
> not be the actual check for if a state is parent or child.
> So you made a workaround bandaid the actual sole solution, instead of
> something that can later be removed again.
> Also, I am very concerned this has unknown side effects, because you
> reduced the check to no longer care about any specific phase 1 states.
> Especially so close to 3.28, I am really concerned about this change.
> I do not think your change is required for anything, other than maybe
> doing a tiny optimalization for now, so I see no important gains from
> this commit.
> This commit really should be reverted at this time.
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
More information about the Swan-dev