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. -antony 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. > > Paul > _______________________________________________ > Swan-dev mailing list > [email protected] > https://lists.libreswan.org/mailman/listinfo/swan-dev _______________________________________________ Swan-dev mailing list [email protected] https://lists.libreswan.org/mailman/listinfo/swan-dev
