[PATCHv2 ipsec] xfrm: fix a warning in xfrm_policy_insert_list

[PATCHv2 ipsec] xfrm: fix a warning in xfrm_policy_insert_list

[Xin Long]

This waring can be triggered simply by:

  # ip xfrm policy update src 192.168.1.1/24 dst 192.168.1.2/24 dir in \
    priority 1 mark 0 mask 0x10  #[1]
  # ip xfrm policy update src 192.168.1.1/24 dst 192.168.1.2/24 dir in \
    priority 2 mark 0 mask 0x1   #[2]
  # ip xfrm policy update src 192.168.1.1/24 dst 192.168.1.2/24 dir in \
    priority 2 mark 0 mask 0x10  #[3]

Then dmesg shows:

  [ ] WARNING: CPU: 1 PID: 7265 at net/xfrm/xfrm_policy.c:1548
  [ ] RIP: 0010:xfrm_policy_insert_list+0x2f2/0x1030
  [ ] Call Trace:
  [ ]  xfrm_policy_inexact_insert+0x85/0xe50
  [ ]  xfrm_policy_insert+0x4ba/0x680
  [ ]  xfrm_add_policy+0x246/0x4d0
  [ ]  xfrm_user_rcv_msg+0x331/0x5c0
  [ ]  netlink_rcv_skb+0x121/0x350
  [ ]  xfrm_netlink_rcv+0x66/0x80
  [ ]  netlink_unicast+0x439/0x630
  [ ]  netlink_sendmsg+0x714/0xbf0
  [ ]  sock_sendmsg+0xe2/0x110

The issue was introduced by Commit 7cb8a93968e3 ("xfrm: Allow inserting
policies with matching mark and different priorities"). After that, the
policies [1] and [2] would be able to be added with different priorities.

However, policy [3] will actually match both [1] and [2]. Policy [1]
was matched due to the 1st 'return true' in xfrm_policy_mark_match(),
and policy [2] was matched due to the 2nd 'return true' in there. It
caused WARN_ON() in xfrm_policy_insert_list().

This patch is to fix it by only (the same value and priority) as the
same policy in xfrm_policy_mark_match().

Thanks to Yuehaibing, we could make this fix better.

v1->v2:
  - check policy->mark.v == pol->mark.v only without mask.

Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities")
Reported-by: Xiumei Mu <xmu@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/xfrm/xfrm_policy.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 297b2fd..564aa649 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 == pol->mark.v &&
 	    policy->priority == pol->priority)
 		return true;
 
-- 
2.1.0

Re: [PATCHv2 ipsec] xfrm: fix a warning in xfrm_policy_insert_list

[Steffen Klassert]

On Mon, May 25, 2020 at 01:53:37PM +0800, Xin Long wrote:
> This waring can be triggered simply by:
> 
>   # ip xfrm policy update src 192.168.1.1/24 dst 192.168.1.2/24 dir in \
>     priority 1 mark 0 mask 0x10  #[1]
>   # ip xfrm policy update src 192.168.1.1/24 dst 192.168.1.2/24 dir in \
>     priority 2 mark 0 mask 0x1   #[2]
>   # ip xfrm policy update src 192.168.1.1/24 dst 192.168.1.2/24 dir in \
>     priority 2 mark 0 mask 0x10  #[3]
> 
> Then dmesg shows:
> 
>   [ ] WARNING: CPU: 1 PID: 7265 at net/xfrm/xfrm_policy.c:1548
>   [ ] RIP: 0010:xfrm_policy_insert_list+0x2f2/0x1030
>   [ ] Call Trace:
>   [ ]  xfrm_policy_inexact_insert+0x85/0xe50
>   [ ]  xfrm_policy_insert+0x4ba/0x680
>   [ ]  xfrm_add_policy+0x246/0x4d0
>   [ ]  xfrm_user_rcv_msg+0x331/0x5c0
>   [ ]  netlink_rcv_skb+0x121/0x350
>   [ ]  xfrm_netlink_rcv+0x66/0x80
>   [ ]  netlink_unicast+0x439/0x630
>   [ ]  netlink_sendmsg+0x714/0xbf0
>   [ ]  sock_sendmsg+0xe2/0x110
> 
> The issue was introduced by Commit 7cb8a93968e3 ("xfrm: Allow inserting
> policies with matching mark and different priorities"). After that, the
> policies [1] and [2] would be able to be added with different priorities.
> 
> However, policy [3] will actually match both [1] and [2]. Policy [1]
> was matched due to the 1st 'return true' in xfrm_policy_mark_match(),
> and policy [2] was matched due to the 2nd 'return true' in there. It
> caused WARN_ON() in xfrm_policy_insert_list().
> 
> This patch is to fix it by only (the same value and priority) as the
> same policy in xfrm_policy_mark_match().
> 
> Thanks to Yuehaibing, we could make this fix better.
> 
> v1->v2:
>   - check policy->mark.v == pol->mark.v only without mask.
> 
> Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities")
> Reported-by: Xiumei Mu <xmu@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Applied, thanks everyone!

Re: [PATCHv2 ipsec] xfrm: fix a warning in xfrm_policy_insert_list

[Tobias Brunner]

Hi Steffen, Xin,

This change could be problematic.  Actually, it's not really this one
but the original one that causes the issue:
> Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities")

However, because the code in xfrm_policy_mark_match() treated policies
with the same mark/mask equal without considering the priority before
this change, it wasn't apparent.  The problem is that the code can now
lead to duplicate policies, which can not correctly be removed or queried.

That's because the priority is sent only in xfrm_userpolicy_info, which
XFRM_MSG_NEWPOLICY and XFRM_MSG_UPDPOLICY expect, but not in
xfrm_userpolicy_id, which is used to query and delete policies with
XFRM_MSG_GETPOLICY and XFRM_MSG_DELPOLICY, respectively (the mark is
sent with a separate attribute, which can be supplied to all commands).
 So we can only query/delete the duplicate policy with the highest
priority.  Such duplicates can even be created inadvertently via
XFRM_MSG_UPDPOLICY if the priority of an existing policy should be
changed, which worked fine so far.

The latter currently happens when strongSwan e.g. replaces a trap or
block policy with one that has templates assigned (those we install with
a higher priority, by default), which uses XFRM_MSG_UPDPOLICY that
doesn't update the existing policy anymore but creates a duplicate
instead.  Since only one XFRM_MSG_DELPOLICY is sent later when the
policy is deleted, a duplicate policy remains (and we couldn't even
delete the exact policy we wanted - the trap might be removed by the
user before the regular policy - due to the missing priority field in
xfrm_userpolicy_id).

I guess we could workaround this issue in strongSwan by installing
policies that share the same mark and selector with the same priority,
so only one instance is ever installed in the kernel.  But the inability
to address the exact policy when querying/deleting still looks like a
problem to me in general.

Regards,
Tobias

Re: [PATCHv2 ipsec] xfrm: fix a warning in xfrm_policy_insert_list

[Xin Long]

 a, .

On Mon, Jun 8, 2020 at 8:02 PM Tobias Brunner <tobias@strongswan.org> wrote:
>
> Hi Steffen, Xin,
>
> This change could be problematic.  Actually, it's not really this one
> but the original one that causes the issue:
> > Fixes: 7cb8a93968e3 ("xfrm: Allow inserting policies with matching mark and different priorities")
>
> However, because the code in xfrm_policy_mark_match() treated policies
> with the same mark/mask equal without considering the priority before
> this change, it wasn't apparent.  The problem is that the code can now
> lead to duplicate policies, which can not correctly be removed or queried.
It's a problem that can't be removed, but not for being queried.

>
> That's because the priority is sent only in xfrm_userpolicy_info, which
> XFRM_MSG_NEWPOLICY and XFRM_MSG_UPDPOLICY expect, but not in
> xfrm_userpolicy_id, which is used to query and delete policies with
> XFRM_MSG_GETPOLICY and XFRM_MSG_DELPOLICY, respectively (the mark is
> sent with a separate attribute, which can be supplied to all commands).
>  So we can only query/delete the duplicate policy with the highest
> priority.  Such duplicates can even be created inadvertently via
> XFRM_MSG_UPDPOLICY if the priority of an existing policy should be
> changed, which worked fine so far.
priority doesn't exist in xfrm_userpolicy_id, and we can add
XFRMA_PRIORITY like XFRMA_MARK to fix it.
since we also take priority to make a policy unique, we can't update
this attribute.

>
> The latter currently happens when strongSwan e.g. replaces a trap or
> block policy with one that has templates assigned (those we install with
> a higher priority, by default), which uses XFRM_MSG_UPDPOLICY that
> doesn't update the existing policy anymore but creates a duplicate
> instead.  Since only one XFRM_MSG_DELPOLICY is sent later when the
> policy is deleted, a duplicate policy remains (and we couldn't even
> delete the exact policy we wanted - the trap might be removed by the
> user before the regular policy - due to the missing priority field in
> xfrm_userpolicy_id).
I see.

>
> I guess we could workaround this issue in strongSwan by installing
> policies that share the same mark and selector with the same priority,
> so only one instance is ever installed in the kernel.  But the inability
> to address the exact policy when querying/deleting still looks like a
> problem to me in general.
>
For deleting, yes, but for querying, I think it makes sense not to pass
the priority, and always get the policy with the highest priority.

We can separate the deleting path from the querying path when
XFRMA_PRIORITY attribute is set.

Is that enough for your case to only fix for the policy deleting?

Re: [PATCHv2 ipsec] xfrm: fix a warning in xfrm_policy_insert_list

[Tobias Brunner]

Hi Xin,

>> I guess we could workaround this issue in strongSwan by installing
>> policies that share the same mark and selector with the same priority,
>> so only one instance is ever installed in the kernel.  But the inability
>> to address the exact policy when querying/deleting still looks like a
>> problem to me in general.
>>
> For deleting, yes, but for querying, I think it makes sense not to pass
> the priority, and always get the policy with the highest priority.

While I agree it's less of a problem (at least for strongSwan),  it
should be possible to query the exact policy one wants.  Because as far
as I understand, the whole point of Steffen's original patch was that
all duplicate policies could get used concurrently, depending on the
marks and masks on them and the traffic, so all of them must be queryable.

But I actually think the previous check that viewed policies with the
exact same mark and value as duplicates made sense, because those will
never be used concurrently.  It would at least fix the default behavior
with strongSwan (users have to configure marks/masks manually).

> We can separate the deleting path from the querying path when
> XFRMA_PRIORITY attribute is set.
> 
> Is that enough for your case to only fix for the policy deleting?

While such an attribute could be part of a solution, it does not fix the
regression your patch created.  The kernel behavior changed and a
userland modification is required to get back to something resembling
the previous behavior (without an additional kernel patch we'll actually
not be able to restore the previous behavior, where we separated
different types of policies into priority classes).  That is, current
and old strongSwan versions could create lots of duplicate/lingering
policies, which is not good.

A problem with such an attribute is how userland would learn when to use
it.  We could query the kernel version, but patches might get
backported.  So how can we know the kernel will create duplicates when
we update a policy and change the priority, which we then have to delete
(or even can delete with such a new attribute)?  Do we have to do a
runtime check (e.g. install two duplicate policies with different
priorities and delete twice to see if the second attempt results in an
error)?  With marks it's relatively easy as users have to configure them
explicitly and they work or they don't depending on the kernel version.
 But here it's not so easy as the IKE daemon uses priorities extensively
already.

Like the marks it might work somehow if the new attribute also had to be
passed in the message that creates a policy (marks have to be passed
with every message, including querying them).  While that's not super
ideal as we'd have two priority values in these messages (and have to
keep track of them in the kernel state), there is some precedent with
the anti-replay config for SAs (which can be passed via xfrm_usersa_info
struct or as separate attribute with more options for ESN).  Userland
would still have to learn somehow that the kernel understands the new
attribute and duplicate policies with different priorities are possible.
 But if there was any advantage in using this, we could perhaps later
add an option for users to enable it.  At least the current behavior
would not change (i.e. older strongSwan versions would continue to run
on newer kernels without modifications).

Regards,
Tobias

Re: [PATCHv2 ipsec] xfrm: fix a warning in xfrm_policy_insert_list

[Xin Long]

On Tue, Jun 9, 2020 at 10:18 PM Tobias Brunner <tobias@strongswan.org> wrote:
>
> Hi Xin,
>
> >> I guess we could workaround this issue in strongSwan by installing
> >> policies that share the same mark and selector with the same priority,
> >> so only one instance is ever installed in the kernel.  But the inability
> >> to address the exact policy when querying/deleting still looks like a
> >> problem to me in general.
> >>
> > For deleting, yes, but for querying, I think it makes sense not to pass
> > the priority, and always get the policy with the highest priority.
>
> While I agree it's less of a problem (at least for strongSwan),  it
> should be possible to query the exact policy one wants.  Because as far
> as I understand, the whole point of Steffen's original patch was that
> all duplicate policies could get used concurrently, depending on the
> marks and masks on them and the traffic, so all of them must be queryable.
>
> But I actually think the previous check that viewed policies with the
> exact same mark and value as duplicates made sense, because those will
> never be used concurrently.  It would at least fix the default behavior
> with strongSwan (users have to configure marks/masks manually).
>
> > We can separate the deleting path from the querying path when
> > XFRMA_PRIORITY attribute is set.
> >
> > Is that enough for your case to only fix for the policy deleting?
>
> While such an attribute could be part of a solution, it does not fix the
> regression your patch created.  The kernel behavior changed and a
> userland modification is required to get back to something resembling
> the previous behavior (without an additional kernel patch we'll actually
> not be able to restore the previous behavior, where we separated
> different types of policies into priority classes).  That is, current
> and old strongSwan versions could create lots of duplicate/lingering
> policies, which is not good.
>
> A problem with such an attribute is how userland would learn when to use
> it.  We could query the kernel version, but patches might get
> backported.  So how can we know the kernel will create duplicates when
> we update a policy and change the priority, which we then have to delete
> (or even can delete with such a new attribute)?  Do we have to do a
> runtime check (e.g. install two duplicate policies with different
> priorities and delete twice to see if the second attempt results in an
> error)?  With marks it's relatively easy as users have to configure them
> explicitly and they work or they don't depending on the kernel version.
>  But here it's not so easy as the IKE daemon uses priorities extensively
> already.
>
> Like the marks it might work somehow if the new attribute also had to be
> passed in the message that creates a policy (marks have to be passed
> with every message, including querying them).  While that's not super
> ideal as we'd have two priority values in these messages (and have to
> keep track of them in the kernel state), there is some precedent with
> the anti-replay config for SAs (which can be passed via xfrm_usersa_info
> struct or as separate attribute with more options for ESN).  Userland
> would still have to learn somehow that the kernel understands the new
> attribute and duplicate policies with different priorities are possible.
>  But if there was any advantage in using this, we could perhaps later
> add an option for users to enable it.  At least the current behavior
> would not change (i.e. older strongSwan versions would continue to run
> on newer kernels without modifications).
>
Now I can see some about how userland is using "priority". We probably
need to revert both this patch and 7cb8a93968e3 ("xfrm: Allow inserting
policies with matching mark and different priorities").

Thanks for the explanation, I will think more about it tomorrow.

Re: [PATCHv2 ipsec] xfrm: fix a warning in xfrm_policy_insert_list

[Xin Long]

On Thu, Jun 11, 2020 at 12:32 AM Xin Long <lucien.xin@gmail.com> wrote:
>
> On Tue, Jun 9, 2020 at 10:18 PM Tobias Brunner <tobias@strongswan.org> wrote:
> >
> > Hi Xin,
> >
> > >> I guess we could workaround this issue in strongSwan by installing
> > >> policies that share the same mark and selector with the same priority,
> > >> so only one instance is ever installed in the kernel.  But the inability
> > >> to address the exact policy when querying/deleting still looks like a
> > >> problem to me in general.
> > >>
> > > For deleting, yes, but for querying, I think it makes sense not to pass
> > > the priority, and always get the policy with the highest priority.
> >
> > While I agree it's less of a problem (at least for strongSwan),  it
> > should be possible to query the exact policy one wants.  Because as far
> > as I understand, the whole point of Steffen's original patch was that
> > all duplicate policies could get used concurrently, depending on the
> > marks and masks on them and the traffic, so all of them must be queryable.
> >
> > But I actually think the previous check that viewed policies with the
> > exact same mark and value as duplicates made sense, because those will
> > never be used concurrently.  It would at least fix the default behavior
> > with strongSwan (users have to configure marks/masks manually).
> >
> > > We can separate the deleting path from the querying path when
> > > XFRMA_PRIORITY attribute is set.
> > >
> > > Is that enough for your case to only fix for the policy deleting?
> >
> > While such an attribute could be part of a solution, it does not fix the
> > regression your patch created.  The kernel behavior changed and a
> > userland modification is required to get back to something resembling
> > the previous behavior (without an additional kernel patch we'll actually
> > not be able to restore the previous behavior, where we separated
> > different types of policies into priority classes).  That is, current
> > and old strongSwan versions could create lots of duplicate/lingering
> > policies, which is not good.
> >
> > A problem with such an attribute is how userland would learn when to use
> > it.  We could query the kernel version, but patches might get
> > backported.  So how can we know the kernel will create duplicates when
> > we update a policy and change the priority, which we then have to delete
> > (or even can delete with such a new attribute)?  Do we have to do a
> > runtime check (e.g. install two duplicate policies with different
> > priorities and delete twice to see if the second attempt results in an
> > error)?  With marks it's relatively easy as users have to configure them
> > explicitly and they work or they don't depending on the kernel version.
> >  But here it's not so easy as the IKE daemon uses priorities extensively
> > already.
> >
> > Like the marks it might work somehow if the new attribute also had to be
> > passed in the message that creates a policy (marks have to be passed
> > with every message, including querying them).  While that's not super
> > ideal as we'd have two priority values in these messages (and have to
> > keep track of them in the kernel state), there is some precedent with
> > the anti-replay config for SAs (which can be passed via xfrm_usersa_info
> > struct or as separate attribute with more options for ESN).  Userland
> > would still have to learn somehow that the kernel understands the new
> > attribute and duplicate policies with different priorities are possible.
> >  But if there was any advantage in using this, we could perhaps later
> > add an option for users to enable it.  At least the current behavior
> > would not change (i.e. older strongSwan versions would continue to run
> > on newer kernels without modifications).
> >
> Now I can see some about how userland is using "priority". We probably
> need to revert both this patch and 7cb8a93968e3 ("xfrm: Allow inserting
> policies with matching mark and different priorities").
>
> Thanks for the explanation, I will think more about it tomorrow.

The issue here in xfrm_mark only exists in xfrm user interface.
It's not consistent when doing get/del and new/update, see below:

NOW:
===
xfrm_get_policy (XFRM_MSG_GETPOLICY):
xfrm_get_policy (XFRM_MSG_DELPOLICY):
        xfrm_policy_byid
        xfrm_policy_bysel_ctx
                __xfrm_policy_bysel_ctx
                        (mark.v & pol->mark.m) == pol->mark.v <--

xfrm_add_policy (XFRM_MSG_NEWPOLICY):
xfrm_add_policy (XFRM_MSG_UPDPOLICY):
        xfrm_policy_insert
                xfrm_policy_insert_list
                xfrm_policy_insert_inexact_list
                        policy->mark.v == pol->mark.v &&
                        policy->priority == pol->priority;  <--


For 'new/update/del', we should do an exact match with
"mark.v == pol->mark.v && mark.m == pol->mark.m", as these are MSGs to
manage the policies, every policy should be able to be matched.

But for 'get', I'm not sure, shouldn't it be working as how it's used
in skb rx/tx path, like in xfrm_policy_match()?
(similar to 'ip route get')
But maybe for ipsec userland it may be different, what do you think?

So my suggestion for the fix is as below, which would also keep the
behavior in commit 7cb8a93968e3 ("xfrm: Allow inserting policies with
matching mark and different priorities").

WITH FIX:
========
xfrm_get_policy (XFRM_MSG_GETPOLICY):
        xfrm_policy_byid
        xfrm_policy_bysel_ctx
                __xfrm_policy_bysel_ctx
                        (mark.v & pol->mark.m) == pol->mark.v <--

xfrm_get_policy (XFRM_MSG_DELPOLICY):
        xfrm_policy_byid
        xfrm_policy_bysel_ctx
                __xfrm_policy_bysel_ctx
                        mark.v == pol->mark.v &&
                        mark.m == pol->mark.m;  <--

xfrm_add_policy (XFRM_MSG_NEWPOLICY):
xfrm_add_policy (XFRM_MSG_UPDPOLICY):
        xfrm_policy_insert
                xfrm_policy_insert_list
                xfrm_policy_insert_inexact_list
                        policy->mark.v == pol->mark.v &&
                        policy->mark.m == pol->mark.m;  <--

Re: [PATCHv2 ipsec] xfrm: fix a warning in xfrm_policy_insert_list

[Tobias Brunner]

Hi Xin,

> For 'new/update/del', we should do an exact match with
> "mark.v == pol->mark.v && mark.m == pol->mark.m", as these are MSGs to
> manage the policies, every policy should be able to be matched.

Agreed, using an exact match for mark/mask would probably make the most
sense here.

> But for 'get', I'm not sure, shouldn't it be working as how it's used
> in skb rx/tx path, like in xfrm_policy_match()?
> (similar to 'ip route get')
> But maybe for ipsec userland it may be different, what do you think?

Interesting idea.  But I don't think it currently has the same semantics
as RTM_GETROUTE, i.e. you don't pass it e.g. some IP addresses and get
the "best" matching policy back.  We use it to query stats (curlft) of a
specific policy.  Basically, we expect to get back the policy added with
XFRM_MSG_NEWPOLICY or updated with XFRM_MSG_UPDPOLICY when we pass the
same selector/mark.  So I think it should work the same way as the
manipulation operations (i.e. it can continue to share the code path
with delete).

Regards,
Tobias