[PATCH ipsec] xfrm: check DST_NOPOLICY as well as DST_NOXFRM

[PATCH ipsec] xfrm: check DST_NOPOLICY as well as DST_NOXFRM

[Mark Gillott]

Before performing a policy bundle lookup, check the DST_NOPOLICY
option, as well as DST_NOXFRM. That is, skip further processing if
either of the disable_policy or disable_xfrm sysctl attributes are
set.

Signed-off-by: Mark Gillott <mgillott@vyatta.att-mail.com>
---
 net/xfrm/xfrm_policy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index f2d1e573ea55..a84df1da54d1 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3075,7 +3075,7 @@ struct dst_entry *xfrm_lookup_with_ifid(struct net *net,
 		xflo.flags = flags;
 
 		/* To accelerate a bit...  */
-		if ((dst_orig->flags & DST_NOXFRM) ||
+		if ((dst_orig->flags & (DST_NOXFRM | DST_NOPOLICY)) ||
 		    !net->xfrm.policy_count[XFRM_POLICY_OUT])
 			goto nopol;
 
-- 
2.17.1

Re: [PATCH ipsec] xfrm: check DST_NOPOLICY as well as DST_NOXFRM

[Nicolas Dichtel]

Le 04/12/2019 à 16:17, Mark Gillott a écrit :
> Before performing a policy bundle lookup, check the DST_NOPOLICY
> option, as well as DST_NOXFRM. That is, skip further processing if
> either of the disable_policy or disable_xfrm sysctl attributes are
> set.
Can you elaborate why this change is needed?

Re: [PATCH ipsec] xfrm: check DST_NOPOLICY as well as DST_NOXFRM

[Mark Gillott]

On Wed, 2019-12-04 at 17:57 +0100, Nicolas Dichtel wrote:
> Le 04/12/2019 à 16:17, Mark Gillott a écrit :
> > Before performing a policy bundle lookup, check the DST_NOPOLICY
> > option, as well as DST_NOXFRM. That is, skip further processing if
> > either of the disable_policy or disable_xfrm sysctl attributes are
> > set.
> 
> Can you elaborate why this change is needed?

We have a separate DPDK-based dataplane that is responsible for all
IPsec processing - policy handing/encryption/decryption. Consequently
we set the net.ipv[4|6].conf.<if>.disable_policy sysctl to 1 for all
"interesting" interfaces. That is we want the kernel to ignore any
IPsec policies.

Despite the above & depending on configuration, we found that
originating traffic was ending up deep inside XFRM where it would get
dropped because of a route lookup problem.

Does that help?

Mark

Re: [PATCH ipsec] xfrm: check DST_NOPOLICY as well as DST_NOXFRM

[Nicolas Dichtel]

Le 05/12/2019 à 09:10, Mark Gillott a écrit :
> On Wed, 2019-12-04 at 17:57 +0100, Nicolas Dichtel wrote:
>> Le 04/12/2019 à 16:17, Mark Gillott a écrit :
>>> Before performing a policy bundle lookup, check the DST_NOPOLICY
>>> option, as well as DST_NOXFRM. That is, skip further processing if
>>> either of the disable_policy or disable_xfrm sysctl attributes are
>>> set.
>>
>> Can you elaborate why this change is needed?
> 
> We have a separate DPDK-based dataplane that is responsible for all
> IPsec processing - policy handing/encryption/decryption. Consequently
> we set the net.ipv[4|6].conf.<if>.disable_policy sysctl to 1 for all
> "interesting" interfaces. That is we want the kernel to ignore any
> IPsec policies.
> 
> Despite the above & depending on configuration, we found that
> originating traffic was ending up deep inside XFRM where it would get
> dropped because of a route lookup problem.
And why don't you set disable_xfrm to thoses interfaces also?
disable_policy means no xfrm policy lookup on output, disable_xfrm means no xfrm
policy check on input.

Nicolas

Re: [PATCH ipsec] xfrm: check DST_NOPOLICY as well as DST_NOXFRM

[Mark Gillott]

On Thu, 2019-12-05 at 09:52 +0100, Nicolas Dichtel wrote:
> Le 05/12/2019 à 09:10, Mark Gillott a écrit :
> > On Wed, 2019-12-04 at 17:57 +0100, Nicolas Dichtel wrote:
> > > Le 04/12/2019 à 16:17, Mark Gillott a écrit :
> > > > Before performing a policy bundle lookup, check the
> > > > DST_NOPOLICY
> > > > option, as well as DST_NOXFRM. That is, skip further processing
> > > > if
> > > > either of the disable_policy or disable_xfrm sysctl attributes
> > > > are
> > > > set.
> > > 
> > > Can you elaborate why this change is needed?
> > 
> > We have a separate DPDK-based dataplane that is responsible for all
> > IPsec processing - policy handing/encryption/decryption.
> > Consequently
> > we set the net.ipv[4|6].conf.<if>.disable_policy sysctl to 1 for
> > all
> > "interesting" interfaces. That is we want the kernel to ignore any
> > IPsec policies.
> > 
> > Despite the above & depending on configuration, we found that
> > originating traffic was ending up deep inside XFRM where it would
> > get
> > dropped because of a route lookup problem.
> 
> And why don't you set disable_xfrm to thoses interfaces also?
> disable_policy means no xfrm policy lookup on output, disable_xfrm
> means no xfrm
> policy check on input.
> 

True, setting disable_xfrm=1 would solve the issue. Except this is
output - the test case is a ping from a peer, the corresponding ICMP
response is discarded by the kernel. Feels like disable_policy is the
right check (the kernel is doing XFRM output).

Cheers,
Mark

Re: [PATCH ipsec] xfrm: check DST_NOPOLICY as well as DST_NOXFRM

[Nicolas Dichtel]

Le 05/12/2019 à 11:05, Mark Gillott a écrit :
> On Thu, 2019-12-05 at 09:52 +0100, Nicolas Dichtel wrote:
>> Le 05/12/2019 à 09:10, Mark Gillott a écrit :
>>> On Wed, 2019-12-04 at 17:57 +0100, Nicolas Dichtel wrote:
>>>> Le 04/12/2019 à 16:17, Mark Gillott a écrit :
>>>>> Before performing a policy bundle lookup, check the
>>>>> DST_NOPOLICY
>>>>> option, as well as DST_NOXFRM. That is, skip further processing
>>>>> if
>>>>> either of the disable_policy or disable_xfrm sysctl attributes
>>>>> are
>>>>> set.
>>>>
>>>> Can you elaborate why this change is needed?
>>>
>>> We have a separate DPDK-based dataplane that is responsible for all
>>> IPsec processing - policy handing/encryption/decryption.
>>> Consequently
>>> we set the net.ipv[4|6].conf.<if>.disable_policy sysctl to 1 for
>>> all
>>> "interesting" interfaces. That is we want the kernel to ignore any
>>> IPsec policies.
>>>
>>> Despite the above & depending on configuration, we found that
>>> originating traffic was ending up deep inside XFRM where it would
>>> get
>>> dropped because of a route lookup problem.
>>
>> And why don't you set disable_xfrm to thoses interfaces also?
>> disable_policy means no xfrm policy lookup on output, disable_xfrm
>> means no xfrm
>> policy check on input.
I inverted them! :/
disable_policy => no xfrm policy check on input
disable_xfrm => no xfrm encryption on output

>>
> 
> True, setting disable_xfrm=1 would solve the issue. Except this is
> output - the test case is a ping from a peer, the corresponding ICMP
> response is discarded by the kernel. Feels like disable_policy is the
> right check (the kernel is doing XFRM output).
If you don't want to perform xfrm encryption on output, you have to set
disable_xfrm. disable_policy is for input path only.

Nicolas

Re: [PATCH ipsec] xfrm: check DST_NOPOLICY as well as DST_NOXFRM

[Mark Gillott]

On Thu, 2019-12-05 at 11:51 +0100, Nicolas Dichtel wrote:
> Le 05/12/2019 à 11:05, Mark Gillott a écrit :
> > On Thu, 2019-12-05 at 09:52 +0100, Nicolas Dichtel wrote:
> > > Le 05/12/2019 à 09:10, Mark Gillott a écrit :
> > > > On Wed, 2019-12-04 at 17:57 +0100, Nicolas Dichtel wrote:
> > > > > Le 04/12/2019 à 16:17, Mark Gillott a écrit :
> > > > > > Before performing a policy bundle lookup, check the
> > > > > > DST_NOPOLICY
> > > > > > option, as well as DST_NOXFRM. That is, skip further
> > > > > > processing
> > > > > > if
> > > > > > either of the disable_policy or disable_xfrm sysctl
> > > > > > attributes
> > > > > > are
> > > > > > set.
> > > > > 
> > > > > Can you elaborate why this change is needed?
> > > > 
> > > > We have a separate DPDK-based dataplane that is responsible for
> > > > all
> > > > IPsec processing - policy handing/encryption/decryption.
> > > > Consequently
> > > > we set the net.ipv[4|6].conf.<if>.disable_policy sysctl to 1
> > > > for
> > > > all
> > > > "interesting" interfaces. That is we want the kernel to ignore
> > > > any
> > > > IPsec policies.
> > > > 
> > > > Despite the above & depending on configuration, we found that
> > > > originating traffic was ending up deep inside XFRM where it
> > > > would
> > > > get
> > > > dropped because of a route lookup problem.
> > > 
> > > And why don't you set disable_xfrm to thoses interfaces also?
> > > disable_policy means no xfrm policy lookup on output,
> > > disable_xfrm
> > > means no xfrm
> > > policy check on input.
> 
> I inverted them! :/
> disable_policy => no xfrm policy check on input
> disable_xfrm => no xfrm encryption on output
> 
> > > 
> > 
> > True, setting disable_xfrm=1 would solve the issue. Except this is
> > output - the test case is a ping from a peer, the corresponding
> > ICMP
> > response is discarded by the kernel. Feels like disable_policy is
> > the
> > right check (the kernel is doing XFRM output).
> 
> If you don't want to perform xfrm encryption on output, you have to
> set
> disable_xfrm. disable_policy is for input path only.
> 

Alright happy to drop this patch and we'll use disable_xfrm (as well
as/instead of disable_policy).

Many thanks,
Mark