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

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

[Nathan Harold]

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(+)

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);
-- 
2.17.0.441.gb46fe60e1d-goog

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

[Eyal Birger]

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

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

[Nathan Harold]

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