< 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. > 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). -Nathan On Mon, Jul 2, 2018 at 10:14 PM, Eyal Birger <eyal.bir...@gmail.com> wrote: > Hi Nathan, > > On Fri, 29 Jun 2018 15:07:10 -0700 > Nathan Harold <nhar...@google.com> wrote: > >> Allow UPDSA to change "set mark" to permit >> policy separation of packet routing decisions from >> SA keying in systems that use mark-based routing. >> >> The set mark, used as a routing and firewall mark >> for outbound packets, is made update-able which >> allows routing decisions to be handled independently >> of keying/SA creation. To maintain consistency with >> other optional attributes, the set mark is only >> updated if sent with a non-zero value. >> >> The per-SA lock and the xfrm_state_lock are taken in >> that order to avoid a deadlock with >> xfrm_timer_handler(), which also takes the locks in >> that order. >> >> Signed-off-by: Nathan Harold <nhar...@google.com> >> Change-Id: Ia05c6733a94c1901cd1e54eb7c7e237704678d71 >> --- >> net/xfrm/xfrm_state.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c >> index e04a510ec992..c9ffcdfa89f6 100644 >> --- a/net/xfrm/xfrm_state.c >> +++ b/net/xfrm/xfrm_state.c >> @@ -1562,6 +1562,15 @@ int xfrm_state_update(struct xfrm_state *x) >> if (x1->curlft.use_time) >> xfrm_state_check_expire(x1); >> >> + if (x->props.smark.m || x->props.smark.v) { >> + spin_lock_bh(&net->xfrm.xfrm_state_lock); >> + >> + x1->props.smark = x->props.smark; >> + >> + __xfrm_state_bump_genids(x1); > > So I'm trying to wrap my head around this genid thing :) > > 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? > > Is it possible that other states will match all of x1 parameters? > > Also, any idea why this isn't needed for other changes in the state? > > Thanks! > Eyal.
