[PATCH] xfrm: policy: Only use mark as policy lookup key

[PATCH] xfrm: policy: Only use mark as policy lookup key

[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

Re: [PATCH] xfrm: policy: Only use mark as policy lookup key

[Steffen Klassert]

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.

Re: [PATCH] xfrm: policy: Only use mark as policy lookup key

[Yuehaibing]

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;



> 
> .
> 

Re: [PATCH] xfrm: policy: Only use mark as policy lookup key

[Xin Long]

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)

>
>
>
> >
> > .
> >
>

Re: [PATCH] xfrm: policy: Only use mark as policy lookup key

[Xin Long]

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)
>
> >
> >
> >
> > >
> > > .
> > >
> >

Re: [PATCH] xfrm: policy: Only use mark as policy lookup key

[Yuehaibing]

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.

> 
>>
>>
>>
>>>
>>> .
>>>
>>
> 
> .
> 

Re: [PATCH] xfrm: policy: Only use mark as policy lookup key

[Xin Long]

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?

>
> >
> >>
> >>
> >>
> >>>
> >>> .
> >>>
> >>
> >
> > .
> >
>

Re: [PATCH] xfrm: policy: Only use mark as policy lookup key

[Yuehaibing]

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?
> 

Re: [PATCH] xfrm: policy: Only use mark as policy lookup key

[Xin Long]

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?
> >
>

Re: [PATCH] xfrm: policy: Only use mark as policy lookup key

[Yuehaibing]

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?
>>>
>>
> 
> .
> 

Re: [PATCH] xfrm: policy: Only use mark as policy lookup key

[Yuehaibing]

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?
>>>>
>>>
>>
>> .
>>
> 
> 
> .
> 

[PATCH v2] xfrm: policy: Fix xfrm policy match

[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

Re: [PATCH v2] xfrm: policy: Fix xfrm policy match

[Yuehaibing]

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;
> 

Re: [PATCH v2] xfrm: policy: Fix xfrm policy match

[Steffen Klassert]

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.

Re: [PATCH v2] xfrm: policy: Fix xfrm policy match

[Xin Long]

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.

Re: [PATCH v2] xfrm: policy: Fix xfrm policy match

[Yuehaibing]

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.

> 
> .
> 

Re: [PATCH v2] xfrm: policy: Fix xfrm policy match

[Xin Long]

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.

Re: [PATCH v2] xfrm: policy: Fix xfrm policy match

[Yuehaibing]

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.

> 
> .
> 

Re: [PATCH v2] xfrm: policy: Fix xfrm policy match

[Xin Long]

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.

Re: [PATCH v2] xfrm: policy: Fix xfrm policy match

[Yuehaibing]

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.
>
> .
>