Date:   Tue, 9 Jun 2020 16:18:31 +0200
From:   Tobias Brunner <tobias@...ongswan.org>
To:     Xin Long <lucien.xin@...il.com>
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

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

Powered by blists - more mailing lists