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