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

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

[Nathan Harold]

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);
+			spin_unlock_bh(&net->xfrm.xfrm_state_lock);
+		}
+
 		err = 0;
 		x->km.state = XFRM_STATE_DEAD;
 		__xfrm_state_put(x);
-- 
2.18.0.399.gad0ab374a1-goog

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

[Eyal Birger]

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: [PATCH ipsec-next] xfrm: Allow Set Mark to be Updated Using UPDSA

[Nathan Harold]

< 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.

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

[Eyal Birger]

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.