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.bir...@gmail.com> wrote: > > Hi Benedict, > > On Mon, 14 Jan 2019 11:24:38 -0800 > Benedict Wong <benedictw...@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 > > <benedictw...@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, >
