[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
<SN7PR18MB531442D0D88031D320F3372BE3E32@SN7PR18MB5314.namprd18.prod.outlook.com>
Date: Tue, 14 May 2024 06:52:38 +0000
From: Bharat Bhushan <bbhushan2@...vell.com>
To: Simon Horman <horms@...nel.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Sunil Kovvuri
Goutham <sgoutham@...vell.com>,
Geethasowjanya Akula <gakula@...vell.com>,
Subbaraya Sundeep Bhatta <sbhatta@...vell.com>,
Hariprasad Kelam
<hkelam@...vell.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"kuba@...nel.org"
<kuba@...nel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>,
Jerin Jacob
<jerinj@...vell.com>, Linu Cherian <lcherian@...vell.com>,
"richardcochran@...il.com" <richardcochran@...il.com>
Subject: RE: [EXTERNAL] Re: [net-next,v2 5/8] cn10k-ipsec: Add SA add/delete
support for outb inline ipsec
Please see inline
> -----Original Message-----
> From: Simon Horman <horms@...nel.org>
> Sent: Monday, May 13, 2024 10:22 PM
> To: Bharat Bhushan <bbhushan2@...vell.com>
> Cc: netdev@...r.kernel.org; linux-kernel@...r.kernel.org; Sunil Kovvuri
> Goutham <sgoutham@...vell.com>; Geethasowjanya Akula
> <gakula@...vell.com>; Subbaraya Sundeep Bhatta <sbhatta@...vell.com>;
> Hariprasad Kelam <hkelam@...vell.com>; davem@...emloft.net;
> edumazet@...gle.com; kuba@...nel.org; pabeni@...hat.com; Jerin Jacob
> <jerinj@...vell.com>; Linu Cherian <lcherian@...vell.com>;
> richardcochran@...il.com
> Subject: [EXTERNAL] Re: [net-next,v2 5/8] cn10k-ipsec: Add SA add/delete
> support for outb inline ipsec
>
>
> ----------------------------------------------------------------------
> On Mon, May 13, 2024 at 04:24:43PM +0530, Bharat Bhushan wrote:
> > This patch adds support to add and delete Security Association
> > (SA) xfrm ops. Hardware maintains SA context in memory allocated by
> > software. Each SA context is 128 byte aligned and size of each context
> > is multiple of 128-byte. Add support for transport and tunnel ipsec
> > mode, ESP protocol, aead aes-gcm-icv16, key size 128/192/256-bits with
> > 32bit salt.
> >
> > Signed-off-by: Bharat Bhushan <bbhushan2@...vell.com>
> > ---
> > v1->v2:
> > - Use dma_wmb() instead of architecture specific barrier
> >
> > .../marvell/octeontx2/nic/cn10k_ipsec.c | 433 +++++++++++++++++-
> > .../marvell/octeontx2/nic/cn10k_ipsec.h | 114 +++++
> > 2 files changed, 546 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.c
> > b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.c
>
> ...
>
> > @@ -356,6 +362,414 @@ static int cn10k_outb_cpt_clean(struct otx2_nic
> *pf)
> > return err;
> > }
> >
> > +static int cn10k_outb_get_sa_index(struct otx2_nic *pf,
> > + struct cn10k_tx_sa_s *sa_entry) {
> > + u32 sa_size = pf->ipsec.sa_size;
> > + u32 sa_index;
> > +
> > + if (!sa_entry || ((void *)sa_entry < pf->ipsec.outb_sa->base))
> > + return -EINVAL;
> > +
> > + sa_index = ((void *)sa_entry - pf->ipsec.outb_sa->base) / sa_size;
> > + if (sa_index >= CN10K_IPSEC_OUTB_MAX_SA)
> > + return -EINVAL;
> > +
> > + return sa_index;
> > +}
> > +
> > +static dma_addr_t cn10k_outb_get_sa_iova(struct otx2_nic *pf,
> > + struct cn10k_tx_sa_s *sa_entry) {
> > + u32 sa_index = cn10k_outb_get_sa_index(pf, sa_entry);
> > +
> > + if (sa_index < 0)
> > + return 0;
>
> Should the type of sa_index be int?
> That would match the return type of cn10k_outb_get_sa_index.
>
> Otherwise, testing for < 0 will always be false.
>
> Likewise in cn10k_outb_free_sa and cn10k_ipsec_del_state.
>
> Flagged by Smatch.
Fill fix all error reported by smatch in next version.
>
> > + return pf->ipsec.outb_sa->iova + sa_index * pf->ipsec.sa_size; }
>
> ...
>
> > +static int cn10k_outb_write_sa(struct otx2_nic *pf, struct
> > +cn10k_tx_sa_s *sa_cptr) {
> > + dma_addr_t res_iova, dptr_iova, sa_iova;
> > + struct cn10k_tx_sa_s *sa_dptr;
> > + struct cpt_inst_s inst;
> > + struct cpt_res_s *res;
> > + u32 sa_size, off;
> > + u64 reg_val;
> > + int ret;
> > +
> > + sa_iova = cn10k_outb_get_sa_iova(pf, sa_cptr);
> > + if (!sa_iova)
> > + return -EINVAL;
> > +
> > + res = dma_alloc_coherent(pf->dev, sizeof(struct cpt_res_s),
> > + &res_iova, GFP_ATOMIC);
> > + if (!res)
> > + return -ENOMEM;
> > +
> > + sa_size = sizeof(struct cn10k_tx_sa_s);
> > + sa_dptr = dma_alloc_coherent(pf->dev, sa_size, &dptr_iova,
> GFP_ATOMIC);
> > + if (!sa_dptr) {
> > + dma_free_coherent(pf->dev, sizeof(struct cpt_res_s), res,
> > + res_iova);
> > + return -ENOMEM;
> > + }
> > +
> > + for (off = 0; off < (sa_size / 8); off++)
> > + *((u64 *)sa_dptr + off) = cpu_to_be64(*((u64 *)sa_cptr +
> off));
>
> Given the layout of struct cn10k_tx_sa_s, it's not clear to me how it makes
> sense for it to be used to store big endian quadwords.
> Which is a something that probably ought to be addressed.
>
> But if not, Sparse complains about the endienness of the types used above. I
> think it wants:
>
> *((__be64 *)sa_dptr + off)
Will fix this and other errors reported by Sparse.
>
> > +
> > + memset(&inst, 0, sizeof(struct cpt_inst_s));
> > +
> > + res->compcode = CN10K_CPT_COMP_E_NOTDONE;
> > + inst.res_addr = res_iova;
> > + inst.dptr = (u64)dptr_iova;
> > + inst.param2 = sa_size >> 3;
> > + inst.dlen = sa_size;
> > + inst.opcode_major = CN10K_IPSEC_MAJOR_OP_WRITE_SA;
> > + inst.opcode_minor = CN10K_IPSEC_MINOR_OP_WRITE_SA;
> > + inst.cptr = sa_iova;
> > + inst.ctx_val = 1;
> > + inst.egrp = CN10K_DEF_CPT_IPSEC_EGRP;
> > +
> > + cn10k_cpt_inst_flush(pf, &inst, sizeof(struct cpt_inst_s));
> > + dma_wmb();
> > + ret = cn10k_wait_for_cpt_respose(pf, res);
> > + if (ret)
> > + goto out;
> > +
> > + /* Trigger CTX flush to write dirty data back to DRAM */
> > + reg_val = FIELD_PREP(CPT_LF_CTX_FLUSH, sa_iova >> 7);
> > + otx2_write64(pf, CN10K_CPT_LF_CTX_FLUSH, reg_val);
> > +
> > +out:
> > + dma_free_coherent(pf->dev, sa_size, sa_dptr, dptr_iova);
> > + dma_free_coherent(pf->dev, sizeof(struct cpt_res_s), res, res_iova);
> > + return ret;
> > +}
>
> ...
>
> > +static void cn10k_outb_prepare_sa(struct xfrm_state *x,
> > + struct cn10k_tx_sa_s *sa_entry) {
> > + int key_len = (x->aead->alg_key_len + 7) / 8;
> > + struct net_device *netdev = x->xso.dev;
> > + u8 *key = x->aead->alg_key;
> > + struct otx2_nic *pf;
> > + u32 *tmp_salt;
> > + u64 *tmp_key;
> > + int idx;
> > +
> > + memset(sa_entry, 0, sizeof(struct cn10k_tx_sa_s));
> > +
> > + /* context size, 128 Byte aligned up */
> > + pf = netdev_priv(netdev);
> > + sa_entry->ctx_size = (pf->ipsec.sa_size / OTX2_ALIGN) & 0xF;
> > + sa_entry->hw_ctx_off = cn10k_ipsec_get_hw_ctx_offset();
> > + sa_entry->ctx_push_size = cn10k_ipsec_get_ctx_push_size();
> > +
> > + /* Ucode to skip two words of CPT_CTX_HW_S */
> > + sa_entry->ctx_hdr_size = 1;
> > +
> > + /* Allow Atomic operation (AOP) */
> > + sa_entry->aop_valid = 1;
> > +
> > + /* Outbound, ESP TRANSPORT/TUNNEL Mode, AES-GCM with AES
> key length
> > + * 128bit.
> > + */
> > + sa_entry->sa_dir = CN10K_IPSEC_SA_DIR_OUTB;
> > + sa_entry->ipsec_protocol = CN10K_IPSEC_SA_IPSEC_PROTO_ESP;
> > + sa_entry->enc_type = CN10K_IPSEC_SA_ENCAP_TYPE_AES_GCM;
> > + if (x->props.mode == XFRM_MODE_TUNNEL)
> > + sa_entry->ipsec_mode =
> CN10K_IPSEC_SA_IPSEC_MODE_TUNNEL;
> > + else
> > + sa_entry->ipsec_mode =
> CN10K_IPSEC_SA_IPSEC_MODE_TRANSPORT;
> > +
> > + sa_entry->spi = cpu_to_be32(x->id.spi);
>
> The type of spi is a 32-bit bitfield of a 64-bit unsigned host endien integer.
>
> 1. I suspect it would make more sense to declare that field as a 32bit integer.
> 2. It is being assigned a big endian value. That doesn't seem right.
>
> The second issue was flagged by Sparse.
>
> > +
> > + /* Last 4 bytes are salt */
> > + key_len -= 4;
> > + sa_entry->aes_key_len = cn10k_ipsec_get_aes_key_len(key_len);
> > + memcpy(sa_entry->cipher_key, key, key_len);
> > + tmp_key = (u64 *)sa_entry->cipher_key;
> > +
> > + for (idx = 0; idx < key_len / 8; idx++)
> > + tmp_key[idx] = be64_to_cpu(tmp_key[idx]);
>
> More endian problems flagged by Sparse on this line.
> An integer variable should typically be used to store a big endian value, a little
> endian value, or a host endian value.
> Not more than one of these.
>
> This is because tooling such as Sparse can then be used to verify the
> correctness of the endian used.
Will fix in next version
>
> > +
> > + memcpy(&sa_entry->iv_gcm_salt, key + key_len, 4);
> > + tmp_salt = (u32 *)&sa_entry->iv_gcm_salt;
> > + *tmp_salt = be32_to_cpu(*tmp_salt);
>
> Likewise here.
>
> > +
> > + /* Write SA context data to memory before enabling */
> > + wmb();
> > +
> > + /* Enable SA */
> > + sa_entry->sa_valid = 1;
> > +}
>
> ...
>
> > +static int cn10k_ipsec_add_state(struct xfrm_state *x,
> > + struct netlink_ext_ack *extack)
> > +{
> > + struct net_device *netdev = x->xso.dev;
> > + struct cn10k_tx_sa_s *sa_entry;
> > + struct cpt_ctx_info_s *sa_info;
> > + struct otx2_nic *pf;
> > + int err;
> > +
> > + err = cn10k_ipsec_validate_state(x);
> > + if (err)
> > + return err;
> > +
> > + if (x->xso.dir == XFRM_DEV_OFFLOAD_IN) {
> > + netdev_err(netdev, "xfrm inbound offload not supported\n");
> > + err = -ENODEV;
>
> This path results in pf being dereferenced while uninitialised towards the
> bottom of this function.
>
> Flagged by Smatch, and Clang-18 W=1 build
>
> > + } else {
> > + pf = netdev_priv(netdev);
> > + if (!mutex_trylock(&pf->ipsec.lock)) {
> > + netdev_err(netdev, "IPSEC device is busy\n");
> > + return -EBUSY;
> > + }
> > +
> > + if (!(pf->flags & OTX2_FLAG_INLINE_IPSEC_ENABLED)) {
> > + netdev_err(netdev, "IPSEC not enabled/supported on
> device\n");
> > + err = -ENODEV;
> > + goto unlock;
> > + }
> > +
> > + sa_entry = cn10k_outb_alloc_sa(pf);
> > + if (!sa_entry) {
> > + netdev_err(netdev, "SA maximum limit %x
> reached\n",
> > + CN10K_IPSEC_OUTB_MAX_SA);
> > + err = -EBUSY;
> > + goto unlock;
> > + }
> > +
> > + cn10k_outb_prepare_sa(x, sa_entry);
> > +
> > + err = cn10k_outb_write_sa(pf, sa_entry);
> > + if (err) {
> > + netdev_err(netdev, "Error writing outbound SA\n");
> > + cn10k_outb_free_sa(pf, sa_entry);
> > + goto unlock;
> > + }
> > +
> > + sa_info = kmalloc(sizeof(*sa_info), GFP_KERNEL);
> > + sa_info->sa_entry = sa_entry;
> > + sa_info->sa_iova = cn10k_outb_get_sa_iova(pf, sa_entry);
> > + x->xso.offload_handle = (unsigned long)sa_info;
> > + }
> > +
> > +unlock:
> > + mutex_unlock(&pf->ipsec.lock);
> > + return err;
> > +}
>
> ...
>
> > +static const struct xfrmdev_ops cn10k_ipsec_xfrmdev_ops = {
> > + .xdo_dev_state_add = cn10k_ipsec_add_state,
> > + .xdo_dev_state_delete = cn10k_ipsec_del_state,
> > +};
> > +
>
> cn10k_ipsec_xfrmdev_ops is unused.
> Perhaps it, along with it's callbacks,
> should be added by the function that uses it?
I wanted to enable ipsec offload in last patch of the series
("[net-next,v2 8/8] cn10k-ipsec: Enable outbound inline ipsec offload")
Is it okay to set xfrmdev_ops in this patch without setting NETIF_F_HW_ESP (below two lines of last patch)
+ /* Set xfrm device ops */
+ netdev->xfrmdev_ops = &cn10k_ipsec_xfrmdev_ops;
Last patch will set below flags.
+ netdev->hw_features |= NETIF_F_HW_ESP;
+ netdev->hw_enc_features |= NETIF_F_HW_ESP;
+
Thanks
-Bharat
>
> Flagged by W=1 builds.
>
> > int cn10k_ipsec_ethtool_init(struct net_device *netdev, bool enable)
> > {
> > struct otx2_nic *pf = netdev_priv(netdev);
>
> ...
>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.h
> > b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.h
>
> ...
>
> > +struct cn10k_tx_sa_s {
> > + u64 esn_en : 1; /* W0 */
> > + u64 rsvd_w0_1_8 : 8;
> > + u64 hw_ctx_off : 7;
> > + u64 ctx_id : 16;
> > + u64 rsvd_w0_32_47 : 16;
> > + u64 ctx_push_size : 7;
> > + u64 rsvd_w0_55 : 1;
> > + u64 ctx_hdr_size : 2;
> > + u64 aop_valid : 1;
> > + u64 rsvd_w0_59 : 1;
> > + u64 ctx_size : 4;
> > + u64 w1; /* W1 */
> > + u64 sa_valid : 1; /* W2 */
> > + u64 sa_dir : 1;
> > + u64 rsvd_w2_2_3 : 2;
> > + u64 ipsec_mode : 1;
> > + u64 ipsec_protocol : 1;
> > + u64 aes_key_len : 2;
> > + u64 enc_type : 3;
> > + u64 rsvd_w2_11_31 : 21;
> > + u64 spi : 32;
> > + u64 w3; /* W3 */
> > + u8 cipher_key[32]; /* W4 - W7 */
> > + u32 rsvd_w8_0_31; /* W8 : IV */
> > + u32 iv_gcm_salt;
> > + u64 rsvd_w9_w30[22]; /* W9 - W30 */
> > + u64 hw_ctx[6]; /* W31 - W36 */
> > +};
>
> ...
Powered by blists - more mailing lists