Message-ID: <CADvbK_egkEe0Pw-Yy8eQggS-cfvndOnj7W2hqvpdNxS2xJ50xg@mail.gmail.com>
Date:   Thu, 11 Jun 2020 00:32:46 +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 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.

Powered by blists - more mailing lists