diff mbox series

[net-next,v1,06/10] net/mlx5e: Support IPsec TX packet offload in tunnel mode

Message ID bad0c22f37a3591aa1abed4d8a8e677b92e034f5.1681388425.git.leonro@nvidia.com (mailing list archive)
State Accepted
Commit efbd31c4d844e55526587cc3820ca8a11d7db733
Delegated to: Netdev Maintainers
Headers show
Series Support tunnel mode in mlx5 IPsec packet offload | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 18 this patch: 18
netdev/cc_maintainers warning 2 maintainers not CCed: linux-rdma@vger.kernel.org borisp@nvidia.com
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 94 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Leon Romanovsky April 13, 2023, 12:29 p.m. UTC 
From: Leon Romanovsky <leonro@nvidia.com>

Extend mlx5 driver with logic to support IPsec TX packet offload
in tunnel mode.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 .../mellanox/mlx5/core/en_accel/ipsec.c       | 12 +++++
 .../mellanox/mlx5/core/en_accel/ipsec_fs.c    | 52 +++++++++++++++++++
 2 files changed, 64 insertions(+)

Comments

Simon Horman April 17, 2023, 1:23 p.m. UTC | #1 
On Thu, Apr 13, 2023 at 03:29:24PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Extend mlx5 driver with logic to support IPsec TX packet offload
> in tunnel mode.
> 
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>

Nit below not withstanding,

Reviewed-by: Simon Horman <simon.horman@corigine.com>

...

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> index 7c55b37c1c01..36f3ffd54355 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> @@ -271,6 +271,18 @@ static void mlx5e_ipsec_init_macs(struct mlx5e_ipsec_sa_entry *sa_entry,
>  		neigh_ha_snapshot(addr, n, netdev);
>  		ether_addr_copy(attrs->smac, addr);
>  		break;
> +	case XFRM_DEV_OFFLOAD_OUT:
> +		ether_addr_copy(attrs->smac, addr);
> +		n = neigh_lookup(&arp_tbl, &attrs->daddr.a4, netdev);
> +		if (!n) {
> +			n = neigh_create(&arp_tbl, &attrs->daddr.a4, netdev);
> +			if (IS_ERR(n))
> +				return;
> +			neigh_event_send(n, NULL);
> +		}
> +		neigh_ha_snapshot(addr, n, netdev);
> +		ether_addr_copy(attrs->dmac, addr);
> +		break;

I see no problem with the above code.
However, it does seem very similar to the code for the previous case,
XFRM_DEV_OFFLOAD_IN. Perhaps this could be refactored somehow.

I'm not suggesting this warrants a respin; a follow-up would be fine IMHO.
I could be wrong entirely :)

>  	default:
>  		return;
>  	}

...
Leon Romanovsky April 18, 2023, 6:48 a.m. UTC | #2 
On Mon, Apr 17, 2023 at 03:23:55PM +0200, Simon Horman wrote:
> On Thu, Apr 13, 2023 at 03:29:24PM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > Extend mlx5 driver with logic to support IPsec TX packet offload
> > in tunnel mode.
> > 
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>

<...>

> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> > @@ -271,6 +271,18 @@ static void mlx5e_ipsec_init_macs(struct mlx5e_ipsec_sa_entry *sa_entry,
> >  		neigh_ha_snapshot(addr, n, netdev);
> >  		ether_addr_copy(attrs->smac, addr);
> >  		break;
> > +	case XFRM_DEV_OFFLOAD_OUT:
> > +		ether_addr_copy(attrs->smac, addr);
> > +		n = neigh_lookup(&arp_tbl, &attrs->daddr.a4, netdev);
> > +		if (!n) {
> > +			n = neigh_create(&arp_tbl, &attrs->daddr.a4, netdev);
> > +			if (IS_ERR(n))
> > +				return;
> > +			neigh_event_send(n, NULL);
> > +		}
> > +		neigh_ha_snapshot(addr, n, netdev);
> > +		ether_addr_copy(attrs->dmac, addr);
> > +		break;
> 
> I see no problem with the above code.
> However, it does seem very similar to the code for the previous case,
> XFRM_DEV_OFFLOAD_IN. Perhaps this could be refactored somehow.

Yes, it can be refactored to something like this:

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
index 59b9927ac90f..55b38544422f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
@@ -252,6 +252,8 @@ static void mlx5e_ipsec_init_macs(struct mlx5e_ipsec_sa_entry *sa_entry,
 	struct net_device *netdev;
 	struct neighbour *n;
 	u8 addr[ETH_ALEN];
+	const void *pkey;
+	u8 *dst, *src;
 
 	if (attrs->mode != XFRM_MODE_TUNNEL ||
 	    attrs->type != XFRM_DEV_OFFLOAD_PACKET)
@@ -262,36 +264,31 @@ static void mlx5e_ipsec_init_macs(struct mlx5e_ipsec_sa_entry *sa_entry,
 	mlx5_query_mac_address(mdev, addr);
 	switch (attrs->dir) {
 	case XFRM_DEV_OFFLOAD_IN:
-		ether_addr_copy(attrs->dmac, addr);
-		n = neigh_lookup(&arp_tbl, &attrs->saddr.a4, netdev);
-		if (!n) {
-			n = neigh_create(&arp_tbl, &attrs->saddr.a4, netdev);
-			if (IS_ERR(n))
-				return;
-			neigh_event_send(n, NULL);
-			attrs->drop = true;
-			break;
-		}
-		neigh_ha_snapshot(addr, n, netdev);
-		ether_addr_copy(attrs->smac, addr);
+		src = attrs->dmac;
+		dst = attrs->smac;
+		pkey = &attrs->saddr.a4;
 		break;
 	case XFRM_DEV_OFFLOAD_OUT:
-		ether_addr_copy(attrs->smac, addr);
-		n = neigh_lookup(&arp_tbl, &attrs->daddr.a4, netdev);
-		if (!n) {
-			n = neigh_create(&arp_tbl, &attrs->daddr.a4, netdev);
-			if (IS_ERR(n))
-				return;
-			neigh_event_send(n, NULL);
-			attrs->drop = true;
-			break;
-		}
-		neigh_ha_snapshot(addr, n, netdev);
-		ether_addr_copy(attrs->dmac, addr);
+		src = attrs->smac;
+		dst = attrs->dmac;
+		pkey = &attrs->daddr.a4;
 		break;
 	default:
 		return;
 	}
+
+	ether_addr_copy(src, addr);
+	n = neigh_lookup(&arp_tbl, pkey, netdev);
+	if (!n) {
+		n = neigh_create(&arp_tbl, pkey, netdev);
+		if (IS_ERR(n))
+			return;
+		neigh_event_send(n, NULL);
+		attrs->drop = true;
+	} else {
+		neigh_ha_snapshot(addr, n, netdev);
+		ether_addr_copy(dst, addr);
+	}
 	neigh_release(n);
 }
 

Thanks
Simon Horman April 18, 2023, 7:09 a.m. UTC | #3 
On Tue, Apr 18, 2023 at 09:48:27AM +0300, Leon Romanovsky wrote:
> On Mon, Apr 17, 2023 at 03:23:55PM +0200, Simon Horman wrote:
> > On Thu, Apr 13, 2023 at 03:29:24PM +0300, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@nvidia.com>
> > > 
> > > Extend mlx5 driver with logic to support IPsec TX packet offload
> > > in tunnel mode.
> > > 
> > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> 
> <...>
> 
> > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> > > @@ -271,6 +271,18 @@ static void mlx5e_ipsec_init_macs(struct mlx5e_ipsec_sa_entry *sa_entry,
> > >  		neigh_ha_snapshot(addr, n, netdev);
> > >  		ether_addr_copy(attrs->smac, addr);
> > >  		break;
> > > +	case XFRM_DEV_OFFLOAD_OUT:
> > > +		ether_addr_copy(attrs->smac, addr);
> > > +		n = neigh_lookup(&arp_tbl, &attrs->daddr.a4, netdev);
> > > +		if (!n) {
> > > +			n = neigh_create(&arp_tbl, &attrs->daddr.a4, netdev);
> > > +			if (IS_ERR(n))
> > > +				return;
> > > +			neigh_event_send(n, NULL);
> > > +		}
> > > +		neigh_ha_snapshot(addr, n, netdev);
> > > +		ether_addr_copy(attrs->dmac, addr);
> > > +		break;
> > 
> > I see no problem with the above code.
> > However, it does seem very similar to the code for the previous case,
> > XFRM_DEV_OFFLOAD_IN. Perhaps this could be refactored somehow.
> 
> Yes, it can be refactored to something like this:

Thanks Leon,

this looks good to me.

> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> index 59b9927ac90f..55b38544422f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> @@ -252,6 +252,8 @@ static void mlx5e_ipsec_init_macs(struct mlx5e_ipsec_sa_entry *sa_entry,
>  	struct net_device *netdev;
>  	struct neighbour *n;
>  	u8 addr[ETH_ALEN];
> +	const void *pkey;
> +	u8 *dst, *src;
>  
>  	if (attrs->mode != XFRM_MODE_TUNNEL ||
>  	    attrs->type != XFRM_DEV_OFFLOAD_PACKET)
> @@ -262,36 +264,31 @@ static void mlx5e_ipsec_init_macs(struct mlx5e_ipsec_sa_entry *sa_entry,
>  	mlx5_query_mac_address(mdev, addr);
>  	switch (attrs->dir) {
>  	case XFRM_DEV_OFFLOAD_IN:
> -		ether_addr_copy(attrs->dmac, addr);
> -		n = neigh_lookup(&arp_tbl, &attrs->saddr.a4, netdev);
> -		if (!n) {
> -			n = neigh_create(&arp_tbl, &attrs->saddr.a4, netdev);
> -			if (IS_ERR(n))
> -				return;
> -			neigh_event_send(n, NULL);
> -			attrs->drop = true;
> -			break;
> -		}
> -		neigh_ha_snapshot(addr, n, netdev);
> -		ether_addr_copy(attrs->smac, addr);
> +		src = attrs->dmac;
> +		dst = attrs->smac;
> +		pkey = &attrs->saddr.a4;
>  		break;
>  	case XFRM_DEV_OFFLOAD_OUT:
> -		ether_addr_copy(attrs->smac, addr);
> -		n = neigh_lookup(&arp_tbl, &attrs->daddr.a4, netdev);
> -		if (!n) {
> -			n = neigh_create(&arp_tbl, &attrs->daddr.a4, netdev);
> -			if (IS_ERR(n))
> -				return;
> -			neigh_event_send(n, NULL);
> -			attrs->drop = true;
> -			break;
> -		}
> -		neigh_ha_snapshot(addr, n, netdev);
> -		ether_addr_copy(attrs->dmac, addr);
> +		src = attrs->smac;
> +		dst = attrs->dmac;
> +		pkey = &attrs->daddr.a4;
>  		break;
>  	default:
>  		return;
>  	}
> +
> +	ether_addr_copy(src, addr);
> +	n = neigh_lookup(&arp_tbl, pkey, netdev);
> +	if (!n) {
> +		n = neigh_create(&arp_tbl, pkey, netdev);
> +		if (IS_ERR(n))
> +			return;
> +		neigh_event_send(n, NULL);
> +		attrs->drop = true;
> +	} else {
> +		neigh_ha_snapshot(addr, n, netdev);
> +		ether_addr_copy(dst, addr);
> +	}
>  	neigh_release(n);
>  }
>  
> 
> Thanks
Leon Romanovsky April 18, 2023, 7:58 a.m. UTC | #4 
On Tue, Apr 18, 2023 at 09:09:13AM +0200, Simon Horman wrote:
> On Tue, Apr 18, 2023 at 09:48:27AM +0300, Leon Romanovsky wrote:
> > On Mon, Apr 17, 2023 at 03:23:55PM +0200, Simon Horman wrote:
> > > On Thu, Apr 13, 2023 at 03:29:24PM +0300, Leon Romanovsky wrote:
> > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > 
> > > > Extend mlx5 driver with logic to support IPsec TX packet offload
> > > > in tunnel mode.
> > > > 
> > > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > 
> > <...>
> > 
> > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> > > > @@ -271,6 +271,18 @@ static void mlx5e_ipsec_init_macs(struct mlx5e_ipsec_sa_entry *sa_entry,
> > > >  		neigh_ha_snapshot(addr, n, netdev);
> > > >  		ether_addr_copy(attrs->smac, addr);
> > > >  		break;
> > > > +	case XFRM_DEV_OFFLOAD_OUT:
> > > > +		ether_addr_copy(attrs->smac, addr);
> > > > +		n = neigh_lookup(&arp_tbl, &attrs->daddr.a4, netdev);
> > > > +		if (!n) {
> > > > +			n = neigh_create(&arp_tbl, &attrs->daddr.a4, netdev);
> > > > +			if (IS_ERR(n))
> > > > +				return;
> > > > +			neigh_event_send(n, NULL);
> > > > +		}
> > > > +		neigh_ha_snapshot(addr, n, netdev);
> > > > +		ether_addr_copy(attrs->dmac, addr);
> > > > +		break;
> > > 
> > > I see no problem with the above code.
> > > However, it does seem very similar to the code for the previous case,
> > > XFRM_DEV_OFFLOAD_IN. Perhaps this could be refactored somehow.
> > 
> > Yes, it can be refactored to something like this:
> 
> Thanks Leon,
> 
> this looks good to me.

Awesome, will prepare patch, test and send.

Thanks
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
index 7c55b37c1c01..36f3ffd54355 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
@@ -271,6 +271,18 @@  static void mlx5e_ipsec_init_macs(struct mlx5e_ipsec_sa_entry *sa_entry,
 		neigh_ha_snapshot(addr, n, netdev);
 		ether_addr_copy(attrs->smac, addr);
 		break;
+	case XFRM_DEV_OFFLOAD_OUT:
+		ether_addr_copy(attrs->smac, addr);
+		n = neigh_lookup(&arp_tbl, &attrs->daddr.a4, netdev);
+		if (!n) {
+			n = neigh_create(&arp_tbl, &attrs->daddr.a4, netdev);
+			if (IS_ERR(n))
+				return;
+			neigh_event_send(n, NULL);
+		}
+		neigh_ha_snapshot(addr, n, netdev);
+		ether_addr_copy(attrs->dmac, addr);
+		break;
 	default:
 		return;
 	}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c
index 001d7c3add6a..4c800b54d8b6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c
@@ -11,6 +11,7 @@ 
 
 #define NUM_IPSEC_FTE BIT(15)
 #define MLX5_REFORMAT_TYPE_ADD_ESP_TRANSPORT_SIZE 16
+#define IPSEC_TUNNEL_DEFAULT_TTL 0x40
 
 struct mlx5e_ipsec_fc {
 	struct mlx5_fc *cnt;
@@ -842,12 +843,31 @@  setup_pkt_tunnel_reformat(struct mlx5_core_dev *mdev,
 			  struct mlx5_accel_esp_xfrm_attrs *attrs,
 			  struct mlx5_pkt_reformat_params *reformat_params)
 {
+	struct ip_esp_hdr *esp_hdr;
+	struct ipv6hdr *ipv6hdr;
 	struct ethhdr *eth_hdr;
+	struct iphdr *iphdr;
 	char *reformatbf;
 	size_t bfflen;
+	void *hdr;
 
 	bfflen = sizeof(*eth_hdr);
 
+	if (attrs->dir == XFRM_DEV_OFFLOAD_OUT) {
+		bfflen += sizeof(*esp_hdr) + 8;
+
+		switch (attrs->family) {
+		case AF_INET:
+			bfflen += sizeof(*iphdr);
+			break;
+		case AF_INET6:
+			bfflen += sizeof(*ipv6hdr);
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
 	reformatbf = kzalloc(bfflen, GFP_KERNEL);
 	if (!reformatbf)
 		return -ENOMEM;
@@ -871,6 +891,38 @@  setup_pkt_tunnel_reformat(struct mlx5_core_dev *mdev,
 	case XFRM_DEV_OFFLOAD_IN:
 		reformat_params->type = MLX5_REFORMAT_TYPE_L3_ESP_TUNNEL_TO_L2;
 		break;
+	case XFRM_DEV_OFFLOAD_OUT:
+		reformat_params->type = MLX5_REFORMAT_TYPE_L2_TO_L3_ESP_TUNNEL;
+		reformat_params->param_0 = attrs->authsize;
+
+		hdr = reformatbf + sizeof(*eth_hdr);
+		switch (attrs->family) {
+		case AF_INET:
+			iphdr = (struct iphdr *)hdr;
+			memcpy(&iphdr->saddr, &attrs->saddr.a4, 4);
+			memcpy(&iphdr->daddr, &attrs->daddr.a4, 4);
+			iphdr->version = 4;
+			iphdr->ihl = 5;
+			iphdr->ttl = IPSEC_TUNNEL_DEFAULT_TTL;
+			iphdr->protocol = IPPROTO_ESP;
+			hdr += sizeof(*iphdr);
+			break;
+		case AF_INET6:
+			ipv6hdr = (struct ipv6hdr *)hdr;
+			memcpy(&ipv6hdr->saddr, &attrs->saddr.a6, 16);
+			memcpy(&ipv6hdr->daddr, &attrs->daddr.a6, 16);
+			ipv6hdr->nexthdr = IPPROTO_ESP;
+			ipv6hdr->version = 6;
+			ipv6hdr->hop_limit = IPSEC_TUNNEL_DEFAULT_TTL;
+			hdr += sizeof(*ipv6hdr);
+			break;
+		default:
+			goto free_reformatbf;
+		}
+
+		esp_hdr = (struct ip_esp_hdr *)hdr;
+		esp_hdr->spi = htonl(attrs->spi);
+		break;
 	default:
 		goto free_reformatbf;
 	}