[PATCH ipsec, resend 0/1] xfrm: set-mark default behavior changes

[PATCH ipsec, resend 0/1] xfrm: set-mark default behavior changes

[Benedict Wong]

A behavior change introduced in 9b42c1f179a6 (“xfrm: Extend the
output_mark to support input direction and masking”) results in a
change in:

1. Default outbound behavior with regards to route lookup marks, and
2. Inbound behavior for SAs used to decapsulate packets when the output
     mark (as specified in 4.14 to 4.18) is set.

This patch set restores the previous default outbound behavior,
resolving (1), but behavior change (2) will require more discussion.

Specifically, in (2), a SA with a "output mark" set will now have that
Mark imposed on the inbound packet (As opposed to the previous
output-mark behavior where the inbound packet's mark would not be
touched). This is less of a concern, as it is limited to the case where:

1. SA output mark is set
2. SA is using non-transport mode
3. SA is configured for inbound decapsulation (local dst IP)

Critically, conditions 1 and 3 imply a configuration that output mark
was not designed to support. The only valid use case for this seems
to be the loopback case (as IP addresses would apply bidirectionally).
As such, we believe that this behavioral change is acceptable as is.

Benedict Wong (1):
  xfrm: Make set-mark default behavior backward compatible

 net/xfrm/xfrm_policy.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.20.1.97.g81188d93c3-goog

[PATCH ipsec, resend 1/1] xfrm: Make set-mark default behavior backward compatible

[Benedict Wong]

Fixes 9b42c1f179a6, which changed the default route lookup behavior for
tunnel mode SAs in the outbound direction to use the skb mark, whereas
previously mark=0 was used if the output mark was unspecified. In
mark-based routing schemes such as Android’s, this change in default
behavior causes routing loops or lookup failures.

This patch restores the default behavior of using a 0 mark while still
incorporating the skb mark if the SET_MARK (and SET_MARK_MASK) is
specified.

Tested with additions to Android's kernel unit test suite:
https://android-review.googlesource.com/c/kernel/tests/+/860150

Fixes: 9b42c1f179a6 ("xfrm: Extend the output_mark to support input direction and masking")
Signed-off-by: Benedict Wong <benedictwong@google.com>
---
 net/xfrm/xfrm_policy.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 934492bad8e0..5f574ede1332 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2600,7 +2600,10 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 		dst_copy_metrics(dst1, dst);
 
 		if (xfrm[i]->props.mode != XFRM_MODE_TRANSPORT) {
-			__u32 mark = xfrm_smark_get(fl->flowi_mark, xfrm[i]);
+			__u32 mark = 0;
+
+			if (xfrm[i]->props.smark.v || xfrm[i]->props.smark.m)
+				mark = xfrm_smark_get(fl->flowi_mark, xfrm[i]);
 
 			family = xfrm[i]->props.family;
 			dst = xfrm_dst_lookup(xfrm[i], tos, fl->flowi_oif,
-- 
2.20.1.97.g81188d93c3-goog

Re: [PATCH ipsec, resend 0/1] xfrm: set-mark default behavior changes

[Steffen Klassert]

On Fri, Jan 11, 2019 at 12:14:11PM -0800, Benedict Wong wrote:
> A behavior change introduced in 9b42c1f179a6 (“xfrm: Extend the
> output_mark to support input direction and masking”) results in a
> change in:
> 
> 1. Default outbound behavior with regards to route lookup marks, and
> 2. Inbound behavior for SAs used to decapsulate packets when the output
>      mark (as specified in 4.14 to 4.18) is set.
> 
> This patch set restores the previous default outbound behavior,
> resolving (1), but behavior change (2) will require more discussion.
> 
> Specifically, in (2), a SA with a "output mark" set will now have that
> Mark imposed on the inbound packet (As opposed to the previous
> output-mark behavior where the inbound packet's mark would not be
> touched). This is less of a concern, as it is limited to the case where:
> 
> 1. SA output mark is set
> 2. SA is using non-transport mode
> 3. SA is configured for inbound decapsulation (local dst IP)
> 
> Critically, conditions 1 and 3 imply a configuration that output mark
> was not designed to support. The only valid use case for this seems
> to be the loopback case (as IP addresses would apply bidirectionally).
> As such, we believe that this behavioral change is acceptable as is.

There was no need to resend this without changes. I still had this
patchset in my queue. I'm ok with the change, but you did not Cc
all the authors of the patch you want to fix. So please resend once
again with Cc to all the authors, so that they have a chance to
review this change.

Thanks!

Re: [PATCH ipsec, resend 0/1] xfrm: set-mark default behavior changes

[Benedict Wong]

(Resend as plaintext. Forgot to check gmail plain-text setting)

> There was no need to resend this without changes. I still had this
> patchset in my queue.

Ahh, my apologies. I will keep that in mind for next time.

> I'm ok with the change, but you did not Cc
> all the authors of the patch you want to fix. So please resend once
> again with Cc to all the authors, so that they have a chance to
> review this change.

Can do. I'll resend with all of the people listed in the co-developed
commit lines of 9b42c1f179a6


On Sat, Jan 12, 2019 at 2:02 AM Steffen Klassert
<steffen.klassert@secunet.com> wrote:
>
> On Fri, Jan 11, 2019 at 12:14:11PM -0800, Benedict Wong wrote:
> > A behavior change introduced in 9b42c1f179a6 (“xfrm: Extend the
> > output_mark to support input direction and masking”) results in a
> > change in:
> >
> > 1. Default outbound behavior with regards to route lookup marks, and
> > 2. Inbound behavior for SAs used to decapsulate packets when the output
> >      mark (as specified in 4.14 to 4.18) is set.
> >
> > This patch set restores the previous default outbound behavior,
> > resolving (1), but behavior change (2) will require more discussion.
> >
> > Specifically, in (2), a SA with a "output mark" set will now have that
> > Mark imposed on the inbound packet (As opposed to the previous
> > output-mark behavior where the inbound packet's mark would not be
> > touched). This is less of a concern, as it is limited to the case where:
> >
> > 1. SA output mark is set
> > 2. SA is using non-transport mode
> > 3. SA is configured for inbound decapsulation (local dst IP)
> >
> > Critically, conditions 1 and 3 imply a configuration that output mark
> > was not designed to support. The only valid use case for this seems
> > to be the loopback case (as IP addresses would apply bidirectionally).
> > As such, we believe that this behavioral change is acceptable as is.
>
> There was no need to resend this without changes. I still had this
> patchset in my queue. I'm ok with the change, but you did not Cc
> all the authors of the patch you want to fix. So please resend once
> again with Cc to all the authors, so that they have a chance to
> review this change.
>
> Thanks!

[PATCH ipsec, resend 0/1] xfrm: set-mark default behavior changes

[Benedict Wong]

Resend; added authors of 9b42c1f179a6 to CC list

A behavior change introduced in 9b42c1f179a6 (“xfrm: Extend the
output_mark to support input direction and masking”) results in a
change in:

1. Default outbound behavior with regards to route lookup marks, and
2. Inbound behavior for SAs used to decapsulate packets when the output
     mark (as specified in 4.14 to 4.18) is set.

This patch set restores the previous default outbound behavior,
resolving (1), but behavior change (2) will require more discussion.

Specifically, in (2), a SA with a "output mark" set will now have that
Mark imposed on the inbound packet (As opposed to the previous
output-mark behavior where the inbound packet's mark would not be
touched). This is less of a concern, as it is limited to the case where:

1. SA output mark is set
2. SA is using non-transport mode
3. SA is configured for inbound decapsulation (local dst IP)

Critically, conditions 1 and 3 imply a configuration that output mark
was not designed to support. The only valid use case for this seems
to be the loopback case (as IP addresses would apply bidirectionally).
As such, we believe that this behavioral change is acceptable as is.

Benedict Wong (1):
  xfrm: Make set-mark default behavior backward compatible

 net/xfrm/xfrm_policy.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.20.1.97.g81188d93c3-goog