netdev - RE: [PATCH net-next v3 3/3] nfp: implement xfrm callbacks and expose ipsec offload feature to upper layer

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 7 Nov 2022 09:46:46 +0000
From:   Yinjun Zhang <yinjun.zhang@...igine.com>
To:     Leon Romanovsky <leon@...nel.org>
CC:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Chengtian Liu <chengtian.liu@...igine.com>,
        HuanHuan Wang <huanhuan.wang@...igine.com>,
        Louis Peens <louis.peens@...igine.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        oss-drivers <oss-drivers@...igine.com>,
        Simon Horman <simon.horman@...igine.com>
Subject: RE: [PATCH net-next v3 3/3] nfp: implement xfrm callbacks and expose
 ipsec offload feature to upper layer

On Mon, 7 Nov 2022 08:14:12 +0200, Leon Romanovsky wrote:
 <...>
> > +	struct sa_ctrl_word {
> > +		uint32_t hash   :4;	  /* From nfp_ipsec_sa_hash_type */
> > +		uint32_t cimode :4;	  /* From nfp_ipsec_sa_cipher_mode */
> > +		uint32_t cipher :4;	  /* From nfp_ipsec_sa_cipher */
> > +		uint32_t mode   :2;	  /* From nfp_ipsec_sa_mode */
> > +		uint32_t proto  :2;	  /* From nfp_ipsec_sa_prot */
> > +		uint32_t dir :1;	  /* SA direction */
> > +		uint32_t ena_arw:1;	  /* Anti-Replay Window */
> > +		uint32_t ext_seq:1;	  /* 64-bit Sequence Num */
> > +		uint32_t ext_arw:1;	  /* 64b Anti-Replay Window */
> > +		uint32_t spare2 :9;	  /* Must be set to 0 */
> > +		uint32_t encap_dsbl:1;	  /* Encap/Decap disable */
> > +		uint32_t gen_seq:1;	  /* Firmware Generate Seq */
> > +		uint32_t spare8 :1;	  /* Must be set to 0 */
> > +	} ctrl_word;
> > +	u32 spi;			  /* SPI Value */
> > +	uint32_t pmtu_limit :16;	  /* PMTU Limit */
> > +	uint32_t spare3     :1;
> > +	uint32_t frag_check :1;		  /* Stateful fragment checking flag */
> > +	uint32_t bypass_DSCP:1;		  /* Bypass DSCP Flag */
> > +	uint32_t df_ctrl    :2;		  /* DF Control bits */
> > +	uint32_t ipv6       :1;		  /* Outbound IPv6 addr format */
> > +	uint32_t udp_enable :1;		  /* Add/Remove UDP header for NAT */
> > +	uint32_t tfc_enable :1;		  /* Traffic Flow Confidentiality */
> > +	uint32_t spare4	 :8;
> > +	u32 soft_lifetime_byte_count;
> > +	u32 hard_lifetime_byte_count;
> 
> These fields are not relevant for IPsec crypto offload. I would be more
> than happy to see only used fields here.

They are not used currently in kernel indeed. However the HW is not designed for 
crypto-offloading only, not for kernel only, some extensive features are supported. 
So they're reserved here.

<...>
> > +
> > +	/* General */
> > +	switch (x->props.mode) {
> > +	case XFRM_MODE_TUNNEL:
> > +		cfg->ctrl_word.mode = NFP_IPSEC_PROTMODE_TUNNEL;
> > +		break;
> > +	case XFRM_MODE_TRANSPORT:
> > +		cfg->ctrl_word.mode = NFP_IPSEC_PROTMODE_TRANSPORT;
> > +		break;
> 
> Why is it important for IPsec crypto? The HW logic must be the same for
> all modes. There are no differences between transport and tunnel.

As I mentioned above, it's differentiated in HW to support more features.

> 
> > +	default:
> > +		nn_err(nn, "Unsupported mode for xfrm offload\n");
> 
> There are no other modes.
> 

<...>
> 
> > +	err = xa_alloc(&nn->xa_ipsec, &saidx, x,
> > +		       XA_LIMIT(0, NFP_NET_IPSEC_MAX_SA_CNT - 1),
> GFP_KERNEL);
> 
> Create XArray with XA_FLAGS_ALLOC1, it will cause to xarray skip 0.
> See DEFINE_XARRAY_ALLOC1() for more info.

Actually 0 is a valid SA id in HW/driver, we don't want to skip 0.

> 
> 
> > +	if (err < 0) {
> > +		nn_err(nn, "Unable to get sa_data number for IPsec\n");
> > +		return err;
> > +	}
> > +
> > +	/* Allocate saidx and commit the SA */
> > +	err = nfp_ipsec_cfg_cmd_issue(nn, NFP_IPSEC_CFG_MSSG_ADD_SA,
> saidx, &msg);
> > +	if (err) {
> > +		xa_erase(&nn->xa_ipsec, saidx);
> > +		nn_err(nn, "Failed to issue IPsec command err ret=%d\n",
> err);
> > +		return err;
> > +	}
> > +
> > +	/* 0 is invalid offload_handle for kernel */
> > +	x->xso.offload_handle = saidx + 1;
> 
> If you create XArray as I said above, you won't need to add +1.
>