| Message ID | 20190114192438.198153-2-benedictwong@google.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | xfrm: set-mark default behavior changes | expand |
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,
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, >
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!
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,
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(-)