* [PATCH ipsec, resend 1/1] xfrm: Make set-mark default behavior backward compatible
2019-01-11 20:14 [PATCH ipsec, resend 0/1] xfrm: set-mark default behavior changes Benedict Wong
@ 2019-01-11 20:14 ` Benedict Wong
0 siblings, 0 replies; 6+ messages in thread
From: Benedict Wong @ 2019-01-11 20:14 UTC (permalink / raw)
To: netdev; +Cc: nharold, benedictwong, lorenzo, maze
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH ipsec, resend 0/1] xfrm: set-mark default behavior changes
@ 2019-01-14 19:24 Benedict Wong
2019-01-14 19:24 ` [PATCH ipsec, resend 1/1] xfrm: Make set-mark default behavior backward compatible Benedict Wong
0 siblings, 1 reply; 6+ messages in thread
From: Benedict Wong @ 2019-01-14 19:24 UTC (permalink / raw)
To: netdev
Cc: steffen.klassert, tobias, eyal.birger, lorenzo, nharold,
benedictwong, maze
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH ipsec, resend 1/1] xfrm: Make set-mark default behavior backward compatible
2019-01-14 19:24 [PATCH ipsec, resend 0/1] xfrm: set-mark default behavior changes Benedict Wong
@ 2019-01-14 19:24 ` Benedict Wong
2019-01-15 5:11 ` Eyal Birger
2019-01-17 11:30 ` Steffen Klassert
0 siblings, 2 replies; 6+ messages in thread
From: Benedict Wong @ 2019-01-14 19:24 UTC (permalink / raw)
To: netdev
Cc: steffen.klassert, tobias, eyal.birger, lorenzo, nharold,
benedictwong, maze
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH ipsec, resend 1/1] xfrm: Make set-mark default behavior backward compatible
2019-01-14 19:24 ` [PATCH ipsec, resend 1/1] xfrm: Make set-mark default behavior backward compatible Benedict Wong
@ 2019-01-15 5:11 ` Eyal Birger
2019-01-15 19:05 ` Benedict Wong
2019-01-17 11:30 ` Steffen Klassert
1 sibling, 1 reply; 6+ messages in thread
From: Eyal Birger @ 2019-01-15 5:11 UTC (permalink / raw)
To: Benedict Wong; +Cc: netdev, steffen.klassert, tobias, lorenzo, nharold, maze
Hi Benedict,
On Mon, 14 Jan 2019 11:24:38 -0800
Benedict Wong <benedictwong@google.com> wrote:
> 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.
It's a little sad that we didn't distinguish between 'no-mark-provided'
and 'mark = mask = 0'. IIUC before this fix, explicitly setting mark,
mask to a) mark = 0, mask = 0xffffffff and b) mark = 0, mask = 0 behaved
differently, whereas after this fix they will act the same.
But as it seems there was never support for the (b) scenario and commit
9b42c1f179a6 broke the existing behavior, so I'm ok with this fix.
Thanks!
Eyal.
>
> 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,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH ipsec, resend 1/1] xfrm: Make set-mark default behavior backward compatible
2019-01-15 5:11 ` Eyal Birger
@ 2019-01-15 19:05 ` Benedict Wong
0 siblings, 0 replies; 6+ messages in thread
From: Benedict Wong @ 2019-01-15 19:05 UTC (permalink / raw)
To: Eyal Birger
Cc: netdev, Steffen Klassert, tobias, Lorenzo Colitti, Nathan Harold,
Maciej Żenczykowski
Hi Eyal,
Thanks for taking a look at this.
> It's a little sad that we didn't distinguish between 'no-mark-provided'
> and 'mark = mask = 0'. IIUC before this fix, explicitly setting mark,
> mask to a) mark = 0, mask = 0xffffffff and b) mark = 0, mask = 0 behaved
> differently, whereas after this fix they will act the same.
Agreed. It's a little hacky, but someone determined to use the entirety
of the mark from the flowi (case b) in the route lookup could use
mark = 0x1, mask = 0x0, and that would give the same results as
mark = 0x0, mask = 0x0. It's a little intuitive though. :)
Perhaps at some future point, it may be worth defining a bit in the
xfrm_state->props->extra_flags as "set-mark provided," At which point
we can separate cases (a) and (b)
Cheers,
Ben.
On Mon, Jan 14, 2019 at 9:11 PM Eyal Birger <eyal.birger@gmail.com> wrote:
>
> Hi Benedict,
>
> On Mon, 14 Jan 2019 11:24:38 -0800
> Benedict Wong <benedictwong@google.com> wrote:
>
> > 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.
>
> It's a little sad that we didn't distinguish between 'no-mark-provided'
> and 'mark = mask = 0'. IIUC before this fix, explicitly setting mark,
> mask to a) mark = 0, mask = 0xffffffff and b) mark = 0, mask = 0 behaved
> differently, whereas after this fix they will act the same.
>
> But as it seems there was never support for the (b) scenario and commit
> 9b42c1f179a6 broke the existing behavior, so I'm ok with this fix.
>
> Thanks!
> Eyal.
>
> >
> > 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,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH ipsec, resend 1/1] xfrm: Make set-mark default behavior backward compatible
2019-01-14 19:24 ` [PATCH ipsec, resend 1/1] xfrm: Make set-mark default behavior backward compatible Benedict Wong
2019-01-15 5:11 ` Eyal Birger
@ 2019-01-17 11:30 ` Steffen Klassert
1 sibling, 0 replies; 6+ messages in thread
From: Steffen Klassert @ 2019-01-17 11:30 UTC (permalink / raw)
To: Benedict Wong; +Cc: netdev, tobias, eyal.birger, lorenzo, nharold, maze
On Mon, Jan 14, 2019 at 11:24:38AM -0800, Benedict Wong wrote:
> 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>
Applied, thanks a lot Benedict!
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-17 11:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-01-14 19:24 [PATCH ipsec, resend 0/1] xfrm: set-mark default behavior changes Benedict Wong
2019-01-14 19:24 ` [PATCH ipsec, resend 1/1] xfrm: Make set-mark default behavior backward compatible Benedict Wong
2019-01-15 5:11 ` Eyal Birger
2019-01-15 19:05 ` Benedict Wong
2019-01-17 11:30 ` Steffen Klassert
-- strict thread matches above, loose matches on Subject: below --
2019-01-11 20:14 [PATCH ipsec, resend 0/1] xfrm: set-mark default behavior changes Benedict Wong
2019-01-11 20:14 ` [PATCH ipsec, resend 1/1] xfrm: Make set-mark default behavior backward compatible Benedict Wong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).