Message ID | 20180509204626.56561-1-nharold@google.com |
---|---|
State | Awaiting Upstream, archived |
Delegated to: | David Miller |
Headers | show |
Series | [ipsec-next] xfrm: Allow Output Mark to be Updated Using UPDSA | expand |
Hi Nathan, On Wed, 9 May 2018 13:46:26 -0700 Nathan Harold <nharold@google.com> wrote: > Allow UPDSA to change output_mark to permit > policy separation of packet routing decisions from > SA keying in systems that use mark-based routing. > > In the output_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 output mark is only > updated if sent with a non-zero value. Once set, the > output mark may not be reset to zero, which ensures > that updating the SA does not require the mark to > be re-sent to avoid the value being clobbered. There is an attempt to extend the 'output_mark' to support the input direction and masking. In the proposed implementation, output_mark is converted to type 'struct xfrm_mark' where the semantics are as follows: - If mark is given by XFRMA_OUTPUT_MARK (renamed to XFRMA_SET_MARK) then a new XFRMA_SET_MARK_MASK attribute is consulted to set the mask value - if no XFRMA_SET_MARK_MASK attribute is provided, the mask is set to 0xffffffff Therefore, if the mask value is 0, we can regard the mark as 'not given'. My question is, in the context of this patch, it seems that the "Once set, the output mark may not be reset to zero" restriction may be lifted in favor of updating the mark only if the new mask is non zero. Does this make sense to you? Eyal
That makes sense to me; the restriction about which you inquire is a practical one rather than a philosophical one, which I will be happy to see lifted. With the new set_mark, a non-zero mask will indicate that the caller has a set an "explicit" zero mark, which sidesteps the currently-ambiguous situation; the logic can then become "if (set_mark || set_mark_mask) { // update mark and update mask}". There is a question of the behavior for a caller who sets a set_mark and set_mark_mask, then subsequently calls UPDSA with only a mark (omitting the mask, or with explicit set_mask == 0). I think it's fair and appropriate the mask be re-set to 0xFFFFFFFF (to avoid the special-case of (if new_set_mask == 0 && set_mask != 0xFFFFFFFF). Of course, this means that the inability to return to zero limitation that I currently mention as being on the output_mark would transfer under that proposal to the set_mark_mask. All of this is fix-able by having the update take into account the presence or absence of the XFRMAs sent rather than just looking at a built xfrm_state, but I'm couldn't fathom any use cases for reverting the mark scheme back to an "unused" state while the SA remains ACTIVE, so I think simpler is better (same reasoning applied to the current change). -Nathan On Wed, May 9, 2018 at 10:44 PM, Eyal Birger <eyal.birger@gmail.com> wrote: > Hi Nathan, > > On Wed, 9 May 2018 13:46:26 -0700 > Nathan Harold <nharold@google.com> wrote: > >> Allow UPDSA to change output_mark to permit >> policy separation of packet routing decisions from >> SA keying in systems that use mark-based routing. >> >> In the output_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 output mark is only >> updated if sent with a non-zero value. Once set, the >> output mark may not be reset to zero, which ensures >> that updating the SA does not require the mark to >> be re-sent to avoid the value being clobbered. > > There is an attempt to extend the 'output_mark' to support the input > direction and masking. > > In the proposed implementation, output_mark is converted to type 'struct > xfrm_mark' where the semantics are as follows: > > - If mark is given by XFRMA_OUTPUT_MARK (renamed to XFRMA_SET_MARK) > then a new XFRMA_SET_MARK_MASK attribute is consulted to set the mask > value > - if no XFRMA_SET_MARK_MASK attribute is provided, the mask is set to > 0xffffffff > > Therefore, if the mask value is 0, we can regard the mark as 'not > given'. > > My question is, in the context of this patch, it seems that the > "Once set, the output mark may not be reset to zero" restriction may be > lifted in favor of updating the mark only if the new mask is non zero. > > Does this make sense to you? > Eyal
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c index f595797a20ce..5d6085e81030 100644 --- a/net/xfrm/xfrm_state.c +++ b/net/xfrm/xfrm_state.c @@ -1554,6 +1554,13 @@ int xfrm_state_update(struct xfrm_state *x) if (x1->curlft.use_time) xfrm_state_check_expire(x1); + spin_lock_bh(&net->xfrm.xfrm_state_lock); + if (x->props.output_mark) { + x1->props.output_mark = x->props.output_mark; + __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 output_mark to permit policy separation of packet routing decisions from SA keying in systems that use mark-based routing. In the output_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 output mark is only updated if sent with a non-zero value. Once set, the output mark may not be reset to zero, which ensures that updating the SA does not require the mark to be re-sent to avoid the value being clobbered. 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 | 7 +++++++ 1 file changed, 7 insertions(+)