Re: [PATCH ipsec-next] xfrm: Allow Set Mark to be Updated Using UPDSA

[Eyal Birger <eyal.birger@gmail.com>]

On Mon, 16 Jul 2018 15:27:26 -0700
Nathan Harold <nharold@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.birger@gmail.com>

Thanks!
Eyal.