| Message ID | 20180629220710.190783-1-nharold@google.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [ipsec-next] xfrm: Allow Set Mark to be Updated Using UPDSA | expand |
Hi Nathan, On Fri, 29 Jun 2018 15:07:10 -0700 Nathan Harold <nharold@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 <nharold@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.
< 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.birger@gmail.com> wrote: > Hi Nathan, > > On Fri, 29 Jun 2018 15:07:10 -0700 > Nathan Harold <nharold@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 <nharold@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.
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.
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); + spin_unlock_bh(&net->xfrm.xfrm_state_lock); + } + err = 0; x->km.state = XFRM_STATE_DEAD; __xfrm_state_put(x);
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 <nharold@google.com> Change-Id: Ia05c6733a94c1901cd1e54eb7c7e237704678d71 --- net/xfrm/xfrm_state.c | 9 +++++++++ 1 file changed, 9 insertions(+)