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

[Xin Long]

On Thu, Apr 23, 2020 at 10:26 AM Yuehaibing <yuehaibing@xxxxxxxxxx> wrote:
>
> On 2020/4/22 23:41, Xin Long wrote:
> > On Wed, Apr 22, 2020 at 8:18 PM Yuehaibing <yuehaibing@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>>> ---
> >>>>  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?

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