[Swan-dev] I think 229e2d24a4 needs to be reverted

Antony Antony 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.

-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
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev


More information about the Swan-dev mailing list