Re: [PATCH] xfrm: return error when esp offload is requested and not supported

[Sabrina Dubroca <sd@queasysnail.net>]

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@secunet.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, 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.

(*) 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.

> Not sure if that was a good idea, but changing this would also change
> the userspace ABI. So if we change this, we should at least not
> consider it as a fix because it would be backported to -stable
> in that case. Thoughts?

Agree, but I don't think we could even change this at all.

At best we could introduce a flag to force offloading, and fail if we
can't offload. But then what should we do if the traffic for that
state is rerouted through a different interface, or if offloading is
temporarily disabled with ethtool? Also, should a kernel with
!CONFIG_XFRM_OFFLOAD ignore that flag or not?

Antony, what prompted you to write this patch? Do you have a use case
for requiring offloading instead of falling back to software?


Thanks,

-- 
Sabrina