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

[Bharat Bhushan]

> -----Original Message-----
> From: Leon Romanovsky <leon@xxxxxxxxxx>
> Sent: Thursday, May 30, 2024 8:20 PM
> To: Bharat Bhushan <bbhushan2@xxxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Sunil Kovvuri
> Goutham <sgoutham@xxxxxxxxxxx>; Geethasowjanya Akula
> <gakula@xxxxxxxxxxx>; Subbaraya Sundeep Bhatta <sbhatta@xxxxxxxxxxx>;
> Hariprasad Kelam <hkelam@xxxxxxxxxxx>; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; Jerin Jacob
> <jerinj@xxxxxxxxxxx>; Linu Cherian <lcherian@xxxxxxxxxxx>;
> richardcochran@xxxxxxxxx
> Subject: [EXTERNAL] Re: [net-next,v3 5/8] cn10k-ipsec: Add SA add/delete
> support for outb inline ipsec
> 
> ----------------------------------------------------------------------
> On Tue, May 28, 2024 at 07:23:46PM +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@xxxxxxxxxxx>
> > ---
> > 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       | 452 ++++++++++++++++++
> >  .../marvell/octeontx2/nic/cn10k_ipsec.h       | 114 +++++
> >  2 files changed, 566 insertions(+)
> 
> <...>
> 
> > +static int cn10k_ipsec_validate_state(struct xfrm_state *x) {
> > +	struct net_device *netdev = x->xso.dev;
> > +
> > +	if (x->props.aalgo != SADB_AALG_NONE) {
> > +		netdev_err(netdev, "Cannot offload authenticated xfrm
> states\n");
> > +		return -EINVAL;
> > +	}
> > +	if (x->props.ealgo != SADB_X_EALG_AES_GCM_ICV16) {
> > +		netdev_err(netdev, "Only AES-GCM-ICV16 xfrm state may be
> offloaded\n");
> > +		return -EINVAL;
> > +	}
> > +	if (x->props.calgo != SADB_X_CALG_NONE) {
> > +		netdev_err(netdev, "Cannot offload compressed xfrm
> states\n");
> > +		return -EINVAL;
> > +	}
> > +	if (x->props.flags & XFRM_STATE_ESN) {
> > +		netdev_err(netdev, "Cannot offload ESN xfrm states\n");
> > +		return -EINVAL;
> > +	}
> 
> I afraid that this check will cause for this offload to be unusable in real life
> scenarios. It is hard to imagine that someone will use offload which requires
> rekeying every 2^32 packets.

I agree, Currently ESN offload is not enabled. Enabling END is in our list. 

> 
> > +	if (x->props.family != AF_INET && x->props.family != AF_INET6) {
> > +		netdev_err(netdev, "Only IPv4/v6 xfrm states may be
> offloaded\n");
> > +		return -EINVAL;
> > +	}
> > +	if (x->props.mode != XFRM_MODE_TRANSPORT &&
> > +	    x->props.mode != XFRM_MODE_TUNNEL) {
> > +		dev_info(&netdev->dev, "Only tunnel/transport xfrm states
> may be offloaded\n");
> > +		return -EINVAL;
> > +	}
> > +	if (x->id.proto != IPPROTO_ESP) {
> > +		netdev_err(netdev, "Only ESP xfrm state may be
> offloaded\n");
> > +		return -EINVAL;
> > +	}
> > +	if (x->encap) {
> > +		netdev_err(netdev, "Encapsulated xfrm state may not be
> offloaded\n");
> > +		return -EINVAL;
> > +	}
> > +	if (!x->aead) {
> > +		netdev_err(netdev, "Cannot offload xfrm states without
> aead\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (x->aead->alg_icv_len != 128) {
> > +		netdev_err(netdev, "Cannot offload xfrm states with AEAD
> ICV length other than 128bit\n");
> > +		return -EINVAL;
> > +	}
> > +	if (x->aead->alg_key_len != 128 + 32 &&
> > +	    x->aead->alg_key_len != 192 + 32 &&
> > +	    x->aead->alg_key_len != 256 + 32) {
> > +		netdev_err(netdev, "Cannot offload xfrm states with AEAD
> key length other than 128/192/256bit\n");
> > +		return -EINVAL;
> > +	}
> > +	if (x->tfcpad) {
> > +		netdev_err(netdev, "Cannot offload xfrm states with tfc
> padding\n");
> > +		return -EINVAL;
> > +	}
> > +	if (!x->geniv) {
> > +		netdev_err(netdev, "Cannot offload xfrm states without
> geniv\n");
> > +		return -EINVAL;
> > +	}
> > +	if (strcmp(x->geniv, "seqiv")) {
> > +		netdev_err(netdev, "Cannot offload xfrm states with geniv
> other than seqiv\n");
> > +		return -EINVAL;
> > +	}
> > +	return 0;
> > +}
> 
> I don't see check for supported offload type among these checks.
> if (x->xso.type != XFRM_DEV_OFFLOAD_CRYPTO) {  ....

Will add the check.

Thanks
-Bharat