netdev - Re: [net-next,v4 5/8] cn10k-ipsec: Add SA add/delete support for outb inline ipsec

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240613184051.GH4966@unreal>
Date: Thu, 13 Jun 2024 21:40:51 +0300
From: Leon Romanovsky <leon@...nel.org>
To: Bharat Bhushan <bbhushan2@...vell.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	sgoutham@...vell.com, gakula@...vell.com, sbhatta@...vell.com,
	hkelam@...vell.com, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, jerinj@...vell.com,
	lcherian@...vell.com, richardcochran@...il.com
Subject: Re: [net-next,v4 5/8] cn10k-ipsec: Add SA add/delete support for
 outb inline ipsec

On Wed, Jun 12, 2024 at 07:16:19PM +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>
> ---
> v3->v4:
>  - Added check for crypto offload (XFRM_DEV_OFFLOAD_CRYPTO)
>    Thanks "Leon Romanovsky" for pointing out
>  
> v2->v3:
>  - Removed memset to zero wherever possible
>   (comment from Kalesh Anakkur Purayil)
>  - Corrected error hanlding when setting SA for inbound
>    (comment from Kalesh Anakkur Purayil)
>  - Move "netdev->xfrmdev_ops = &cn10k_ipsec_xfrmdev_ops;" to this patch
>    This fix build error with W=1
> 
>  .../marvell/octeontx2/nic/cn10k_ipsec.c       | 456 ++++++++++++++++++
>  .../marvell/octeontx2/nic/cn10k_ipsec.h       | 114 +++++
>  2 files changed, 570 insertions(+)
> 
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.c
> index fc1029c17c00..892bdbde92ee 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.c
> @@ -336,6 +336,12 @@ static int cn10k_outb_cpt_clean(struct otx2_nic *pf)
>  	/* Set inline ipsec disabled for this device */
>  	pf->flags &= ~OTX2_FLAG_INLINE_IPSEC_ENABLED;
>  
> +	if (!bitmap_empty(pf->ipsec.sa_bitmap, CN10K_IPSEC_OUTB_MAX_SA)) {
> +		netdev_err(pf->netdev, "SA installed on this device\n");
> +		mutex_unlock(&pf->ipsec.lock);
> +		return -EBUSY;
> +	}

Sorry for not really reviewing the patches and posting some random
comments, but this addition makes me wonder if it is correct
design/implementation. At the stage of IPsec cleanup, all SAs should be
removed before this call.

BTW, In kernel, this type of IPsec is called "Crypto Offload" and not
"inline ipsec".

Thanks