Message-ID: <YFTlAoPPWOTG0AAf@hog>
Date:   Fri, 19 Mar 2021 18:53:06 +0100
From:   Sabrina Dubroca <sd@...asysnail.net>
To:     Antony Antony <antony.antony@...unet.com>
Cc:     Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Yossi Kuperman <yossiku@...lanox.com>,
        Guy Shapiro <guysh@...lanox.com>, netdev@...r.kernel.org,
        antony@...nome.org
Subject: Re: [PATCH] xfrm: return error when esp offload is requested and not
 supported

2021-03-17, 09:42:43 +0100, Antony Antony wrote:
> Hi,
> 
> On Mon, Mar 15, 2021 at 16:29:59 +0100, Sabrina Dubroca wrote:
> > 2021-03-15, 11:43:50 +0100, Steffen Klassert wrote:
> > > On Wed, Mar 10, 2021 at 10:36:11AM +0100, Antony Antony wrote:
> > > > When ESP offload is not supported by the device return an error,
> > > > -EINVAL, instead of silently ignoring it, creating a SA without offload,
> > > > and returning success.
> > > > 
> > > > with this fix ip x s a would return
> > > > RTNETLINK answers: Invalid argument
> > > > 
> > > > Also, return an error, -EINVAL, when CONFIG_XFRM_OFFLOAD is
> > > > not defined and the user is trying to create an SA with the offload.
> > > > 
> > > > Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
> > > > Signed-off-by: Antony Antony <antony.antony@...unet.com>
> > > 
> > > I feal a bit unease about this one. When we designed the offloading
> > > API, we decided to fallback to software if HW offload is not available.
> 
> Right! Now, I recollect the silent fallback feature, which was mentioned
> when the offload was introduced. However, while using I found it
> confusing. And I thought it was an unintended side effect and came up
> with a fix. Looking closer at the proposed patch and commit 4a132095dd6 I
> wonder what would be a consistent behavior.
> 
> 
> > Right, but it's a little bit inconsistent. If HW offload is not
> > available, we get silent fallback. This is great for compatibility
> > (old kernels will completely ignore the XFRMA_OFFLOAD_DEV attribute,
> > new kernels try to emulate this), and because routes can change and
> > suddenly the packets that should have been going through some device
> > go through another, which may have different capabilities.
> > 
> > On the other hand, if HW offload is available but doesn't support the
> > exact features we're trying to enable (UDP encap, wrong algorithm, etc
> > (*)), then we can fail in a visible way.
> 
> Yes.
> 
> > (*) I know there's an "if (err != -EOPNOTSUPP)" on the result of
> > ->xdo_dev_state_add(), but for example mlx5 seems to return EINVAL
> > instead of EOPNOTSUPP.
> 
> That is interesting. I think !CONFIG_XFRM_OFFLOAD should be
> EOPNOTSUPP?

That would be inconsistent behavior between !CONFIG_XFRM_OFFLOAD and
CONFIG_XFRM_OFFLOAD, besides changing the existing behavior.

> > Antony, what prompted you to write this patch? Do you have a use case
> > for requiring offloading instead of falling back to software?
> 
> I was using strongSWAN. Due to NIC changes and route changes the SA,
> with offload, failed to create.
> Actually, strongSWAN didn't even try to create a SA. It reported an
> error when it checked offload support on the device, after IKE
> negotiation just before installing the SA.
> 
> [KNL] 192.168.1.1 is on interface eth2
> [KNL] HW offload is not supported by device
> 
> To debug I tried "ip x s a eth2" and a the SA got created, which
> confused me. After a closer showed offload was silently ignored.
> 
> Next I tried to create an offload to loopback device.
> "ip x s a ... offload dev lo dir in" and there was an SA without
> offload, Device lo will never have offload:)
> This led me to look at the kernel code. There I found the offload
> request can be silently ignored. So I created a fix.
> 
> One important point to note: IKE daemons check for offload support,
> using ETHTOOL_GFEATURES or ETH_SS_FEATURES, before requesting kernel
> to add a SA with offload. I see strongSWAN and libreswan trying to
> detect it before requesting offload. This why I got the error.

That kind of check is somewhere between "kind of unreliable" and
"completely bogus", considering that between the check and SA
insertion, the IP address could move to a different device, the route
could change, IPsec offload could be disabled or enabled on the
device, or the device could entirely disappear from the system. Most
of the time it's probably going to turn out ok, but as you noticed, it
can fail pretty spectacularly.

> I had a closeer look and I noticed that deciding when to fail and when
> to fallback is complex. e.g no NAT support with offload is failure,
> -EINVAL and not a fallback.
> 
> I think the current kernel, when it has code to support a feature,
> should fail explictly when the feature requested can't be
> enabled. While, an older kernel which is not aware of the feature may
> silently ignore an unsupported feature. Atleast that is how I feel
> now!

I'd lean toward keeping the current behavior as is (because changing
behavior isn't that great, and because it seems a bit less likely to
trip people up), and possibly introduce a "force offload" option.

Thanks for explaining.

-- 
Sabrina

Powered by blists - more mailing lists