[Intel-wired-lan] [RFC PATCH jkirsher/next-queue 3/9] ixgbe: add the ipsec support code files

[Alexander Duyck]

On Sat, Nov 18, 2017 at 12:18 PM, Shannon Nelson
<shannon.nelson at oracle.com> wrote:
> Again, thanks for the review on this early code.  I have a few responses
> below.
>
> sln

No problem. It is a nice break from working on macvlan code where
ixgbe seems to have more "undocumented features" than I know what to
do with.. :-)

I had a couple responses to your responses included them inline below
and snipped the portions of the patch I think we are in agreement on.

> On 11/17/2017 1:30 PM, Alexander Duyck wrote:
>>
>> On Thu, Nov 16, 2017 at 11:54 AM, Shannon Nelson
>> <shannon.nelson at oracle.com> wrote:
>>>
>>> This adds the ipsec support code into the driver, but doesn't yet
>>> include it into the compile.
>>>
>>> It is possible that the .h file contents could simply go into
>>> the ixgbe_type.h and ixgbe.h files.
>>>
>>> Signed-off-by: Shannon Nelson <shannon.nelson at oracle.com>
>>
>>
>> Generally I really hate it when code is just thrown in like this as it
>> is really difficult to review and/or debug. I would really much prefer
>> to have this code added as it is needed for specific functionality.
>>
>>> ---
>>>   drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 1133
>>> ++++++++++++++++++++++++
>>>   drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h |   80 ++
>>>   2 files changed, 1213 insertions(+)
>>>   create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>>   create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.h
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> new file mode 100644
>>> index 0000000..177b915
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>> @@ -0,0 +1,1133 @@
>>>

<snip>

>>> +
>>> +       /* decode error bits, if any */
>>> +       if ((status_error & IXGBE_RXDADV_ERR_IPSEC_AUTH_FAILED) ==
>>> IXGBE_RXDADV_ERR_IPSEC_AUTH_FAILED) {
>>> +               if (pkt_info & IXGBE_RXDADV_PKTTYPE_IPSEC_AH)
>>> +                       crypto_status = CRYPTO_TRANSPORT_AH_AUTH_FAILED;
>>> +               else if (pkt_info & IXGBE_RXDADV_PKTTYPE_IPSEC_ESP)
>>> +                       crypto_status = CRYPTO_TRANSPORT_ESP_AUTH_FAILED;
>>> +       } else if (status_error & IXGBE_RXDADV_ERR_IPSEC_INV_PROTOCOL) {
>>> +               crypto_status = CRYPTO_INVALID_PROTOCOL;
>>> +       } else if (status_error & IXGBE_RXDADV_ERR_IPSEC_INV_LENGTH) {
>>> +               crypto_status = CRYPTO_INVALID_PACKET_SYNTAX;
>>> +       } else {
>>> +               crypto_status = CRYPTO_SUCCESS;
>>> +       }
>>> +
>>
>>
>> You might look at converting this into a look-up table since you have
>> essentially 2 protocols (AH or ESP), and 4 possible return values that
>> have to be translated int the final result value. A single 2
>> dimensional 8 byte array should be enough to handle all the possible
>> crypto_status return values.
>
>
> Actually, if we throw the packet away earlier in the path when looking at
> the IXGBE_RXDADV_ERR_FRAME_ERR_MASK macro, we won't need to look for these
> error bits at all.

The question is if it is valid to throw away the frame or if we should
handle this like we handle a checksum error and just pass it up the
stack to be processed. I assume if we have an error we don't decode
the frame. If so we could probably pass the frame up with no offloads
at all and let the stack deal with it all. That way if there end up
being any false bugs in the hardware we don't end up dropping valid
frames.

>>
>>> +       skb->sp = secpath_dup(skb->sp);
>>> +       if (unlikely(!skb->sp))
>>> +               return;
>>> +
>>
>>
>> It seems like this should be done as soon as you have found your
>> transform state. Otherwise there isn't much point in decoding the
>> status_error bits.
>>
>>> +       skb->sp->xvec[skb->sp->len++] = xs;
>>> +       skb->sp->olen++;
>>> +       xo = xfrm_offload(skb);
>>> +       xo->flags = CRYPTO_DONE;
>>> +       xo->status = crypto_status;
>>> +
>>> +       if (xo->status == CRYPTO_SUCCESS)
>>> +               rx_ring->rx_stats.ipsec_offloads++;
>>> +       else
>>> +               rx_ring->rx_stats.ipsec_offload_faileds++;
>>
>>
>> You might rename offload_faileds to offload_errors, since the word
>> "failed" isn't really something to pluralize.
>
>
> I'm trying to decide if I want to keep these stats around at all.

I'd say the stats are useful so they could probably be kept. If
nothing else they tell you if the offload is on, and if it is working
or not.