On Mon, 16 Jul 2018 15:27:26 -0700 Nathan Harold <nhar...@google.com> wrote:
> < re-sent with apologies due to incorrect formatting last > time... :-( > > > Hi Eyal, > > > If x1 points to a state previously found using > > __xfrm_state_locate(x), won't __xfrm_state_bump_genids(x1) be > > equivalent to x1->genid++ in this case? > > In the vanilla case this is true. IE, if there are no strange/abusive > uses of the API such as the test below where multiple SAs can match > the locate(). > > > Is it possible that other states will match all of x1 parameters? > > Yes. Not sure if it's a bug or a feature, but it's possible for > multiple SAs to match... for a depressing example, check out > https://android-review.googlesource.com/c/kernel/tests/+/680958. There > may be cases where something like this is desired behavior that I'm > not aware of. Since this is control path, it felt to me like the > formalism of using the xfrm_state_bump_genids() was worth not possibly > walking into a different subtle bug later. Ok. This is indeed depressing and also unexpected. I wonder if this behavior could be fixed... I'd find it odd if anyone is relying on being to able to delete a 'no mark' state by supplying parameters that do include an explicit mark. I have no idea if anyone is relying on the state insertion order wrt marks - though it would seem odd to me as well -- obviously such a change is unrelated to this patch. I now better understand the need to be cautious. > > > Also, any idea why this isn't needed for other changes in the > > state? > > The set_mark (output_mark) is somewhat special because changing this > mark impacts the routing lookup, which up to now, none of the other > parameters in the update_sa function do. A new output_mark can and > will reroute packets to different interfaces. Thus, when we change > this thing, we want to ensure that we always build a new bundle with a > new bundle with a new route lookup based on the new set_mark. Since we > removed the flow cache, things might *incidentally* seem to work right > now; but, I think that's incidental rather than correct. By bumping > the genid, we get the dst_entry->check() function to correctly return > that the dst is obsolete when we call check(). I'm honestly not sure > what corner cases we could land in if we didn't bump the genid in such > a case. > > There's definitely a lot going on behind the scenes in this little > change that I only tenuously grasp, so it's possible that I'm being > overly cautious in this case. Please let me know your further thoughts > on whether we need to bump the genid. FYI once this patch is settled, > I plan to upload a patch to update the xfrm_if_id, which I planned to > nestle in to this same logic (and with similar, albeit possibly > more-straightforward rationale). Thanks so much for the clarification. Indeed there are nuances here and I appreciate you taking the time to describe them. FWIW you can add my: Reviewed-by: Eyal Birger <eyal.bir...@gmail.com> Thanks! Eyal.