Message-ID: <CADvbK_faty7VayrEfgJ-Hcr2Ah-2au6uFvq1hU0ofdNZHODNHA@mail.gmail.com>
Date:   Thu, 11 Jun 2020 16:48:17 +0800
From:   Xin Long <lucien.xin@...il.com>
To:     Tobias Brunner <tobias@...ongswan.org>
Cc:     network dev <netdev@...r.kernel.org>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Sabrina Dubroca <sd@...asysnail.net>,
        Yuehaibing <yuehaibing@...wei.com>,
        Andreas Steffen <andreas.steffen@...ongswan.org>
Subject: Re: [PATCHv2 ipsec] xfrm: fix a warning in xfrm_policy_insert_list

On Thu, Jun 11, 2020 at 12:32 AM Xin Long <lucien.xin@...il.com> wrote:
>
> On Tue, Jun 9, 2020 at 10:18 PM Tobias Brunner <tobias@...ongswan.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;  <--

Powered by blists - more mailing lists