Hi Steffen,
While adding the ipsec-offload API to netdevsim I ran across an issue
with the use of x->xso.offload_handle that I think needs attention, and
would like your opinion before I try to address it.
The offload_handle is essentially an opaque magic cookie to be used by
the driver to help track the driver's offload information. As such, the
driver fills it in when an SA is setup for an offload, and then the
offload_handle value can be used in the driver's xmit handler to quickly
find the device specific config information for setting up each packet.
Since this is driver specific information the stack code, e.g.
xfrm_dev_offload_ok() and validate_xmit_xfrm() and a couple of esp
functions, really shouldn't be trying to use it. However, since they
are checking offload_handle for == 0 to see if it has been used, the
drivers need to be sure they have set it to non-zero. This requirement
is not clear in the API description at the moment.
I ran across this because my addition to netdevsim uses offload_handle
to store the index of the array item that has the related SA
information. I found in testing that the Tx offload wasn't getting
called because the array index was 0 and so xfrm_dev_offload_ok() would
think there was no offload and bypass the offloaded Tx. It turns out
that the ixgbe implementation has the same bug, but I missed this in
earlier testing. The Mellanox driver doesn't run into this problem
because they are storing a struct pointer rather than an array index.
Does the stack really need to check offload_handle? I don't think it
does, but perhaps I'm missing something. Here's where I found it used:
- validate_xmit_xfrm() - added in 3dca3f38
- xfrm_dev_offload_ok() - added in original patch d77e38e6
- esp4_gso_segment() - added in 3dca3f38
- esp_xmit() - added in fca11ebd
- esp6_gso_segment() - added in 3dca3f38
- esp6_xmit() - added in 3dca3f38
I think in all cases it is unnecessary and could be pulled out.
If these checks need to be left in, then any client drivers need to be
sure this is a non-zero value, possibly by adding a VALID bit to their
opaque value. The ixgbe and netdevsim implementations would be fine
with using a high bit as a VALID flag, but that might not play well with
drivers like Mellanox that store a struct address.
So, after all that... shall we remove the offload_handle references in
the stack code?
sln
--
==================================================
Shannon Nelson shannon.nel...@oracle.com
Parents can't afford to be squeamish
