Message-ID: <d9fcca46-476a-1788-6049-c86bf5afb7c5@oracle.com>
Date:   Wed, 6 Dec 2017 21:43:31 -0800
From:   Shannon Nelson <shannon.nelson@...cle.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     intel-wired-lan <intel-wired-lan@...ts.osuosl.org>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Sowmini Varadhan <sowmini.varadhan@...cle.com>,
        Netdev <netdev@...r.kernel.org>
Subject: Re: [Intel-wired-lan] [next-queue 05/10] ixgbe: implement ipsec add
 and remove of offloaded SA

On 12/5/2017 9:26 AM, Alexander Duyck wrote:
> On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
> <shannon.nelson@...cle.com> wrote:
>> Add the functions for setting up and removing offloaded SAs (Security
>> Associations) with the x540 hardware.  We set up the callback structure
>> but we don't yet set the hardware feature bit to be sure the XFRM service
>> won't actually try to use us for an offload yet.
>>
>> The software tables are made up to mimic the hardware tables to make it
>> easier to track what's in the hardware, and the SA table index is used
>> for the XFRM offload handle.  However, there is a hashing field in the
>> Rx SA tracking that will be used to facilitate faster table searches in
>> the Rx fast path.
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@...cle.com>
>> ---
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 377 +++++++++++++++++++++++++
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |   6 +
>>   2 files changed, 383 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> index 38a1a16..7b01d92 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> @@ -26,6 +26,8 @@
>>    ******************************************************************************/
>>
>>   #include "ixgbe.h"
>> +#include <net/xfrm.h>
>> +#include <crypto/aead.h>
>>
>>   /**
>>    * ixgbe_ipsec_set_tx_sa - set the Tx SA registers
>> @@ -128,6 +130,7 @@ static void ixgbe_ipsec_set_rx_ip(struct ixgbe_hw *hw, u16 idx, u32 addr[])
>>    **/
>>   void ixgbe_ipsec_clear_hw_tables(struct ixgbe_adapter *adapter)
>>   {
>> +       struct ixgbe_ipsec *ipsec = adapter->ipsec;
>>          struct ixgbe_hw *hw = &adapter->hw;
>>          u32 buf[4] = {0, 0, 0, 0};
>>          u16 idx;
>> @@ -139,9 +142,11 @@ void ixgbe_ipsec_clear_hw_tables(struct ixgbe_adapter *adapter)
>>          /* scrub the tables */
>>          for (idx = 0; idx < IXGBE_IPSEC_MAX_SA_COUNT; idx++)
>>                  ixgbe_ipsec_set_tx_sa(hw, idx, buf, 0);
>> +       ipsec->num_tx_sa = 0;
>>
>>          for (idx = 0; idx < IXGBE_IPSEC_MAX_SA_COUNT; idx++)
>>                  ixgbe_ipsec_set_rx_sa(hw, idx, 0, buf, 0, 0, 0);
>> +       ipsec->num_rx_sa = 0;
>>
>>          for (idx = 0; idx < IXGBE_IPSEC_MAX_RX_IP_COUNT; idx++)
>>                  ixgbe_ipsec_set_rx_ip(hw, idx, buf);
>> @@ -287,11 +292,383 @@ static void ixgbe_ipsec_start_engine(struct ixgbe_adapter *adapter)
>>   }
>>
>>   /**
>> + * ixgbe_ipsec_find_empty_idx - find the first unused security parameter index
>> + * @ipsec: pointer to ipsec struct
>> + * @rxtable: true if we need to look in the Rx table
>> + *
>> + * Returns the first unused index in either the Rx or Tx SA table
>> + **/
>> +static int ixgbe_ipsec_find_empty_idx(struct ixgbe_ipsec *ipsec, bool rxtable)
>> +{
>> +       u32 i;
>> +
>> +       if (rxtable) {
>> +               if (ipsec->num_rx_sa == IXGBE_IPSEC_MAX_SA_COUNT)
>> +                       return -ENOSPC;
>> +
>> +               /* search rx sa table */
>> +               for (i = 0; i < IXGBE_IPSEC_MAX_SA_COUNT; i++) {
>> +                       if (!ipsec->rx_tbl[i].used)
>> +                               return i;
>> +               }
>> +       } else {
>> +               if (ipsec->num_rx_sa == IXGBE_IPSEC_MAX_SA_COUNT)
>> +                       return -ENOSPC;
> 
> Should this bi num_tx_sa?

Hmm - can you say cut-and-paste?  Will fix.

> 
>> +
>> +               /* search tx sa table */
>> +               for (i = 0; i < IXGBE_IPSEC_MAX_SA_COUNT; i++) {
>> +                       if (!ipsec->tx_tbl[i].used)
>> +                               return i;
>> +               }
>> +       }
>> +
>> +       return -ENOSPC;
>> +}
>> +
>> +/**
>> + * ixgbe_ipsec_parse_proto_keys - find the key and salt based on the protocol
>> + * @xs: pointer to xfrm_state struct
>> + * @mykey: pointer to key array to populate
>> + * @mysalt: pointer to salt value to populate
>> + *
>> + * This copies the protocol keys and salt to our own data tables.  The
>> + * 82599 family only supports the one algorithm.
>> + **/
>> +static int ixgbe_ipsec_parse_proto_keys(struct xfrm_state *xs,
>> +                                       u32 *mykey, u32 *mysalt)
>> +{
>> +       struct net_device *dev = xs->xso.dev;
>> +       unsigned char *key_data;
>> +       char *alg_name = NULL;
>> +       char *aes_gcm_name = "rfc4106(gcm(aes))";
> 
> aes_gcm_name should probably be a static const char array instead of a pointer.

Sure.

> 
>> +       int key_len;
>> +
>> +       if (xs->aead) {
>> +               key_data = &xs->aead->alg_key[0];
>> +               key_len = xs->aead->alg_key_len;
>> +               alg_name = xs->aead->alg_name;
>> +       } else {
>> +               netdev_err(dev, "Unsupported IPsec algorithm\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (strcmp(alg_name, aes_gcm_name)) {
>> +               netdev_err(dev, "Unsupported IPsec algorithm - please use %s\n",
>> +                          aes_gcm_name);
>> +               return -EINVAL;
>> +       }
>> +
>> +       /* 160 accounts for 16 byte key and 4 byte salt */
>> +       if (key_len == 128) {
>> +               netdev_info(dev, "IPsec hw offload parameters missing 32 bit salt value\n");
>> +       } else if (key_len != 160) {
>> +               netdev_err(dev, "IPsec hw offload only supports keys up to 128 bits with a 32 bit salt\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       /* The key bytes come down in a bigendian array of bytes, and
>> +        * salt is always the last 4 bytes of the key array.
>> +        * We don't need to do any byteswapping.
>> +        */
>> +       memcpy(mykey, key_data, 16);
>> +       if (key_len == 160)
>> +               *mysalt = ((u32 *)key_data)[4];
>> +       else
>> +               *mysalt = 0;
> 
> You could combine these key_len checks into a single if/else set.
> Basically just do something like the following:

Alex, ever the reductionist :-)
Yep, makes sense.

> 
> /* 160 accounts for 16 byte key and 4 byte salt */
> if (key_len == 160) {
>           *mysalt = ((u32 *)key_data)[4];
> } else if (key_len != 128) {
>          netdev_err(dev, "IPsec hw offload only supports keys up to 128
> bits with a 32 bit salt\n");
>          return -EINVAL;
> } else {
>          netdev_info(dev, "IPsec hw offload parameters missing 32 bit
> salt value\n");
>          *mysalt = 0;
> }
> 
>   /* The key bytes come down in a bigendian array of bytes, and
>    * salt is always the last 4 bytes of the key array.
>    * We don't need to do any byteswapping.
>    */
> memcpy(mykey, key_data, 16);
> 
>> +
>> +       return 0;
>> +}
>> +
>> +/**
>> + * ixgbe_ipsec_add_sa - program device with a security association
>> + * @xs: pointer to transformer state struct
>> + **/
>> +static int ixgbe_ipsec_add_sa(struct xfrm_state *xs)
>> +{
>> +       struct net_device *dev = xs->xso.dev;
>> +       struct ixgbe_adapter *adapter = netdev_priv(dev);
>> +       struct ixgbe_ipsec *ipsec = adapter->ipsec;
>> +       struct ixgbe_hw *hw = &adapter->hw;
>> +       int checked, match, first;
>> +       u16 sa_idx;
>> +       int ret;
>> +       int i;
>> +
>> +       if (xs->id.proto != IPPROTO_ESP && xs->id.proto != IPPROTO_AH) {
>> +               netdev_err(dev, "Unsupported protocol 0x%04x for ipsec offload\n",
>> +                          xs->id.proto);
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (xs->xso.flags & XFRM_OFFLOAD_INBOUND) {
>> +               struct rx_sa rsa;
>> +
>> +               if (xs->calg) {
>> +                       netdev_err(dev, "Compression offload not supported\n");
>> +                       return -EINVAL;
>> +               }
>> +
>> +               /* find the first unused index */
>> +               ret = ixgbe_ipsec_find_empty_idx(ipsec, true);
>> +               if (ret < 0) {
>> +                       netdev_err(dev, "No space for SA in Rx table!\n");
>> +                       return ret;
>> +               }
>> +               sa_idx = (u16)ret;
>> +
>> +               memset(&rsa, 0, sizeof(rsa));
>> +               rsa.used = true;
>> +               rsa.xs = xs;
>> +
>> +               if (rsa.xs->id.proto & IPPROTO_ESP)
>> +                       rsa.decrypt = xs->ealg || xs->aead;
>> +
>> +               /* get the key and salt */
>> +               ret = ixgbe_ipsec_parse_proto_keys(xs, rsa.key, &rsa.salt);
>> +               if (ret) {
>> +                       netdev_err(dev, "Failed to get key data for Rx SA table\n");
>> +                       return ret;
>> +               }
>> +
>> +               /* get ip for rx sa table */
>> +               if (xs->xso.flags & XFRM_OFFLOAD_IPV6)
>> +                       memcpy(rsa.ipaddr, &xs->id.daddr.a6, 16);
>> +               else
>> +                       memcpy(&rsa.ipaddr[3], &xs->id.daddr.a4, 4);
>> +
>> +               /* The HW does not have a 1:1 mapping from keys to IP addrs, so
>> +                * check for a matching IP addr entry in the table.  If the addr
>> +                * already exists, use it; else find an unused slot and add the
>> +                * addr.  If one does not exist and there are no unused table
>> +                * entries, fail the request.
>> +                */
>> +
>> +               /* Find an existing match or first not used, and stop looking
>> +                * after we've checked all we know we have.
>> +                */
>> +               checked = 0;
>> +               match = -1;
>> +               first = -1;
>> +               for (i = 0;
>> +                    i < IXGBE_IPSEC_MAX_RX_IP_COUNT &&
>> +                    (checked < ipsec->num_rx_sa || first < 0);
>> +                    i++) {
>> +                       if (ipsec->ip_tbl[i].used) {
>> +                               if (!memcmp(ipsec->ip_tbl[i].ipaddr,
>> +                                           rsa.ipaddr, sizeof(rsa.ipaddr))) {
>> +                                       match = i;
>> +                                       break;
>> +                               }
>> +                               checked++;
>> +                       } else if (first < 0) {
>> +                               first = i;  /* track the first empty seen */
>> +                       }
>> +               }
>> +
>> +               if (ipsec->num_rx_sa == 0)
>> +                       first = 0;
>> +
>> +               if (match >= 0) {
>> +                       /* addrs are the same, we should use this one */
>> +                       rsa.iptbl_ind = match;
>> +                       ipsec->ip_tbl[match].ref_cnt++;
>> +
>> +               } else if (first >= 0) {
>> +                       /* no matches, but here's an empty slot */
>> +                       rsa.iptbl_ind = first;
>> +
>> +                       memcpy(ipsec->ip_tbl[first].ipaddr,
>> +                              rsa.ipaddr, sizeof(rsa.ipaddr));
>> +                       ipsec->ip_tbl[first].ref_cnt = 1;
>> +                       ipsec->ip_tbl[first].used = true;
>> +
>> +                       ixgbe_ipsec_set_rx_ip(hw, rsa.iptbl_ind, rsa.ipaddr);
>> +
>> +               } else {
>> +                       /* no match and no empty slot */
>> +                       netdev_err(dev, "No space for SA in Rx IP SA table\n");
>> +                       memset(&rsa, 0, sizeof(rsa));
>> +                       return -ENOSPC;
>> +               }
>> +
>> +               rsa.mode = IXGBE_RXMOD_VALID;
>> +               if (rsa.xs->id.proto & IPPROTO_ESP)
>> +                       rsa.mode |= IXGBE_RXMOD_PROTO_ESP;
>> +               if (rsa.decrypt)
>> +                       rsa.mode |= IXGBE_RXMOD_DECRYPT;
>> +               if (rsa.xs->xso.flags & XFRM_OFFLOAD_IPV6)
>> +                       rsa.mode |= IXGBE_RXMOD_IPV6;
>> +
>> +               /* the preparations worked, so save the info */
>> +               memcpy(&ipsec->rx_tbl[sa_idx], &rsa, sizeof(rsa));
>> +
>> +               ixgbe_ipsec_set_rx_sa(hw, sa_idx, rsa.xs->id.spi, rsa.key,
>> +                                     rsa.salt, rsa.mode, rsa.iptbl_ind);
>> +               xs->xso.offload_handle = sa_idx + IXGBE_IPSEC_BASE_RX_INDEX;
>> +
>> +               ipsec->num_rx_sa++;
>> +
>> +               /* hash the new entry for faster search in Rx path */
>> +               hash_add_rcu(ipsec->rx_sa_list, &ipsec->rx_tbl[sa_idx].hlist,
>> +                            rsa.xs->id.spi);
>> +       } else {
>> +               struct tx_sa tsa;
>> +
>> +               /* find the first unused index */
>> +               ret = ixgbe_ipsec_find_empty_idx(ipsec, false);
>> +               if (ret < 0) {
>> +                       netdev_err(dev, "No space for SA in Tx table\n");
>> +                       return ret;
>> +               }
>> +               sa_idx = (u16)ret;
>> +
>> +               memset(&tsa, 0, sizeof(tsa));
>> +               tsa.used = true;
>> +               tsa.xs = xs;
>> +
>> +               if (xs->id.proto & IPPROTO_ESP)
>> +                       tsa.encrypt = xs->ealg || xs->aead;
>> +
>> +               ret = ixgbe_ipsec_parse_proto_keys(xs, tsa.key, &tsa.salt);
>> +               if (ret) {
>> +                       netdev_err(dev, "Failed to get key data for Tx SA table\n");
>> +                       memset(&tsa, 0, sizeof(tsa));
>> +                       return ret;
>> +               }
>> +
>> +               /* the preparations worked, so save the info */
>> +               memcpy(&ipsec->tx_tbl[sa_idx], &tsa, sizeof(tsa));
>> +
>> +               ixgbe_ipsec_set_tx_sa(hw, sa_idx, tsa.key, tsa.salt);
>> +
>> +               xs->xso.offload_handle = sa_idx + IXGBE_IPSEC_BASE_TX_INDEX;
>> +
>> +               ipsec->num_tx_sa++;
>> +       }
>> +
>> +       /* enable the engine if not already warmed up */
>> +       if (!(adapter->flags2 & IXGBE_FLAG2_IPSEC_ENABLED)) {
>> +               ixgbe_ipsec_start_engine(adapter);
>> +               adapter->flags2 |= IXGBE_FLAG2_IPSEC_ENABLED;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +/**
>> + * ixgbe_ipsec_del_sa - clear out this specific SA
>> + * @xs: pointer to transformer state struct
>> + **/
>> +static void ixgbe_ipsec_del_sa(struct xfrm_state *xs)
>> +{
>> +       struct net_device *dev = xs->xso.dev;
>> +       struct ixgbe_adapter *adapter = netdev_priv(dev);
>> +       struct ixgbe_ipsec *ipsec = adapter->ipsec;
>> +       struct ixgbe_hw *hw = &adapter->hw;
>> +       u32 zerobuf[4] = {0, 0, 0, 0};
>> +       u16 sa_idx;
>> +
>> +       if (xs->xso.flags & XFRM_OFFLOAD_INBOUND) {
>> +               struct rx_sa *rsa;
>> +               u8 ipi;
>> +
>> +               sa_idx = xs->xso.offload_handle - IXGBE_IPSEC_BASE_RX_INDEX;
>> +               rsa = &ipsec->rx_tbl[sa_idx];
>> +
>> +               if (!rsa->used) {
>> +                       netdev_err(dev, "Invalid Rx SA selected sa_idx=%d offload_handle=%lu\n",
>> +                                  sa_idx, xs->xso.offload_handle);
>> +                       return;
>> +               }
>> +
>> +               ixgbe_ipsec_set_rx_sa(hw, sa_idx, 0, zerobuf, 0, 0, 0);
>> +               hash_del_rcu(&rsa->hlist);
>> +
>> +               /* if the IP table entry is referenced by only this SA,
>> +                * i.e. ref_cnt is only 1, clear the IP table entry as well
>> +                */
>> +               ipi = rsa->iptbl_ind;
>> +               if (ipsec->ip_tbl[ipi].ref_cnt > 0) {
>> +                       ipsec->ip_tbl[ipi].ref_cnt--;
>> +
>> +                       if (!ipsec->ip_tbl[ipi].ref_cnt) {
>> +                               memset(&ipsec->ip_tbl[ipi], 0,
>> +                                      sizeof(struct rx_ip_sa));
>> +                               ixgbe_ipsec_set_rx_ip(hw, ipi, zerobuf);
>> +                       }
>> +               }
>> +
>> +               memset(rsa, 0, sizeof(struct rx_sa));
>> +               ipsec->num_rx_sa--;
>> +       } else {
>> +               sa_idx = xs->xso.offload_handle - IXGBE_IPSEC_BASE_TX_INDEX;
>> +
>> +               if (!ipsec->tx_tbl[sa_idx].used) {
>> +                       netdev_err(dev, "Invalid Tx SA selected sa_idx=%d offload_handle=%lu\n",
>> +                                  sa_idx, xs->xso.offload_handle);
>> +                       return;
>> +               }
>> +
>> +               ixgbe_ipsec_set_tx_sa(hw, sa_idx, zerobuf, 0);
>> +               memset(&ipsec->tx_tbl[sa_idx], 0, sizeof(struct tx_sa));
>> +               ipsec->num_tx_sa--;
>> +       }
>> +
>> +       /* if there are no SAs left, stop the engine to save energy */
>> +       if (ipsec->num_rx_sa == 0 && ipsec->num_tx_sa == 0) {
>> +               adapter->flags2 &= ~IXGBE_FLAG2_IPSEC_ENABLED;
>> +               ixgbe_ipsec_stop_engine(adapter);
>> +       }
>> +}
>> +
>> +static const struct xfrmdev_ops ixgbe_xfrmdev_ops = {
>> +       .xdo_dev_state_add = ixgbe_ipsec_add_sa,
>> +       .xdo_dev_state_delete = ixgbe_ipsec_del_sa,
>> +};
>> +
>> +/**
>>    * ixgbe_init_ipsec_offload - initialize security registers for IPSec operation
>>    * @adapter: board private structure
>>    **/
>>   void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter)
>>   {
>> +       struct ixgbe_ipsec *ipsec;
>> +       size_t size;
>> +
>> +       ipsec = kzalloc(sizeof(*ipsec), GFP_KERNEL);
>> +       if (!ipsec)
>> +               goto err;
> 
> I would say just add another label to skip over the if statement you
> added below.

Yep.

> 
>> +       hash_init(ipsec->rx_sa_list);
>> +
>> +       size = sizeof(struct rx_sa) * IXGBE_IPSEC_MAX_SA_COUNT;
>> +       ipsec->rx_tbl = kzalloc(size, GFP_KERNEL);
>> +       if (!ipsec->rx_tbl)
>> +               goto err;
>> +
>> +       size = sizeof(struct tx_sa) * IXGBE_IPSEC_MAX_SA_COUNT;
>> +       ipsec->tx_tbl = kzalloc(size, GFP_KERNEL);
>> +       if (!ipsec->tx_tbl)
>> +               goto err;
>> +
>> +       size = sizeof(struct rx_ip_sa) * IXGBE_IPSEC_MAX_RX_IP_COUNT;
>> +       ipsec->ip_tbl = kzalloc(size, GFP_KERNEL);
>> +       if (!ipsec->ip_tbl)
>> +               goto err;
> 
> Do all these tables need to be allocated separately? I'm just
> wondering if we can get away with doing something like what we did
> with the ixgbe_q_vector structure where you just allocate this as one
> physical block of memory and just split it up into multiple chunks
> with a separate pointer to each chunk. Doing that would cut down on
> the exception handling needed since it would be a single allocation
> failure you would have to deal with.

This may really just come down to style, and my thoughts around this are 
relatively trivial:
  - Is it nicer to the memory system to do one large alloc or a couple 
of smaller ones?
  - If any bounds-checking scans are done, this method would allow for 
checking, while I think the single large alloc wouldn't be as good for 
bounds checking between these tables.

> 
>> +       ipsec->num_rx_sa = 0;
>> +       ipsec->num_tx_sa = 0;
>> +
>> +       adapter->ipsec = ipsec;
>>          ixgbe_ipsec_clear_hw_tables(adapter);
>>          ixgbe_ipsec_stop_engine(adapter);

By the way, I hink I need to turn these two around and make sure the 
engine is stopped first.  It just seems right.

>> +
>> +       return;
>> +err:
>> +       if (ipsec) {
>> +               kfree(ipsec->ip_tbl);
>> +               kfree(ipsec->rx_tbl);
>> +               kfree(ipsec->tx_tbl);
>> +               kfree(adapter->ipsec);
>> +       }
>> +       netdev_err(adapter->netdev, "Unable to allocate memory for SA tables");
>>   }
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 51fb3cf..01fd89b 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -10542,6 +10542,12 @@ static void ixgbe_remove(struct pci_dev *pdev)
>>          set_bit(__IXGBE_REMOVING, &adapter->state);
>>          cancel_work_sync(&adapter->service_task);
>>
>> +#ifdef CONFIG_XFRM
>> +       kfree(adapter->ipsec->ip_tbl);
>> +       kfree(adapter->ipsec->rx_tbl);
>> +       kfree(adapter->ipsec->tx_tbl);
>> +       kfree(adapter->ipsec);
>> +#endif /* CONFIG_XFRM */
> 
> It might be useful if you were to move this into a function of its
> own. Also you should probably check for adapter->ipsec first,
> otherwise you are going to cause NULL pointer dereference any time
> adapter->ipsec isn't defined. because you are dereferencing it when
> you go to free each of those tables.

Yep

Thanks,
sln

> 
>>
>>   #ifdef CONFIG_IXGBE_DCA
>>          if (adapter->flags & IXGBE_FLAG_DCA_ENABLED) {
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan@...osl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Powered by blists - more mailing lists