* [PATCH] xfrm: policy: Only use mark as policy lookup key @ 2020-04-21 14:31 YueHaibing 2020-04-22 9:33 ` Steffen Klassert 2020-04-22 12:53 ` [PATCH v2] xfrm: policy: Fix xfrm policy match YueHaibing 0 siblings, 2 replies; 20+ messages in thread From: YueHaibing @ 2020-04-21 14:31 UTC (permalink / raw) To: steffen.klassert, herbert, davem, kuba Cc: netdev, linux-kernel, lucien.xin, YueHaibing 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 when the found policy has the same lookup keys (mark.v & mark.m) no matter priority. Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities") Signed-off-by: YueHaibing <yuehaibing@huawei.com> --- net/xfrm/xfrm_policy.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 297b2fd..67d0469 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -1436,13 +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 && - policy->priority == pol->priority) + if ((policy->mark.v & policy->mark.m) == (pol->mark.v & pol->mark.m)) return true; return false; @@ -1628,7 +1622,7 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl) 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 +1720,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 +1892,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 +2171,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; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] xfrm: policy: Only use mark as policy lookup key 2020-04-21 14:31 [PATCH] xfrm: policy: Only use mark as policy lookup key YueHaibing @ 2020-04-22 9:33 ` Steffen Klassert 2020-04-22 12:18 ` Yuehaibing 2020-04-22 12:53 ` [PATCH v2] xfrm: policy: Fix xfrm policy match YueHaibing 1 sibling, 1 reply; 20+ messages in thread From: Steffen Klassert @ 2020-04-22 9:33 UTC (permalink / raw) To: YueHaibing; +Cc: herbert, davem, kuba, netdev, linux-kernel, lucien.xin On Tue, Apr 21, 2020 at 10:31:49PM +0800, 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 when the found policy has the > same lookup keys (mark.v & mark.m) no matter priority. > > Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities") > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > --- > net/xfrm/xfrm_policy.c | 16 +++++----------- > 1 file changed, 5 insertions(+), 11 deletions(-) > > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > index 297b2fd..67d0469 100644 > --- a/net/xfrm/xfrm_policy.c > +++ b/net/xfrm/xfrm_policy.c > @@ -1436,13 +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 && > - policy->priority == pol->priority) If you remove the priority check, you can't insert policies with matching mark and different priorities anymore. This brings us back the old bug. I plan to apply the patch from Xin Long, this seems to be the right way to address this problem. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] xfrm: policy: Only use mark as policy lookup key 2020-04-22 9:33 ` Steffen Klassert @ 2020-04-22 12:18 ` Yuehaibing 2020-04-22 15:41 ` Xin Long 0 siblings, 1 reply; 20+ messages in thread From: Yuehaibing @ 2020-04-22 12:18 UTC (permalink / raw) To: Steffen Klassert; +Cc: herbert, davem, kuba, netdev, linux-kernel, lucien.xin On 2020/4/22 17:33, Steffen Klassert wrote: > On Tue, Apr 21, 2020 at 10:31:49PM +0800, 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 when the found policy has the >> same lookup keys (mark.v & mark.m) no matter priority. >> >> Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities") >> Signed-off-by: YueHaibing <yuehaibing@huawei.com> >> --- >> net/xfrm/xfrm_policy.c | 16 +++++----------- >> 1 file changed, 5 insertions(+), 11 deletions(-) >> >> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c >> index 297b2fd..67d0469 100644 >> --- a/net/xfrm/xfrm_policy.c >> +++ b/net/xfrm/xfrm_policy.c >> @@ -1436,13 +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 && >> - policy->priority == pol->priority) > > If you remove the priority check, you can't insert policies with matching > mark and different priorities anymore. This brings us back the old bug. Yes, this is true. > > I plan to apply the patch from Xin Long, this seems to be the right way > to address this problem. That still brings an issue, update like this: policy A (mark.v = 1, mark.m = 0, priority = 1) policy B (mark.v = 1, mark.m = 0, priority = 1) A and B will all in the list. So should do this: 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; > > . > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] xfrm: policy: Only use mark as policy lookup key 2020-04-22 12:18 ` Yuehaibing @ 2020-04-22 15:41 ` Xin Long 2020-04-22 15:54 ` Xin Long 2020-04-23 2:25 ` Yuehaibing 0 siblings, 2 replies; 20+ messages in thread From: Xin Long @ 2020-04-22 15:41 UTC (permalink / raw) To: Yuehaibing; +Cc: Steffen Klassert, Herbert Xu, davem, kuba, network dev, LKML On Wed, Apr 22, 2020 at 8:18 PM Yuehaibing <yuehaibing@huawei.com> wrote: > > On 2020/4/22 17:33, Steffen Klassert wrote: > > On Tue, Apr 21, 2020 at 10:31:49PM +0800, 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 when the found policy has the > >> same lookup keys (mark.v & mark.m) no matter priority. > >> > >> Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities") > >> Signed-off-by: YueHaibing <yuehaibing@huawei.com> > >> --- > >> net/xfrm/xfrm_policy.c | 16 +++++----------- > >> 1 file changed, 5 insertions(+), 11 deletions(-) > >> > >> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > >> index 297b2fd..67d0469 100644 > >> --- a/net/xfrm/xfrm_policy.c > >> +++ b/net/xfrm/xfrm_policy.c > >> @@ -1436,13 +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 && > >> - policy->priority == pol->priority) > > > > If you remove the priority check, you can't insert policies with matching > > mark and different priorities anymore. This brings us back the old bug. > > Yes, this is true. > > > > > I plan to apply the patch from Xin Long, this seems to be the right way > > to address this problem. > > That still brings an issue, update like this: > > policy A (mark.v = 1, mark.m = 0, priority = 1) > policy B (mark.v = 1, mark.m = 0, priority = 1) > > A and B will all in the list. I think this is another issue even before: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities") > > So should do this: > > 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; "mark.v & mark.m" looks weird to me, it should be: ((something & mark.m) == mark.v) So why should we just do this here?: (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m && policy->priority == pol->priority) > > > > > > > . > > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] xfrm: policy: Only use mark as policy lookup key 2020-04-22 15:41 ` Xin Long @ 2020-04-22 15:54 ` Xin Long 2020-04-23 2:25 ` Yuehaibing 1 sibling, 0 replies; 20+ messages in thread From: Xin Long @ 2020-04-22 15:54 UTC (permalink / raw) To: Yuehaibing; +Cc: Steffen Klassert, Herbert Xu, davem, kuba, network dev, LKML On Wed, Apr 22, 2020 at 11:41 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Wed, Apr 22, 2020 at 8:18 PM Yuehaibing <yuehaibing@huawei.com> wrote: > > > > On 2020/4/22 17:33, Steffen Klassert wrote: > > > On Tue, Apr 21, 2020 at 10:31:49PM +0800, 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 when the found policy has the > > >> same lookup keys (mark.v & mark.m) no matter priority. > > >> > > >> Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities") > > >> Signed-off-by: YueHaibing <yuehaibing@huawei.com> > > >> --- > > >> net/xfrm/xfrm_policy.c | 16 +++++----------- > > >> 1 file changed, 5 insertions(+), 11 deletions(-) > > >> > > >> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > > >> index 297b2fd..67d0469 100644 > > >> --- a/net/xfrm/xfrm_policy.c > > >> +++ b/net/xfrm/xfrm_policy.c > > >> @@ -1436,13 +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 && > > >> - policy->priority == pol->priority) > > > > > > If you remove the priority check, you can't insert policies with matching > > > mark and different priorities anymore. This brings us back the old bug. > > > > Yes, this is true. > > > > > > > > I plan to apply the patch from Xin Long, this seems to be the right way > > > to address this problem. > > > > That still brings an issue, update like this: > > > > policy A (mark.v = 1, mark.m = 0, priority = 1) > > policy B (mark.v = 1, mark.m = 0, priority = 1) > > > > A and B will all in the list. > I think this is another issue even before: > 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and > different priorities") > > > > > So should do this: > > > > 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; > "mark.v & mark.m" looks weird to me, it should be: > ((something & mark.m) == mark.v) > > So why should we just do this here?: *shouldn't, sorry ;D > (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m && > policy->priority == pol->priority) > > > > > > > > > > > > > . > > > > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] xfrm: policy: Only use mark as policy lookup key 2020-04-22 15:41 ` Xin Long 2020-04-22 15:54 ` Xin Long @ 2020-04-23 2:25 ` Yuehaibing 2020-04-23 6:37 ` Xin Long 1 sibling, 1 reply; 20+ messages in thread From: Yuehaibing @ 2020-04-23 2:25 UTC (permalink / raw) To: Xin Long; +Cc: Steffen Klassert, Herbert Xu, davem, kuba, network dev, LKML On 2020/4/22 23:41, Xin Long wrote: > On Wed, Apr 22, 2020 at 8:18 PM Yuehaibing <yuehaibing@huawei.com> wrote: >> >> On 2020/4/22 17:33, Steffen Klassert wrote: >>> On Tue, Apr 21, 2020 at 10:31:49PM +0800, 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 when the found policy has the >>>> same lookup keys (mark.v & mark.m) no matter priority. >>>> >>>> Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities") >>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com> >>>> --- >>>> net/xfrm/xfrm_policy.c | 16 +++++----------- >>>> 1 file changed, 5 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c >>>> index 297b2fd..67d0469 100644 >>>> --- a/net/xfrm/xfrm_policy.c >>>> +++ b/net/xfrm/xfrm_policy.c >>>> @@ -1436,13 +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 && >>>> - policy->priority == pol->priority) >>> >>> If you remove the priority check, you can't insert policies with matching >>> mark and different priorities anymore. This brings us back the old bug. >> >> Yes, this is true. >> >>> >>> I plan to apply the patch from Xin Long, this seems to be the right way >>> to address this problem. >> >> That still brings an issue, update like this: >> >> policy A (mark.v = 1, mark.m = 0, priority = 1) >> policy B (mark.v = 1, mark.m = 0, priority = 1) >> >> A and B will all in the list. > I think this is another issue even before: > 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and > different priorities") > >> >> So should do this: >> >> 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; > "mark.v & mark.m" looks weird to me, it should be: > ((something & mark.m) == mark.v) > > So why should we just do this here?: > (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m && > policy->priority == pol->priority) This leads to this issue: ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir in mark 0x00000001 mask 0x00000005 ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir in mark 0x00000001 mask 0x00000003 the two policies will be in list, which should not be allowed. > >> >> >> >>> >>> . >>> >> > > . > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] xfrm: policy: Only use mark as policy lookup key 2020-04-23 2:25 ` Yuehaibing @ 2020-04-23 6:37 ` Xin Long 2020-04-23 8:40 ` Yuehaibing 0 siblings, 1 reply; 20+ messages in thread From: Xin Long @ 2020-04-23 6:37 UTC (permalink / raw) To: Yuehaibing Cc: Steffen Klassert, Herbert Xu, davem, kuba, network dev, LKML, Jamal Hadi Salim On Thu, Apr 23, 2020 at 10:26 AM Yuehaibing <yuehaibing@huawei.com> wrote: > > On 2020/4/22 23:41, Xin Long wrote: > > On Wed, Apr 22, 2020 at 8:18 PM Yuehaibing <yuehaibing@huawei.com> wrote: > >> > >> On 2020/4/22 17:33, Steffen Klassert wrote: > >>> On Tue, Apr 21, 2020 at 10:31:49PM +0800, 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 when the found policy has the > >>>> same lookup keys (mark.v & mark.m) no matter priority. > >>>> > >>>> Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities") > >>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com> > >>>> --- > >>>> net/xfrm/xfrm_policy.c | 16 +++++----------- > >>>> 1 file changed, 5 insertions(+), 11 deletions(-) > >>>> > >>>> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > >>>> index 297b2fd..67d0469 100644 > >>>> --- a/net/xfrm/xfrm_policy.c > >>>> +++ b/net/xfrm/xfrm_policy.c > >>>> @@ -1436,13 +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 && > >>>> - policy->priority == pol->priority) > >>> > >>> If you remove the priority check, you can't insert policies with matching > >>> mark and different priorities anymore. This brings us back the old bug. > >> > >> Yes, this is true. > >> > >>> > >>> I plan to apply the patch from Xin Long, this seems to be the right way > >>> to address this problem. > >> > >> That still brings an issue, update like this: > >> > >> policy A (mark.v = 1, mark.m = 0, priority = 1) > >> policy B (mark.v = 1, mark.m = 0, priority = 1) > >> > >> A and B will all in the list. > > I think this is another issue even before: > > 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and > > different priorities") > > > >> > >> So should do this: > >> > >> 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; > > "mark.v & mark.m" looks weird to me, it should be: > > ((something & mark.m) == mark.v) > > > > So why should we just do this here?: > > (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m && > > policy->priority == pol->priority) > > > This leads to this issue: > > ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir in mark 0x00000001 mask 0x00000005 > ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir in mark 0x00000001 mask 0x00000003 > > the two policies will be in list, which should not be allowed. I think these are two different policies. For instance: mark = 0x1234567b will match the 1st one only. mark = 0x1234567d will match the 2st one only So these should have been allowed, no? I'm actually confused now. does the mask work against its own value, or the other value? as 'A == (mark.v&mark.m)' and '(A & mark.m) == mark.v' are different things. This can date back to Jamal's xfrm by MARK: https://lwn.net/Articles/375829/ where it does 'm->v & m->m' in xfrm_mark_get() and 'policy->mark.v & policy->mark.m' in xfrm_policy_insert() while it does '(A & pol->mark.m) == pol->mark.v' in other places. Now I'm thinking 'm->v & m->m' is meaningless, by which if we get a value != m->v, it means this mark can never be matched by any. policy A (mark.v = 1, mark.m = 0, priority = 1) policy B (mark.v = 1, mark.m = 0, priority = 1) So probably we should avoid this case by check m->v == (m->v & m->m) when adding a new policy. wdyt? > > > > >> > >> > >> > >>> > >>> . > >>> > >> > > > > . > > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] xfrm: policy: Only use mark as policy lookup key 2020-04-23 6:37 ` Xin Long @ 2020-04-23 8:40 ` Yuehaibing 2020-04-23 9:43 ` Xin Long 0 siblings, 1 reply; 20+ messages in thread From: Yuehaibing @ 2020-04-23 8:40 UTC (permalink / raw) To: Xin Long Cc: Steffen Klassert, Herbert Xu, davem, kuba, network dev, LKML, Jamal Hadi Salim On 2020/4/23 14:37, Xin Long wrote: > On Thu, Apr 23, 2020 at 10:26 AM Yuehaibing <yuehaibing@huawei.com> wrote: >> >> On 2020/4/22 23:41, Xin Long wrote: >>> On Wed, Apr 22, 2020 at 8:18 PM Yuehaibing <yuehaibing@huawei.com> wrote: >>>> >>>> On 2020/4/22 17:33, Steffen Klassert wrote: >>>>> On Tue, Apr 21, 2020 at 10:31:49PM +0800, 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 when the found policy has the >>>>>> same lookup keys (mark.v & mark.m) no matter priority. >>>>>> >>>>>> Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities") >>>>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com> >>>>>> --- >>>>>> net/xfrm/xfrm_policy.c | 16 +++++----------- >>>>>> 1 file changed, 5 insertions(+), 11 deletions(-) >>>>>> >>>>>> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c >>>>>> index 297b2fd..67d0469 100644 >>>>>> --- a/net/xfrm/xfrm_policy.c >>>>>> +++ b/net/xfrm/xfrm_policy.c >>>>>> @@ -1436,13 +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 && >>>>>> - policy->priority == pol->priority) >>>>> >>>>> If you remove the priority check, you can't insert policies with matching >>>>> mark and different priorities anymore. This brings us back the old bug. >>>> >>>> Yes, this is true. >>>> >>>>> >>>>> I plan to apply the patch from Xin Long, this seems to be the right way >>>>> to address this problem. >>>> >>>> That still brings an issue, update like this: >>>> >>>> policy A (mark.v = 1, mark.m = 0, priority = 1) >>>> policy B (mark.v = 1, mark.m = 0, priority = 1) >>>> >>>> A and B will all in the list. >>> I think this is another issue even before: >>> 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and >>> different priorities") >>> >>>> >>>> So should do this: >>>> >>>> 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; >>> "mark.v & mark.m" looks weird to me, it should be: >>> ((something & mark.m) == mark.v) >>> >>> So why should we just do this here?: >>> (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m && >>> policy->priority == pol->priority) >> >> >> This leads to this issue: >> >> ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir in mark 0x00000001 mask 0x00000005 >> ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir in mark 0x00000001 mask 0x00000003 >> >> the two policies will be in list, which should not be allowed. > I think these are two different policies. > For instance: > mark = 0x1234567b will match the 1st one only. > mark = 0x1234567d will match the 2st one only > > So these should have been allowed, no? If mark = 0x12345671, it may match different policy depends on the order of inserting, ip xfrm policy update src 172.16.2.0/24 dst 172.16.1.0/24 dir in ptype main \ tmpl src 192.168.2.10 dst 192.168.1.20 proto esp mode tunnel mark 0x00000001 mask 0x00000005 ip xfrm policy update src 172.16.2.0/24 dst 172.16.1.0/24 dir in ptype main \ tmpl src 192.168.2.100 dst 192.168.1.100 proto esp mode beet mark 0x00000001 mask 0x00000003 In fact, your case should use different priority to match. > > I'm actually confused now. > does the mask work against its own value, or the other value? > as 'A == (mark.v&mark.m)' and '(A & mark.m) == mark.v' are different things. > > This can date back to Jamal's xfrm by MARK: > > https://lwn.net/Articles/375829/ > > where it does 'm->v & m->m' in xfrm_mark_get() and > 'policy->mark.v & policy->mark.m' in xfrm_policy_insert() while > it does '(A & pol->mark.m) == pol->mark.v' in other places. > > Now I'm thinking 'm->v & m->m' is meaningless, by which if we get > a value != m->v, it means this mark can never be matched by any. > > policy A (mark.v = 1, mark.m = 0, priority = 1) > policy B (mark.v = 1, mark.m = 0, priority = 1) > > So probably we should avoid this case by check m->v == (m->v & m->m) > when adding a new policy. > > wdyt? > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] xfrm: policy: Only use mark as policy lookup key 2020-04-23 8:40 ` Yuehaibing @ 2020-04-23 9:43 ` Xin Long 2020-04-24 3:48 ` Yuehaibing 0 siblings, 1 reply; 20+ messages in thread From: Xin Long @ 2020-04-23 9:43 UTC (permalink / raw) To: Yuehaibing Cc: Steffen Klassert, Herbert Xu, davem, kuba, network dev, LKML, Jamal Hadi Salim On Thu, Apr 23, 2020 at 4:41 PM Yuehaibing <yuehaibing@huawei.com> wrote: > > On 2020/4/23 14:37, Xin Long wrote: > > On Thu, Apr 23, 2020 at 10:26 AM Yuehaibing <yuehaibing@huawei.com> wrote: > >> > >> On 2020/4/22 23:41, Xin Long wrote: > >>> On Wed, Apr 22, 2020 at 8:18 PM Yuehaibing <yuehaibing@huawei.com> wrote: > >>>> > >>>> On 2020/4/22 17:33, Steffen Klassert wrote: > >>>>> On Tue, Apr 21, 2020 at 10:31:49PM +0800, 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 when the found policy has the > >>>>>> same lookup keys (mark.v & mark.m) no matter priority. > >>>>>> > >>>>>> Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities") > >>>>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com> > >>>>>> --- > >>>>>> net/xfrm/xfrm_policy.c | 16 +++++----------- > >>>>>> 1 file changed, 5 insertions(+), 11 deletions(-) > >>>>>> > >>>>>> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > >>>>>> index 297b2fd..67d0469 100644 > >>>>>> --- a/net/xfrm/xfrm_policy.c > >>>>>> +++ b/net/xfrm/xfrm_policy.c > >>>>>> @@ -1436,13 +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 && > >>>>>> - policy->priority == pol->priority) > >>>>> > >>>>> If you remove the priority check, you can't insert policies with matching > >>>>> mark and different priorities anymore. This brings us back the old bug. > >>>> > >>>> Yes, this is true. > >>>> > >>>>> > >>>>> I plan to apply the patch from Xin Long, this seems to be the right way > >>>>> to address this problem. > >>>> > >>>> That still brings an issue, update like this: > >>>> > >>>> policy A (mark.v = 1, mark.m = 0, priority = 1) > >>>> policy B (mark.v = 1, mark.m = 0, priority = 1) > >>>> > >>>> A and B will all in the list. > >>> I think this is another issue even before: > >>> 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and > >>> different priorities") > >>> > >>>> > >>>> So should do this: > >>>> > >>>> 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; > >>> "mark.v & mark.m" looks weird to me, it should be: > >>> ((something & mark.m) == mark.v) > >>> > >>> So why should we just do this here?: > >>> (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m && > >>> policy->priority == pol->priority) > >> > >> > >> This leads to this issue: > >> > >> ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir in mark 0x00000001 mask 0x00000005 > >> ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir in mark 0x00000001 mask 0x00000003 > >> > >> the two policies will be in list, which should not be allowed. > > I think these are two different policies. > > For instance: > > mark = 0x1234567b will match the 1st one only. > > mark = 0x1234567d will match the 2st one only > > > > So these should have been allowed, no? > > If mark = 0x12345671, it may match different policy depends on the order of inserting, > > ip xfrm policy update src 172.16.2.0/24 dst 172.16.1.0/24 dir in ptype main \ > tmpl src 192.168.2.10 dst 192.168.1.20 proto esp mode tunnel mark 0x00000001 mask 0x00000005 > > ip xfrm policy update src 172.16.2.0/24 dst 172.16.1.0/24 dir in ptype main \ > tmpl src 192.168.2.100 dst 192.168.1.100 proto esp mode beet mark 0x00000001 mask 0x00000003 > > In fact, your case should use different priority to match. Sorry, but it does match your above policies now, like in xfrm_policy_match(), when fl->flowi_mark == 0x1234567b: (fl->flowi_mark & pol->mark.m) != pol->mark.v 0x1234567b & 0x00000005 == 0x00000001 and when fl->flowi_mark == 0x1234567d: 0x1234567d & 0x00000003 == 0x00000001 am I missing something? > > > > > I'm actually confused now. > > does the mask work against its own value, or the other value? > > as 'A == (mark.v&mark.m)' and '(A & mark.m) == mark.v' are different things. > > > > This can date back to Jamal's xfrm by MARK: > > > > https://lwn.net/Articles/375829/ > > > > where it does 'm->v & m->m' in xfrm_mark_get() and > > 'policy->mark.v & policy->mark.m' in xfrm_policy_insert() while > > it does '(A & pol->mark.m) == pol->mark.v' in other places. > > > > Now I'm thinking 'm->v & m->m' is meaningless, by which if we get > > a value != m->v, it means this mark can never be matched by any. > > > > policy A (mark.v = 1, mark.m = 0, priority = 1) > > policy B (mark.v = 1, mark.m = 0, priority = 1) > > > > So probably we should avoid this case by check m->v == (m->v & m->m) > > when adding a new policy. > > > > wdyt? > > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] xfrm: policy: Only use mark as policy lookup key 2020-04-23 9:43 ` Xin Long @ 2020-04-24 3:48 ` Yuehaibing 2020-04-30 6:30 ` Yuehaibing 0 siblings, 1 reply; 20+ messages in thread From: Yuehaibing @ 2020-04-24 3:48 UTC (permalink / raw) To: Xin Long Cc: Steffen Klassert, Herbert Xu, davem, kuba, network dev, LKML, Jamal Hadi Salim On 2020/4/23 17:43, Xin Long wrote: > On Thu, Apr 23, 2020 at 4:41 PM Yuehaibing <yuehaibing@huawei.com> wrote: >> >> On 2020/4/23 14:37, Xin Long wrote: >>> On Thu, Apr 23, 2020 at 10:26 AM Yuehaibing <yuehaibing@huawei.com> wrote: >>>> >>>> On 2020/4/22 23:41, Xin Long wrote: >>>>> On Wed, Apr 22, 2020 at 8:18 PM Yuehaibing <yuehaibing@huawei.com> wrote: >>>>>> >>>>>> On 2020/4/22 17:33, Steffen Klassert wrote: >>>>>>> On Tue, Apr 21, 2020 at 10:31:49PM +0800, 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 when the found policy has the >>>>>>>> same lookup keys (mark.v & mark.m) no matter priority. >>>>>>>> >>>>>>>> Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities") >>>>>>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com> >>>>>>>> --- >>>>>>>> net/xfrm/xfrm_policy.c | 16 +++++----------- >>>>>>>> 1 file changed, 5 insertions(+), 11 deletions(-) >>>>>>>> >>>>>>>> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c >>>>>>>> index 297b2fd..67d0469 100644 >>>>>>>> --- a/net/xfrm/xfrm_policy.c >>>>>>>> +++ b/net/xfrm/xfrm_policy.c >>>>>>>> @@ -1436,13 +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 && >>>>>>>> - policy->priority == pol->priority) >>>>>>> >>>>>>> If you remove the priority check, you can't insert policies with matching >>>>>>> mark and different priorities anymore. This brings us back the old bug. >>>>>> >>>>>> Yes, this is true. >>>>>> >>>>>>> >>>>>>> I plan to apply the patch from Xin Long, this seems to be the right way >>>>>>> to address this problem. >>>>>> >>>>>> That still brings an issue, update like this: >>>>>> >>>>>> policy A (mark.v = 1, mark.m = 0, priority = 1) >>>>>> policy B (mark.v = 1, mark.m = 0, priority = 1) >>>>>> >>>>>> A and B will all in the list. >>>>> I think this is another issue even before: >>>>> 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and >>>>> different priorities") >>>>> >>>>>> >>>>>> So should do this: >>>>>> >>>>>> 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; >>>>> "mark.v & mark.m" looks weird to me, it should be: >>>>> ((something & mark.m) == mark.v) >>>>> >>>>> So why should we just do this here?: >>>>> (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m && >>>>> policy->priority == pol->priority) >>>> >>>> >>>> This leads to this issue: >>>> >>>> ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir in mark 0x00000001 mask 0x00000005 >>>> ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir in mark 0x00000001 mask 0x00000003 >>>> >>>> the two policies will be in list, which should not be allowed. >>> I think these are two different policies. >>> For instance: >>> mark = 0x1234567b will match the 1st one only. >>> mark = 0x1234567d will match the 2st one only >>> >>> So these should have been allowed, no? >> >> If mark = 0x12345671, it may match different policy depends on the order of inserting, >> >> ip xfrm policy update src 172.16.2.0/24 dst 172.16.1.0/24 dir in ptype main \ >> tmpl src 192.168.2.10 dst 192.168.1.20 proto esp mode tunnel mark 0x00000001 mask 0x00000005 >> >> ip xfrm policy update src 172.16.2.0/24 dst 172.16.1.0/24 dir in ptype main \ >> tmpl src 192.168.2.100 dst 192.168.1.100 proto esp mode beet mark 0x00000001 mask 0x00000003 >> >> In fact, your case should use different priority to match. > Sorry, but it does match your above policies now, like in xfrm_policy_match(), > when fl->flowi_mark == 0x1234567b: > > (fl->flowi_mark & pol->mark.m) != pol->mark.v > 0x1234567b & 0x00000005 == 0x00000001 > > and when fl->flowi_mark == 0x1234567d: > 0x1234567d & 0x00000003 == 0x00000001 > > am I missing something? when fl->flowi_mark == 0x12345671 0x12345671 & 0x00000005 == 0x00000001 0x12345671 & 0x00000003 == 0x00000001 This will match different policy depends on the order of policy inserting, it is not expected. > > >> >>> >>> I'm actually confused now. >>> does the mask work against its own value, or the other value? >>> as 'A == (mark.v&mark.m)' and '(A & mark.m) == mark.v' are different things. >>> >>> This can date back to Jamal's xfrm by MARK: >>> >>> https://lwn.net/Articles/375829/ >>> >>> where it does 'm->v & m->m' in xfrm_mark_get() and >>> 'policy->mark.v & policy->mark.m' in xfrm_policy_insert() while >>> it does '(A & pol->mark.m) == pol->mark.v' in other places. >>> >>> Now I'm thinking 'm->v & m->m' is meaningless, by which if we get >>> a value != m->v, it means this mark can never be matched by any. >>> >>> policy A (mark.v = 1, mark.m = 0, priority = 1) >>> policy B (mark.v = 1, mark.m = 0, priority = 1) >>> >>> So probably we should avoid this case by check m->v == (m->v & m->m) >>> when adding a new policy. >>> >>> wdyt? >>> >> > > . > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] xfrm: policy: Only use mark as policy lookup key 2020-04-24 3:48 ` Yuehaibing @ 2020-04-30 6:30 ` Yuehaibing 0 siblings, 0 replies; 20+ messages in thread From: Yuehaibing @ 2020-04-30 6:30 UTC (permalink / raw) To: Steffen Klassert Cc: Xin Long, Herbert Xu, davem, kuba, network dev, LKML, Jamal Hadi Salim On 2020/4/24 11:48, Yuehaibing wrote: > On 2020/4/23 17:43, Xin Long wrote: >> On Thu, Apr 23, 2020 at 4:41 PM Yuehaibing <yuehaibing@huawei.com> wrote: >>> >>> On 2020/4/23 14:37, Xin Long wrote: >>>> On Thu, Apr 23, 2020 at 10:26 AM Yuehaibing <yuehaibing@huawei.com> wrote: >>>>> >>>>> On 2020/4/22 23:41, Xin Long wrote: >>>>>> On Wed, Apr 22, 2020 at 8:18 PM Yuehaibing <yuehaibing@huawei.com> wrote: >>>>>>> >>>>>>> On 2020/4/22 17:33, Steffen Klassert wrote: >>>>>>>> On Tue, Apr 21, 2020 at 10:31:49PM +0800, 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 when the found policy has the >>>>>>>>> same lookup keys (mark.v & mark.m) no matter priority. >>>>>>>>> >>>>>>>>> Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities") >>>>>>>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com> >>>>>>>>> --- >>>>>>>>> net/xfrm/xfrm_policy.c | 16 +++++----------- >>>>>>>>> 1 file changed, 5 insertions(+), 11 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c >>>>>>>>> index 297b2fd..67d0469 100644 >>>>>>>>> --- a/net/xfrm/xfrm_policy.c >>>>>>>>> +++ b/net/xfrm/xfrm_policy.c >>>>>>>>> @@ -1436,13 +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 && >>>>>>>>> - policy->priority == pol->priority) >>>>>>>> >>>>>>>> If you remove the priority check, you can't insert policies with matching >>>>>>>> mark and different priorities anymore. This brings us back the old bug. >>>>>>> >>>>>>> Yes, this is true. >>>>>>> >>>>>>>> >>>>>>>> I plan to apply the patch from Xin Long, this seems to be the right way >>>>>>>> to address this problem. >>>>>>> >>>>>>> That still brings an issue, update like this: >>>>>>> >>>>>>> policy A (mark.v = 1, mark.m = 0, priority = 1) >>>>>>> policy B (mark.v = 1, mark.m = 0, priority = 1) >>>>>>> >>>>>>> A and B will all in the list. >>>>>> I think this is another issue even before: >>>>>> 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and >>>>>> different priorities") >>>>>> >>>>>>> >>>>>>> So should do this: >>>>>>> >>>>>>> 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; >>>>>> "mark.v & mark.m" looks weird to me, it should be: >>>>>> ((something & mark.m) == mark.v) >>>>>> >>>>>> So why should we just do this here?: >>>>>> (policy->mark.v == pol->mark.v && policy->mark.m == pol->mark.m && >>>>>> policy->priority == pol->priority) >>>>> >>>>> >>>>> This leads to this issue: >>>>> >>>>> ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir in mark 0x00000001 mask 0x00000005 >>>>> ip -6 xfrm policy add src fd00::1/128 dst fd00::2/128 dir in mark 0x00000001 mask 0x00000003 >>>>> >>>>> the two policies will be in list, which should not be allowed. >>>> I think these are two different policies. >>>> For instance: >>>> mark = 0x1234567b will match the 1st one only. >>>> mark = 0x1234567d will match the 2st one only >>>> >>>> So these should have been allowed, no? >>> >>> If mark = 0x12345671, it may match different policy depends on the order of inserting, >>> >>> ip xfrm policy update src 172.16.2.0/24 dst 172.16.1.0/24 dir in ptype main \ >>> tmpl src 192.168.2.10 dst 192.168.1.20 proto esp mode tunnel mark 0x00000001 mask 0x00000005 >>> >>> ip xfrm policy update src 172.16.2.0/24 dst 172.16.1.0/24 dir in ptype main \ >>> tmpl src 192.168.2.100 dst 192.168.1.100 proto esp mode beet mark 0x00000001 mask 0x00000003 >>> >>> In fact, your case should use different priority to match. >> Sorry, but it does match your above policies now, like in xfrm_policy_match(), >> when fl->flowi_mark == 0x1234567b: >> >> (fl->flowi_mark & pol->mark.m) != pol->mark.v >> 0x1234567b & 0x00000005 == 0x00000001 >> >> and when fl->flowi_mark == 0x1234567d: >> 0x1234567d & 0x00000003 == 0x00000001 >> >> am I missing something? > > when fl->flowi_mark == 0x12345671 > > 0x12345671 & 0x00000005 == 0x00000001 > 0x12345671 & 0x00000003 == 0x00000001 > > This will match different policy depends on the order of policy inserting, it is not expected. > Steffen, any futher comment ? >> >> >>> >>>> >>>> I'm actually confused now. >>>> does the mask work against its own value, or the other value? >>>> as 'A == (mark.v&mark.m)' and '(A & mark.m) == mark.v' are different things. >>>> >>>> This can date back to Jamal's xfrm by MARK: >>>> >>>> https://lwn.net/Articles/375829/ >>>> >>>> where it does 'm->v & m->m' in xfrm_mark_get() and >>>> 'policy->mark.v & policy->mark.m' in xfrm_policy_insert() while >>>> it does '(A & pol->mark.m) == pol->mark.v' in other places. >>>> >>>> Now I'm thinking 'm->v & m->m' is meaningless, by which if we get >>>> a value != m->v, it means this mark can never be matched by any. >>>> >>>> policy A (mark.v = 1, mark.m = 0, priority = 1) >>>> policy B (mark.v = 1, mark.m = 0, priority = 1) >>>> >>>> So probably we should avoid this case by check m->v == (m->v & m->m) >>>> when adding a new policy. >>>> >>>> wdyt? >>>> >>> >> >> . >> > > > . > ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2] xfrm: policy: Fix xfrm policy match 2020-04-21 14:31 [PATCH] xfrm: policy: Only use mark as policy lookup key YueHaibing 2020-04-22 9:33 ` Steffen Klassert @ 2020-04-22 12:53 ` YueHaibing 2020-05-15 8:39 ` Yuehaibing 1 sibling, 1 reply; 20+ messages in thread From: YueHaibing @ 2020-04-22 12:53 UTC (permalink / raw) To: steffen.klassert, herbert, davem, kuba Cc: netdev, linux-kernel, lucien.xin, YueHaibing 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; -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2] xfrm: policy: Fix xfrm policy match 2020-04-22 12:53 ` [PATCH v2] xfrm: policy: Fix xfrm policy match YueHaibing @ 2020-05-15 8:39 ` Yuehaibing 2020-05-19 8:53 ` Steffen Klassert 0 siblings, 1 reply; 20+ messages in thread From: Yuehaibing @ 2020-05-15 8:39 UTC (permalink / raw) To: steffen.klassert, herbert, davem, kuba; +Cc: netdev, linux-kernel, lucien.xin 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; > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] xfrm: policy: Fix xfrm policy match 2020-05-15 8:39 ` Yuehaibing @ 2020-05-19 8:53 ` Steffen Klassert 2020-05-21 6:49 ` Xin Long 0 siblings, 1 reply; 20+ messages in thread From: Steffen Klassert @ 2020-05-19 8:53 UTC (permalink / raw) To: Yuehaibing; +Cc: herbert, davem, kuba, netdev, linux-kernel, lucien.xin 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] xfrm: policy: Fix xfrm policy match 2020-05-19 8:53 ` Steffen Klassert @ 2020-05-21 6:49 ` Xin Long 2020-05-22 1:45 ` Yuehaibing 0 siblings, 1 reply; 20+ messages in thread From: Xin Long @ 2020-05-21 6:49 UTC (permalink / raw) To: Steffen Klassert Cc: Yuehaibing, Herbert Xu, davem, Jakub Kicinski, network dev, LKML 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] xfrm: policy: Fix xfrm policy match 2020-05-21 6:49 ` Xin Long @ 2020-05-22 1:45 ` Yuehaibing 2020-05-22 5:49 ` Xin Long 0 siblings, 1 reply; 20+ messages in thread From: Yuehaibing @ 2020-05-22 1:45 UTC (permalink / raw) To: Xin Long, Steffen Klassert Cc: Herbert Xu, davem, Jakub Kicinski, network dev, LKML 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. > > . > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] xfrm: policy: Fix xfrm policy match 2020-05-22 1:45 ` Yuehaibing @ 2020-05-22 5:49 ` Xin Long 2020-05-22 12:39 ` Yuehaibing 0 siblings, 1 reply; 20+ messages in thread From: Xin Long @ 2020-05-22 5:49 UTC (permalink / raw) To: Yuehaibing Cc: Steffen Klassert, Herbert Xu, davem, Jakub Kicinski, network dev, LKML 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] xfrm: policy: Fix xfrm policy match 2020-05-22 5:49 ` Xin Long @ 2020-05-22 12:39 ` Yuehaibing 2020-05-23 9:02 ` Xin Long 0 siblings, 1 reply; 20+ messages in thread From: Yuehaibing @ 2020-05-22 12:39 UTC (permalink / raw) To: Xin Long Cc: Steffen Klassert, Herbert Xu, davem, Jakub Kicinski, network dev, LKML 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. > > . > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] xfrm: policy: Fix xfrm policy match 2020-05-22 12:39 ` Yuehaibing @ 2020-05-23 9:02 ` Xin Long 2020-05-25 3:04 ` Yuehaibing 0 siblings, 1 reply; 20+ messages in thread From: Xin Long @ 2020-05-23 9:02 UTC (permalink / raw) To: Yuehaibing Cc: Steffen Klassert, Herbert Xu, davem, Jakub Kicinski, network dev, LKML 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. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] xfrm: policy: Fix xfrm policy match 2020-05-23 9:02 ` Xin Long @ 2020-05-25 3:04 ` Yuehaibing 0 siblings, 0 replies; 20+ messages in thread From: Yuehaibing @ 2020-05-25 3:04 UTC (permalink / raw) To: Xin Long Cc: Steffen Klassert, Herbert Xu, davem, Jakub Kicinski, network dev, LKML 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. > > . > ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-05-25 3:05 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-21 14:31 [PATCH] xfrm: policy: Only use mark as policy lookup key YueHaibing 2020-04-22 9:33 ` Steffen Klassert 2020-04-22 12:18 ` Yuehaibing 2020-04-22 15:41 ` Xin Long 2020-04-22 15:54 ` Xin Long 2020-04-23 2:25 ` Yuehaibing 2020-04-23 6:37 ` Xin Long 2020-04-23 8:40 ` Yuehaibing 2020-04-23 9:43 ` Xin Long 2020-04-24 3:48 ` Yuehaibing 2020-04-30 6:30 ` Yuehaibing 2020-04-22 12:53 ` [PATCH v2] xfrm: policy: Fix xfrm policy match YueHaibing 2020-05-15 8:39 ` Yuehaibing 2020-05-19 8:53 ` Steffen Klassert 2020-05-21 6:49 ` Xin Long 2020-05-22 1:45 ` Yuehaibing 2020-05-22 5:49 ` Xin Long 2020-05-22 12:39 ` Yuehaibing 2020-05-23 9:02 ` Xin Long 2020-05-25 3:04 ` Yuehaibing
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).