| Message ID | 20200421143149.45108-1-yuehaibing@huawei.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | xfrm: policy: Only use mark as policy lookup key | expand |
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.
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; > > . >
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) > > > > > > > . > > >
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) > > > > > > > > > > > > > . > > > > >
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. > >> >> >> >>> >>> . >>> >> > > . >
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? > > > > >> > >> > >> > >>> > >>> . > >>> > >> > > > > . > > >
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? >
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? > > >
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? >>> >> > > . >
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? >>>> >>> >> >> . >> > > > . >
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;
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(-)