On Tue, Jun 9, 2020 at 10:18 PM Tobias Brunner <tob...@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.
