Message ID | 20200422125346.27756-1-yuehaibing@huawei.com |
---|---|
State | Awaiting Upstream |
Delegated to: | David Miller |
Headers | show |
Series | [v2] xfrm: policy: Fix xfrm policy match | expand |
Friendly ping... Any plan for this issue? On 2020/4/22 20:53, YueHaibing wrote: > While update xfrm policy as follow: > > ip -6 xfrm policy update src fd00::1/128 dst fd00::2/128 dir in \ > priority 1 mark 0 mask 0x10 > ip -6 xfrm policy update src fd00::1/128 dst fd00::2/128 dir in \ > priority 2 mark 0 mask 0x00 > ip -6 xfrm policy update src fd00::1/128 dst fd00::2/128 dir in \ > priority 2 mark 0 mask 0x10 > > We get this warning: > > WARNING: CPU: 0 PID: 4808 at net/xfrm/xfrm_policy.c:1548 > Kernel panic - not syncing: panic_on_warn set ... > CPU: 0 PID: 4808 Comm: ip Not tainted 5.7.0-rc1+ #151 > Call Trace: > RIP: 0010:xfrm_policy_insert_list+0x153/0x1e0 > xfrm_policy_inexact_insert+0x70/0x330 > xfrm_policy_insert+0x1df/0x250 > xfrm_add_policy+0xcc/0x190 [xfrm_user] > xfrm_user_rcv_msg+0x1d1/0x1f0 [xfrm_user] > netlink_rcv_skb+0x4c/0x120 > xfrm_netlink_rcv+0x32/0x40 [xfrm_user] > netlink_unicast+0x1b3/0x270 > netlink_sendmsg+0x350/0x470 > sock_sendmsg+0x4f/0x60 > > Policy C and policy A has the same mark.v and mark.m, so policy A is > matched in first round lookup while updating C. However policy C and > policy B has same mark and priority, which also leads to matched. So > the WARN_ON is triggered. > > xfrm policy lookup should only be matched if the found policy has the > same lookup keys (mark.v & mark.m) and priority. > > Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities") > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > --- > v2: policy matched while have same mark and priority > --- > net/xfrm/xfrm_policy.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > index 297b2fdb3c29..2a0d7f5e6545 100644 > --- a/net/xfrm/xfrm_policy.c > +++ b/net/xfrm/xfrm_policy.c > @@ -1436,12 +1436,7 @@ static void xfrm_policy_requeue(struct xfrm_policy *old, > static bool xfrm_policy_mark_match(struct xfrm_policy *policy, > struct xfrm_policy *pol) > { > - u32 mark = policy->mark.v & policy->mark.m; > - > - if (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m) > - return true; > - > - if ((mark & pol->mark.m) == pol->mark.v && > + if ((policy->mark.v & policy->mark.m) == (pol->mark.v & pol->mark.m) && > policy->priority == pol->priority) > return true; > > @@ -1628,7 +1623,7 @@ __xfrm_policy_bysel_ctx(struct hlist_head *chain, u32 mark, u32 if_id, > hlist_for_each_entry(pol, chain, bydst) { > if (pol->type == type && > pol->if_id == if_id && > - (mark & pol->mark.m) == pol->mark.v && > + mark == (pol->mark.m & pol->mark.v) && > !selector_cmp(sel, &pol->selector) && > xfrm_sec_ctx_match(ctx, pol->security)) > return pol; > @@ -1726,7 +1721,7 @@ struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u32 if_id, > hlist_for_each_entry(pol, chain, byidx) { > if (pol->type == type && pol->index == id && > pol->if_id == if_id && > - (mark & pol->mark.m) == pol->mark.v) { > + mark == (pol->mark.m & pol->mark.v)) { > xfrm_pol_hold(pol); > if (delete) { > *err = security_xfrm_policy_delete( > @@ -1898,7 +1893,7 @@ static int xfrm_policy_match(const struct xfrm_policy *pol, > > if (pol->family != family || > pol->if_id != if_id || > - (fl->flowi_mark & pol->mark.m) != pol->mark.v || > + fl->flowi_mark != (pol->mark.m & pol->mark.v) || > pol->type != type) > return ret; > > @@ -2177,7 +2172,7 @@ static struct xfrm_policy *xfrm_sk_policy_lookup(const struct sock *sk, int dir, > > match = xfrm_selector_match(&pol->selector, fl, family); > if (match) { > - if ((sk->sk_mark & pol->mark.m) != pol->mark.v || > + if (sk->sk_mark != (pol->mark.m & pol->mark.v) || > pol->if_id != if_id) { > pol = NULL; > goto out; >
On Fri, May 15, 2020 at 04:39:57PM +0800, Yuehaibing wrote: > > Friendly ping... > > Any plan for this issue? There was still no consensus between you and Xin on how to fix this issue. Once this happens, I consider applying a fix.
On Tue, May 19, 2020 at 4:53 PM Steffen Klassert <steffen.klassert@secunet.com> wrote: > > On Fri, May 15, 2020 at 04:39:57PM +0800, Yuehaibing wrote: > > > > Friendly ping... > > > > Any plan for this issue? > > There was still no consensus between you and Xin on how > to fix this issue. Once this happens, I consider applying > a fix. > Sorry, Yuehaibing, I can't really accept to do: (A->mark.m & A->mark.v) I'm thinking to change to: static bool xfrm_policy_mark_match(struct xfrm_policy *policy, struct xfrm_policy *pol) { - u32 mark = policy->mark.v & policy->mark.m; - - if (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m) - return true; - - if ((mark & pol->mark.m) == pol->mark.v && - policy->priority == pol->priority) + if (policy->mark.v == pol->mark.v && + (policy->mark.m == pol->mark.m || + policy->priority == pol->priority)) return true; return false; which means we consider (the same value and mask) or (the same value and priority) as the same one. This will cover both problems.
On 2020/5/21 14:49, Xin Long wrote: > On Tue, May 19, 2020 at 4:53 PM Steffen Klassert > <steffen.klassert@secunet.com> wrote: >> >> On Fri, May 15, 2020 at 04:39:57PM +0800, Yuehaibing wrote: >>> >>> Friendly ping... >>> >>> Any plan for this issue? >> >> There was still no consensus between you and Xin on how >> to fix this issue. Once this happens, I consider applying >> a fix. >> > Sorry, Yuehaibing, I can't really accept to do: (A->mark.m & A->mark.v) > I'm thinking to change to: > > static bool xfrm_policy_mark_match(struct xfrm_policy *policy, > struct xfrm_policy *pol) > { > - u32 mark = policy->mark.v & policy->mark.m; > - > - if (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m) > - return true; > - > - if ((mark & pol->mark.m) == pol->mark.v && > - policy->priority == pol->priority) > + if (policy->mark.v == pol->mark.v && > + (policy->mark.m == pol->mark.m || > + policy->priority == pol->priority)) > return true; > > return false; > > which means we consider (the same value and mask) or > (the same value and priority) as the same one. This will > cover both problems. policy A (mark.v = 0x1011, mark.m = 0x1011, priority = 1) policy B (mark.v = 0x1001, mark.m = 0x1001, priority = 1) when fl->flowi_mark == 0x12341011, in xfrm_policy_match() do check like this: (fl->flowi_mark & pol->mark.m) != pol->mark.v 0x12341011 & 0x1011 == 0x00001011 0x12341011 & 0x1001 == 0x00001001 This also match different policy depends on the order of policy inserting. > > . >
On Fri, May 22, 2020 at 9:45 AM Yuehaibing <yuehaibing@huawei.com> wrote: > > On 2020/5/21 14:49, Xin Long wrote: > > On Tue, May 19, 2020 at 4:53 PM Steffen Klassert > > <steffen.klassert@secunet.com> wrote: > >> > >> On Fri, May 15, 2020 at 04:39:57PM +0800, Yuehaibing wrote: > >>> > >>> Friendly ping... > >>> > >>> Any plan for this issue? > >> > >> There was still no consensus between you and Xin on how > >> to fix this issue. Once this happens, I consider applying > >> a fix. > >> > > Sorry, Yuehaibing, I can't really accept to do: (A->mark.m & A->mark.v) > > I'm thinking to change to: > > > > static bool xfrm_policy_mark_match(struct xfrm_policy *policy, > > struct xfrm_policy *pol) > > { > > - u32 mark = policy->mark.v & policy->mark.m; > > - > > - if (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m) > > - return true; > > - > > - if ((mark & pol->mark.m) == pol->mark.v && > > - policy->priority == pol->priority) > > + if (policy->mark.v == pol->mark.v && > > + (policy->mark.m == pol->mark.m || > > + policy->priority == pol->priority)) > > return true; > > > > return false; > > > > which means we consider (the same value and mask) or > > (the same value and priority) as the same one. This will > > cover both problems. > > policy A (mark.v = 0x1011, mark.m = 0x1011, priority = 1) > policy B (mark.v = 0x1001, mark.m = 0x1001, priority = 1) I'd think these are 2 different policies. > > when fl->flowi_mark == 0x12341011, in xfrm_policy_match() do check like this: > > (fl->flowi_mark & pol->mark.m) != pol->mark.v > > 0x12341011 & 0x1011 == 0x00001011 > 0x12341011 & 0x1001 == 0x00001001 > > This also match different policy depends on the order of policy inserting. Yes, this may happen when a user adds 2 policies like that. But I think this's a problem that the user doesn't configure it well, 'priority' should be set. and this can not be avoided, also such as: policy A (mark.v = 0xff00, mark.m = 0x1000, priority = 1) policy B (mark.v = 0x00ff, mark.m = 0x0011, priority = 1) try with 0x12341011 So just be it, let users decide.
On 2020/5/22 13:49, Xin Long wrote: > On Fri, May 22, 2020 at 9:45 AM Yuehaibing <yuehaibing@huawei.com> wrote: >> >> On 2020/5/21 14:49, Xin Long wrote: >>> On Tue, May 19, 2020 at 4:53 PM Steffen Klassert >>> <steffen.klassert@secunet.com> wrote: >>>> >>>> On Fri, May 15, 2020 at 04:39:57PM +0800, Yuehaibing wrote: >>>>> >>>>> Friendly ping... >>>>> >>>>> Any plan for this issue? >>>> >>>> There was still no consensus between you and Xin on how >>>> to fix this issue. Once this happens, I consider applying >>>> a fix. >>>> >>> Sorry, Yuehaibing, I can't really accept to do: (A->mark.m & A->mark.v) >>> I'm thinking to change to: >>> >>> static bool xfrm_policy_mark_match(struct xfrm_policy *policy, >>> struct xfrm_policy *pol) >>> { >>> - u32 mark = policy->mark.v & policy->mark.m; >>> - >>> - if (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m) >>> - return true; >>> - >>> - if ((mark & pol->mark.m) == pol->mark.v && >>> - policy->priority == pol->priority) >>> + if (policy->mark.v == pol->mark.v && >>> + (policy->mark.m == pol->mark.m || >>> + policy->priority == pol->priority)) >>> return true; >>> >>> return false; >>> >>> which means we consider (the same value and mask) or >>> (the same value and priority) as the same one. This will >>> cover both problems. >> >> policy A (mark.v = 0x1011, mark.m = 0x1011, priority = 1) >> policy B (mark.v = 0x1001, mark.m = 0x1001, priority = 1) > I'd think these are 2 different policies. > >> >> when fl->flowi_mark == 0x12341011, in xfrm_policy_match() do check like this: >> >> (fl->flowi_mark & pol->mark.m) != pol->mark.v >> >> 0x12341011 & 0x1011 == 0x00001011 >> 0x12341011 & 0x1001 == 0x00001001 >> >> This also match different policy depends on the order of policy inserting. > Yes, this may happen when a user adds 2 policies like that. > But I think this's a problem that the user doesn't configure it well, > 'priority' should be set. > and this can not be avoided, also such as: > > policy A (mark.v = 0xff00, mark.m = 0x1000, priority = 1) > policy B (mark.v = 0x00ff, mark.m = 0x0011, priority = 1) > > try with 0x12341011 > > So just be it, let users decide. Ok, this make sense. > > . >
On Fri, May 22, 2020 at 8:39 PM Yuehaibing <yuehaibing@huawei.com> wrote: > > On 2020/5/22 13:49, Xin Long wrote: > > On Fri, May 22, 2020 at 9:45 AM Yuehaibing <yuehaibing@huawei.com> wrote: > >> > >> On 2020/5/21 14:49, Xin Long wrote: > >>> On Tue, May 19, 2020 at 4:53 PM Steffen Klassert > >>> <steffen.klassert@secunet.com> wrote: > >>>> > >>>> On Fri, May 15, 2020 at 04:39:57PM +0800, Yuehaibing wrote: > >>>>> > >>>>> Friendly ping... > >>>>> > >>>>> Any plan for this issue? > >>>> > >>>> There was still no consensus between you and Xin on how > >>>> to fix this issue. Once this happens, I consider applying > >>>> a fix. > >>>> > >>> Sorry, Yuehaibing, I can't really accept to do: (A->mark.m & A->mark.v) > >>> I'm thinking to change to: > >>> > >>> static bool xfrm_policy_mark_match(struct xfrm_policy *policy, > >>> struct xfrm_policy *pol) > >>> { > >>> - u32 mark = policy->mark.v & policy->mark.m; > >>> - > >>> - if (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m) > >>> - return true; > >>> - > >>> - if ((mark & pol->mark.m) == pol->mark.v && > >>> - policy->priority == pol->priority) > >>> + if (policy->mark.v == pol->mark.v && > >>> + (policy->mark.m == pol->mark.m || > >>> + policy->priority == pol->priority)) > >>> return true; > >>> > >>> return false; > >>> > >>> which means we consider (the same value and mask) or > >>> (the same value and priority) as the same one. This will > >>> cover both problems. > >> > >> policy A (mark.v = 0x1011, mark.m = 0x1011, priority = 1) > >> policy B (mark.v = 0x1001, mark.m = 0x1001, priority = 1) > > I'd think these are 2 different policies. > > > >> > >> when fl->flowi_mark == 0x12341011, in xfrm_policy_match() do check like this: > >> > >> (fl->flowi_mark & pol->mark.m) != pol->mark.v > >> > >> 0x12341011 & 0x1011 == 0x00001011 > >> 0x12341011 & 0x1001 == 0x00001001 > >> > >> This also match different policy depends on the order of policy inserting. > > Yes, this may happen when a user adds 2 policies like that. > > But I think this's a problem that the user doesn't configure it well, > > 'priority' should be set. > > and this can not be avoided, also such as: > > > > policy A (mark.v = 0xff00, mark.m = 0x1000, priority = 1) > > policy B (mark.v = 0x00ff, mark.m = 0x0011, priority = 1) > > > > try with 0x12341011 > > > > So just be it, let users decide. > > Ok, this make sense. Thanks Yuehaibing, it's good we're on the same page now. Just realized the patch I created above won't work for the case: policy A (mark.v = 0x10, mark.m = 0, priority = 1) policy B (mark.v = 0x1, mark.m = 0, priority = 2) policy C (mark.v = 0x10, mark.m = 0, priority = 2) when policy C is being added, the warning still occurs. So I will just check value and priority: - u32 mark = policy->mark.v & policy->mark.m; - - if (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m) - return true; - - if ((mark & pol->mark.m) == pol->mark.v && + if (policy->mark.v == pol->mark.v && policy->priority == pol->priority) return true; This allows two policies like this exist: policy A (mark.v = 0x10, mark.m = 0, priority = 1) policy C (mark.v = 0x10, mark.m = 0, priority = 2) But I don't think it's a problem.
On 2020/5/23 17:02, Xin Long wrote: > On Fri, May 22, 2020 at 8:39 PM Yuehaibing <yuehaibing@huawei.com> wrote: >> >> On 2020/5/22 13:49, Xin Long wrote: >>> On Fri, May 22, 2020 at 9:45 AM Yuehaibing <yuehaibing@huawei.com> wrote: >>>> >>>> On 2020/5/21 14:49, Xin Long wrote: >>>>> On Tue, May 19, 2020 at 4:53 PM Steffen Klassert >>>>> <steffen.klassert@secunet.com> wrote: >>>>>> >>>>>> On Fri, May 15, 2020 at 04:39:57PM +0800, Yuehaibing wrote: >>>>>>> >>>>>>> Friendly ping... >>>>>>> >>>>>>> Any plan for this issue? >>>>>> >>>>>> There was still no consensus between you and Xin on how >>>>>> to fix this issue. Once this happens, I consider applying >>>>>> a fix. >>>>>> >>>>> Sorry, Yuehaibing, I can't really accept to do: (A->mark.m & A->mark.v) >>>>> I'm thinking to change to: >>>>> >>>>> static bool xfrm_policy_mark_match(struct xfrm_policy *policy, >>>>> struct xfrm_policy *pol) >>>>> { >>>>> - u32 mark = policy->mark.v & policy->mark.m; >>>>> - >>>>> - if (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m) >>>>> - return true; >>>>> - >>>>> - if ((mark & pol->mark.m) == pol->mark.v && >>>>> - policy->priority == pol->priority) >>>>> + if (policy->mark.v == pol->mark.v && >>>>> + (policy->mark.m == pol->mark.m || >>>>> + policy->priority == pol->priority)) >>>>> return true; >>>>> >>>>> return false; >>>>> >>>>> which means we consider (the same value and mask) or >>>>> (the same value and priority) as the same one. This will >>>>> cover both problems. >>>> >>>> policy A (mark.v = 0x1011, mark.m = 0x1011, priority = 1) >>>> policy B (mark.v = 0x1001, mark.m = 0x1001, priority = 1) >>> I'd think these are 2 different policies. >>> >>>> >>>> when fl->flowi_mark == 0x12341011, in xfrm_policy_match() do check like this: >>>> >>>> (fl->flowi_mark & pol->mark.m) != pol->mark.v >>>> >>>> 0x12341011 & 0x1011 == 0x00001011 >>>> 0x12341011 & 0x1001 == 0x00001001 >>>> >>>> This also match different policy depends on the order of policy inserting. >>> Yes, this may happen when a user adds 2 policies like that. >>> But I think this's a problem that the user doesn't configure it well, >>> 'priority' should be set. >>> and this can not be avoided, also such as: >>> >>> policy A (mark.v = 0xff00, mark.m = 0x1000, priority = 1) >>> policy B (mark.v = 0x00ff, mark.m = 0x0011, priority = 1) >>> >>> try with 0x12341011 >>> >>> So just be it, let users decide. >> >> Ok, this make sense. > Thanks Yuehaibing, it's good we're on the same page now. > > Just realized the patch I created above won't work for the case: > > policy A (mark.v = 0x10, mark.m = 0, priority = 1) > policy B (mark.v = 0x1, mark.m = 0, priority = 2) > policy C (mark.v = 0x10, mark.m = 0, priority = 2) > > when policy C is being added, the warning still occurs. Do you means this: policy A (mark.v = 0x10, mark.m = 0, priority = 1) policy B (mark.v = 0x10, mark.m = 1, priority = 2) policy C (mark.v = 0x10, mark.m = 0, priority = 2) > > So I will just check value and priority: > - u32 mark = policy->mark.v & policy->mark.m; > - > - if (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m) > - return true; > - > - if ((mark & pol->mark.m) == pol->mark.v && > + if (policy->mark.v == pol->mark.v && > policy->priority == pol->priority) > return true; > > This allows two policies like this exist: > > policy A (mark.v = 0x10, mark.m = 0, priority = 1) > policy C (mark.v = 0x10, mark.m = 0, priority = 2) > > But I don't think it's a problem. Agreed. > > . >
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 297b2fdb3c29..2a0d7f5e6545 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -1436,12 +1436,7 @@ static void xfrm_policy_requeue(struct xfrm_policy *old, static bool xfrm_policy_mark_match(struct xfrm_policy *policy, struct xfrm_policy *pol) { - u32 mark = policy->mark.v & policy->mark.m; - - if (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m) - return true; - - if ((mark & pol->mark.m) == pol->mark.v && + if ((policy->mark.v & policy->mark.m) == (pol->mark.v & pol->mark.m) && policy->priority == pol->priority) return true; @@ -1628,7 +1623,7 @@ __xfrm_policy_bysel_ctx(struct hlist_head *chain, u32 mark, u32 if_id, hlist_for_each_entry(pol, chain, bydst) { if (pol->type == type && pol->if_id == if_id && - (mark & pol->mark.m) == pol->mark.v && + mark == (pol->mark.m & pol->mark.v) && !selector_cmp(sel, &pol->selector) && xfrm_sec_ctx_match(ctx, pol->security)) return pol; @@ -1726,7 +1721,7 @@ struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u32 if_id, hlist_for_each_entry(pol, chain, byidx) { if (pol->type == type && pol->index == id && pol->if_id == if_id && - (mark & pol->mark.m) == pol->mark.v) { + mark == (pol->mark.m & pol->mark.v)) { xfrm_pol_hold(pol); if (delete) { *err = security_xfrm_policy_delete( @@ -1898,7 +1893,7 @@ static int xfrm_policy_match(const struct xfrm_policy *pol, if (pol->family != family || pol->if_id != if_id || - (fl->flowi_mark & pol->mark.m) != pol->mark.v || + fl->flowi_mark != (pol->mark.m & pol->mark.v) || pol->type != type) return ret; @@ -2177,7 +2172,7 @@ static struct xfrm_policy *xfrm_sk_policy_lookup(const struct sock *sk, int dir, match = xfrm_selector_match(&pol->selector, fl, family); if (match) { - if ((sk->sk_mark & pol->mark.m) != pol->mark.v || + if (sk->sk_mark != (pol->mark.m & pol->mark.v) || pol->if_id != if_id) { pol = NULL; goto out;
While update xfrm policy as follow: ip -6 xfrm policy update src fd00::1/128 dst fd00::2/128 dir in \ priority 1 mark 0 mask 0x10 ip -6 xfrm policy update src fd00::1/128 dst fd00::2/128 dir in \ priority 2 mark 0 mask 0x00 ip -6 xfrm policy update src fd00::1/128 dst fd00::2/128 dir in \ priority 2 mark 0 mask 0x10 We get this warning: WARNING: CPU: 0 PID: 4808 at net/xfrm/xfrm_policy.c:1548 Kernel panic - not syncing: panic_on_warn set ... CPU: 0 PID: 4808 Comm: ip Not tainted 5.7.0-rc1+ #151 Call Trace: RIP: 0010:xfrm_policy_insert_list+0x153/0x1e0 xfrm_policy_inexact_insert+0x70/0x330 xfrm_policy_insert+0x1df/0x250 xfrm_add_policy+0xcc/0x190 [xfrm_user] xfrm_user_rcv_msg+0x1d1/0x1f0 [xfrm_user] netlink_rcv_skb+0x4c/0x120 xfrm_netlink_rcv+0x32/0x40 [xfrm_user] netlink_unicast+0x1b3/0x270 netlink_sendmsg+0x350/0x470 sock_sendmsg+0x4f/0x60 Policy C and policy A has the same mark.v and mark.m, so policy A is matched in first round lookup while updating C. However policy C and policy B has same mark and priority, which also leads to matched. So the WARN_ON is triggered. xfrm policy lookup should only be matched if the found policy has the same lookup keys (mark.v & mark.m) and priority. Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities") Signed-off-by: YueHaibing <yuehaibing@huawei.com> --- v2: policy matched while have same mark and priority --- net/xfrm/xfrm_policy.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)