| Message ID | 20210310093611.GA5406@moon.secunet.de (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | xfrm: return error when esp offload is requested and not supported | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | fail | 1 blamed authors not CCed: ilant@mellanox.com; 1 maintainers not CCed: ilant@mellanox.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 323 this patch: 323 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 278 this patch: 278 |
| netdev/header_inline | success | Link |
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. 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?
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,
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@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! 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? > > > 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 agreed. It could be a new feature? > > 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? 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. 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! thanks for the feedback. -antony
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@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! 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.
diff --git a/include/net/xfrm.h b/include/net/xfrm.h index bfbc7810df94..05d9f178093c 100644 --- a/include/net/xfrm.h +++ b/include/net/xfrm.h @@ -1914,7 +1914,7 @@ static inline struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_fea static inline int xfrm_dev_state_add(struct net *net, struct xfrm_state *x, struct xfrm_user_offload *xuo) { - return 0; + return -EINVAL; } static inline void xfrm_dev_state_delete(struct xfrm_state *x) diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c index edf11893dbe8..1e1a9493c8db 100644 --- a/net/xfrm/xfrm_device.c +++ b/net/xfrm/xfrm_device.c @@ -250,7 +250,7 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x, if (!dev->xfrmdev_ops || !dev->xfrmdev_ops->xdo_dev_state_add) { xso->dev = NULL; dev_put(dev); - return 0; + return -EINVAL; } if (x->props.flags & XFRM_STATE_ESN &&
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> --- include/net/xfrm.h | 2 +- net/xfrm/xfrm_device.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) -- 2.20.1