[PATCH xfrm-next v7 0/8] Extend XFRM core to allow packet offload configuration

[PATCH xfrm-next v7 0/8] Extend XFRM core to allow packet offload configuration

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Changelog:
v7: As was discussed in IPsec workshop:
 * Renamed "full offload" to be "packet offload".
 * Added check that offloaded SA and policy have same device while sending packet
 * Added to SAD same optimization as was done for SPD to speed-up lookups.
v6: https://lore.kernel.org/all/cover.1666692948.git.leonro@nvidia.com
 * Fixed misplaced "!" in sixth patch.
v5: https://lore.kernel.org/all/cover.1666525321.git.leonro@nvidia.com
 * Rebased to latest ipsec-next.
 * Replaced HW priority patch with solution which mimics separated SPDs
   for SW and HW. See more description in this cover letter.
 * Dropped RFC tag, usecase, API and implementation are clear.
v4: https://lore.kernel.org/all/cover.1662295929.git.leonro@nvidia.com
 * Changed title from "PATCH" to "PATCH RFC" per-request.
 * Added two new patches: one to update hard/soft limits and another
   initial take on documentation.
 * Added more info about lifetime/rekeying flow to cover letter, see
   relevant section.
 * perf traces for crypto mode will come later.
v3: https://lore.kernel.org/all/cover.1661260787.git.leonro@nvidia.com
 * I didn't hear any suggestion what term to use instead of
   "packet offload", so left it as is. It is used in commit messages
   and documentation only and easy to rename.
 * Added performance data and background info to cover letter
 * Reused xfrm_output_resume() function to support multiple XFRM transformations
 * Add PMTU check in addition to driver .xdo_dev_offload_ok validation
 * Documentation is in progress, but not part of this series yet.
v2: https://lore.kernel.org/all/cover.1660639789.git.leonro@nvidia.com
 * Rebased to latest 6.0-rc1
 * Add an extra check in TX datapath patch to validate packets before
   forwarding to HW.
 * Added policy cleanup logic in case of netdev down event
v1: https://lore.kernel.org/all/cover.1652851393.git.leonro@nvidia.com
 * Moved comment to be before if (...) in third patch.
v0: https://lore.kernel.org/all/cover.1652176932.git.leonro@nvidia.com
-----------------------------------------------------------------------

The following series extends XFRM core code to handle a new type of IPsec
offload - packet offload.

In this mode, the HW is going to be responsible for the whole data path,
so both policy and state should be offloaded.

IPsec packet offload is an improved version of IPsec crypto mode,
In packet mode, HW is responsible to trim/add headers in addition to
decrypt/encrypt. In this mode, the packet arrives to the stack as already
decrypted and vice versa for TX (exits to HW as not-encrypted).

Devices that implement IPsec packet offload mode offload policies too.
In the RX path, it causes the situation that HW can't effectively
handle mixed SW and HW priorities unless users make sure that HW offloaded
policies have higher priorities.

It means that we don't need to perform any search of inexact policies
and/or priority checks if HW policy was discovered. In such situation,
the HW will catch the packets anyway and HW can still implement inexact
lookups.

In case specific policy is not found, we will continue with packet lookup
and check for existence of HW policies in inexact list.

HW policies are added to the head of SPD to ensure fast lookup, as XFRM
iterates over all policies in the loop.

This simple solution allows us to achieve same benefits of separate HW/SW
policies databases without over-engineering the code to iterate and manage
two databases at the same path.

To not over-engineer the code, HW policies are treated as SW ones and
don't take into account netdev to allow reuse of the same priorities for
policies databases without over-engineering the code to iterate and manage
two databases at the same path.

To not over-engineer the code, HW policies are treated as SW ones and
don't take into account netdev to allow reuse of the same priorities for
different devices.
 * No software fallback
 * Fragments are dropped, both in RX and TX
 * No sockets policies
 * Only IPsec transport mode is implemented

================================================================================
Rekeying:

In order to support rekeying, as XFRM core is skipped, the HW/driver should
do the following:
 * Count the handled packets
 * Raise event that limits are reached
 * Drop packets once hard limit is occurred.

The XFRM core calls to newly introduced xfrm_dev_state_update_curlft()
function in order to perform sync between device statistics and internal
structures. On HW limit event, driver calls to xfrm_state_check_expire()
to allow XFRM core take relevant decisions.

This separation between control logic (in XFRM) and data plane allows us
to packet reuse SW stack.

================================================================================
Configuration:

iproute2: https://lore.kernel.org/netdev/cover.1652179360.git.leonro@nvidia.com/

Full offload mode:
  ip xfrm state offload packet dev <if-name> dir <in|out>
  ip xfrm policy .... offload packet dev <if-name>
Crypto offload mode:
  ip xfrm state offload crypto dev <if-name> dir <in|out>
or (backward compatibility)
  ip xfrm state offload dev <if-name> dir <in|out>

================================================================================
Performance results:

TCP multi-stream, using iperf3 instance per-CPU.
+----------------------+--------+--------+--------+--------+---------+---------+
|                      | 1 CPU  | 2 CPUs | 4 CPUs | 8 CPUs | 16 CPUs | 32 CPUs |
|                      +--------+--------+--------+--------+---------+---------+
|                      |                   BW (Gbps)                           |
+----------------------+--------+--------+-------+---------+---------+---------+
| Baseline             | 27.9   | 59     | 93.1  | 92.8    | 93.7    | 94.4    |
+----------------------+--------+--------+-------+---------+---------+---------+
| Software IPsec       | 6      | 11.9   | 23.3  | 45.9    | 83.8    | 91.8    |
+----------------------+--------+--------+-------+---------+---------+---------+
| IPsec crypto offload | 15     | 29.7   | 58.5  | 89.6    | 90.4    | 90.8    |
+----------------------+--------+--------+-------+---------+---------+---------+
| IPsec packet offload   | 28     | 57     | 90.7  | 91      | 91.3    | 91.9    |
IPsec packet offload mode behaves as baseline and reaches linerate with same amount
of CPUs.

Setups details (similar for both sides):
* NIC: ConnectX6-DX dual port, 100 Gbps each.
  Single port used in the tests.
* CPU: Intel(R) Xeon(R) Platinum 8380 CPU @ 2.30GHz

================================================================================
Series together with mlx5 part:
https://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git/log/?h=xfrm-next

Thanks

Leon Romanovsky (8):
  xfrm: add new packet offload flag
  xfrm: allow state packet offload mode
  xfrm: add an interface to offload policy
  xfrm: add TX datapath support for IPsec packet offload mode
  xfrm: add RX datapath protection for IPsec packet offload mode
  xfrm: speed-up lookup of HW policies
  xfrm: add support to HW update soft and hard limits
  xfrm: document IPsec packet offload mode

 Documentation/networking/xfrm_device.rst      |  62 +++++++--
 .../inline_crypto/ch_ipsec/chcr_ipsec.c       |   4 +
 .../net/ethernet/intel/ixgbe/ixgbe_ipsec.c    |   5 +
 drivers/net/ethernet/intel/ixgbevf/ipsec.c    |   5 +
 .../mellanox/mlx5/core/en_accel/ipsec.c       |   4 +
 drivers/net/netdevsim/ipsec.c                 |   5 +
 include/linux/netdevice.h                     |   4 +
 include/net/xfrm.h                            | 124 ++++++++++++++----
 include/uapi/linux/xfrm.h                     |   6 +
 net/xfrm/xfrm_device.c                        | 109 +++++++++++++--
 net/xfrm/xfrm_output.c                        |  13 +-
 net/xfrm/xfrm_policy.c                        | 106 ++++++++++++++-
 net/xfrm/xfrm_state.c                         |  70 +++++++---
 net/xfrm/xfrm_user.c                          |  20 +++
 14 files changed, 474 insertions(+), 63 deletions(-)

-- 
2.38.1

[PATCH xfrm-next v7 1/8] xfrm: add new packet offload flag

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

In the next patches, the xfrm core code will be extended to support
new type of offload - packet offload. In that mode, both policy and state
should be specially configured in order to perform whole offloaded data
path.

Full offload takes care of encryption, decryption, encapsulation and
other operations with headers.

As this mode is new for XFRM policy flow, we can "start fresh" with flag
bits and release first and second bit for future use.

Reviewed-by: Raed Salem <raeds@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 include/net/xfrm.h        | 7 +++++++
 include/uapi/linux/xfrm.h | 6 ++++++
 net/xfrm/xfrm_device.c    | 3 +++
 net/xfrm/xfrm_user.c      | 2 ++
 4 files changed, 18 insertions(+)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index dbc81f5eb553..304001b76fc5 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -131,12 +131,19 @@ enum {
 	XFRM_DEV_OFFLOAD_OUT,
 };
 
+enum {
+	XFRM_DEV_OFFLOAD_UNSPECIFIED,
+	XFRM_DEV_OFFLOAD_CRYPTO,
+	XFRM_DEV_OFFLOAD_PACKET,
+};
+
 struct xfrm_dev_offload {
 	struct net_device	*dev;
 	netdevice_tracker	dev_tracker;
 	struct net_device	*real_dev;
 	unsigned long		offload_handle;
 	u8			dir : 2;
+	u8			type : 2;
 };
 
 struct xfrm_mode {
diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
index 4f84ea7ee14c..23543c33fee8 100644
--- a/include/uapi/linux/xfrm.h
+++ b/include/uapi/linux/xfrm.h
@@ -519,6 +519,12 @@ struct xfrm_user_offload {
  */
 #define XFRM_OFFLOAD_IPV6	1
 #define XFRM_OFFLOAD_INBOUND	2
+/* Two bits above are relevant for state path only, while
+ * offload is used for both policy and state flows.
+ *
+ * In policy offload mode, they are free and can be safely reused.
+ */
+#define XFRM_OFFLOAD_PACKET	4
 
 struct xfrm_userpolicy_default {
 #define XFRM_USERPOLICY_UNSPEC	0
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 5f5aafd418af..7c4e0f14df27 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -278,12 +278,15 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 	else
 		xso->dir = XFRM_DEV_OFFLOAD_OUT;
 
+	xso->type = XFRM_DEV_OFFLOAD_CRYPTO;
+
 	err = dev->xfrmdev_ops->xdo_dev_state_add(x);
 	if (err) {
 		xso->dev = NULL;
 		xso->dir = 0;
 		xso->real_dev = NULL;
 		netdev_put(dev, &xso->dev_tracker);
+		xso->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
 
 		if (err != -EOPNOTSUPP) {
 			NL_SET_ERR_MSG(extack, "Device failed to offload this state");
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index e73f9efc54c1..573b60873b60 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -943,6 +943,8 @@ static int copy_user_offload(struct xfrm_dev_offload *xso, struct sk_buff *skb)
 	xuo->ifindex = xso->dev->ifindex;
 	if (xso->dir == XFRM_DEV_OFFLOAD_IN)
 		xuo->flags = XFRM_OFFLOAD_INBOUND;
+	if (xso->type == XFRM_DEV_OFFLOAD_PACKET)
+		xuo->flags |= XFRM_OFFLOAD_PACKET;
 
 	return 0;
 }
-- 
2.38.1

[PATCH xfrm-next v7 2/8] xfrm: allow state packet offload mode

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Allow users to configure xfrm states with packet offload mode.
The packet mode must be requested both for policy and state, and
such requires us to do not implement fallback.

We explicitly return an error if requested packet mode can't
be configured.

Reviewed-by: Raed Salem <raeds@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 .../inline_crypto/ch_ipsec/chcr_ipsec.c       |  4 ++++
 .../net/ethernet/intel/ixgbe/ixgbe_ipsec.c    |  5 ++++
 drivers/net/ethernet/intel/ixgbevf/ipsec.c    |  5 ++++
 .../mellanox/mlx5/core/en_accel/ipsec.c       |  4 ++++
 drivers/net/netdevsim/ipsec.c                 |  5 ++++
 net/xfrm/xfrm_device.c                        | 24 +++++++++++++++----
 6 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ipsec/chcr_ipsec.c b/drivers/net/ethernet/chelsio/inline_crypto/ch_ipsec/chcr_ipsec.c
index 585590520076..ca21794281d6 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ipsec/chcr_ipsec.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ipsec/chcr_ipsec.c
@@ -283,6 +283,10 @@ static int ch_ipsec_xfrm_add_state(struct xfrm_state *x)
 		pr_debug("Cannot offload xfrm states with geniv other than seqiv\n");
 		return -EINVAL;
 	}
+	if (x->xso.type != XFRM_DEV_OFFLOAD_CRYPTO) {
+		pr_debug("Unsupported xfrm offload\n");
+		return -EINVAL;
+	}
 
 	sa_entry = kzalloc(sizeof(*sa_entry), GFP_KERNEL);
 	if (!sa_entry) {
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
index 774de63dd93a..53a969e34883 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
@@ -585,6 +585,11 @@ static int ixgbe_ipsec_add_sa(struct xfrm_state *xs)
 		return -EINVAL;
 	}
 
+	if (xs->xso.type != XFRM_DEV_OFFLOAD_CRYPTO) {
+		netdev_err(dev, "Unsupported ipsec offload type\n");
+		return -EINVAL;
+	}
+
 	if (xs->xso.dir == XFRM_DEV_OFFLOAD_IN) {
 		struct rx_sa rsa;
 
diff --git a/drivers/net/ethernet/intel/ixgbevf/ipsec.c b/drivers/net/ethernet/intel/ixgbevf/ipsec.c
index 9984ebc62d78..c1cf540d162a 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ipsec.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ipsec.c
@@ -280,6 +280,11 @@ static int ixgbevf_ipsec_add_sa(struct xfrm_state *xs)
 		return -EINVAL;
 	}
 
+	if (xs->xso.type != XFRM_DEV_OFFLOAD_CRYPTO) {
+		netdev_err(dev, "Unsupported ipsec offload type\n");
+		return -EINVAL;
+	}
+
 	if (xs->xso.dir == XFRM_DEV_OFFLOAD_IN) {
 		struct rx_sa rsa;
 
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 60499715db67..074f3a89f60b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
@@ -253,6 +253,10 @@ static inline int mlx5e_xfrm_validate_state(struct xfrm_state *x)
 		netdev_info(netdev, "Cannot offload xfrm states with geniv other than seqiv\n");
 		return -EINVAL;
 	}
+	if (x->xso.type != XFRM_DEV_OFFLOAD_CRYPTO) {
+		netdev_info(netdev, "Unsupported xfrm offload type\n");
+		return -EINVAL;
+	}
 	return 0;
 }
 
diff --git a/drivers/net/netdevsim/ipsec.c b/drivers/net/netdevsim/ipsec.c
index 386336a38f34..b93baf5c8bee 100644
--- a/drivers/net/netdevsim/ipsec.c
+++ b/drivers/net/netdevsim/ipsec.c
@@ -149,6 +149,11 @@ static int nsim_ipsec_add_sa(struct xfrm_state *xs)
 		return -EINVAL;
 	}
 
+	if (xs->xso.type != XFRM_DEV_OFFLOAD_CRYPTO) {
+		netdev_err(dev, "Unsupported ipsec offload type\n");
+		return -EINVAL;
+	}
+
 	/* find the first unused index */
 	ret = nsim_ipsec_find_empty_idx(ipsec);
 	if (ret < 0) {
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 7c4e0f14df27..dc4fb58dd7eb 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -216,6 +216,7 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 	struct xfrm_dev_offload *xso = &x->xso;
 	xfrm_address_t *saddr;
 	xfrm_address_t *daddr;
+	bool is_packet_offload;
 
 	if (!x->type_offload) {
 		NL_SET_ERR_MSG(extack, "Type doesn't support offload");
@@ -228,11 +229,13 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 		return -EINVAL;
 	}
 
-	if (xuo->flags & ~(XFRM_OFFLOAD_IPV6 | XFRM_OFFLOAD_INBOUND)) {
+	if (xuo->flags &
+	    ~(XFRM_OFFLOAD_IPV6 | XFRM_OFFLOAD_INBOUND | XFRM_OFFLOAD_PACKET)) {
 		NL_SET_ERR_MSG(extack, "Unrecognized flags in offload request");
 		return -EINVAL;
 	}
 
+	is_packet_offload = xuo->flags & XFRM_OFFLOAD_PACKET;
 	dev = dev_get_by_index(net, xuo->ifindex);
 	if (!dev) {
 		if (!(xuo->flags & XFRM_OFFLOAD_INBOUND)) {
@@ -247,7 +250,7 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 					x->props.family,
 					xfrm_smark_get(0, x));
 		if (IS_ERR(dst))
-			return 0;
+			return (is_packet_offload) ? -EINVAL : 0;
 
 		dev = dst->dev;
 
@@ -258,7 +261,7 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 	if (!dev->xfrmdev_ops || !dev->xfrmdev_ops->xdo_dev_state_add) {
 		xso->dev = NULL;
 		dev_put(dev);
-		return 0;
+		return (is_packet_offload) ? -EINVAL : 0;
 	}
 
 	if (x->props.flags & XFRM_STATE_ESN &&
@@ -278,7 +281,10 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 	else
 		xso->dir = XFRM_DEV_OFFLOAD_OUT;
 
-	xso->type = XFRM_DEV_OFFLOAD_CRYPTO;
+	if (is_packet_offload)
+		xso->type = XFRM_DEV_OFFLOAD_PACKET;
+	else
+		xso->type = XFRM_DEV_OFFLOAD_CRYPTO;
 
 	err = dev->xfrmdev_ops->xdo_dev_state_add(x);
 	if (err) {
@@ -288,7 +294,15 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 		netdev_put(dev, &xso->dev_tracker);
 		xso->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
 
-		if (err != -EOPNOTSUPP) {
+		/* User explicitly requested packet offload mode and configured
+		 * policy in addition to the XFRM state. So be civil to users,
+		 * and return an error instead of taking fallback path.
+		 *
+		 * This WARN_ON() can be seen as a documentation for driver
+		 * authors to do not return -EOPNOTSUPP in packet offload mode.
+		 */
+		WARN_ON(err == -EOPNOTSUPP && is_packet_offload);
+		if (err != -EOPNOTSUPP || is_packet_offload) {
 			NL_SET_ERR_MSG(extack, "Device failed to offload this state");
 			return err;
 		}
-- 
2.38.1

[PATCH xfrm-next v7 3/8] xfrm: add an interface to offload policy

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Extend netlink interface to add and delete XFRM policy from the device.
This functionality is a first step to implement packet IPsec offload solution.

Signed-off-by: Raed Salem <raeds@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 include/linux/netdevice.h |  3 ++
 include/net/xfrm.h        | 45 +++++++++++++++++++++++++
 net/xfrm/xfrm_device.c    | 67 ++++++++++++++++++++++++++++++++++++-
 net/xfrm/xfrm_policy.c    | 69 +++++++++++++++++++++++++++++++++++++++
 net/xfrm/xfrm_user.c      | 18 ++++++++++
 5 files changed, 201 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d45713a06568..5eb25f2b082f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1033,6 +1033,9 @@ struct xfrmdev_ops {
 	bool	(*xdo_dev_offload_ok) (struct sk_buff *skb,
 				       struct xfrm_state *x);
 	void	(*xdo_dev_state_advance_esn) (struct xfrm_state *x);
+	int	(*xdo_dev_policy_add) (struct xfrm_policy *x);
+	void	(*xdo_dev_policy_delete) (struct xfrm_policy *x);
+	void	(*xdo_dev_policy_free) (struct xfrm_policy *x);
 };
 #endif
 
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 304001b76fc5..e9c0cc245623 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -129,6 +129,7 @@ struct xfrm_state_walk {
 enum {
 	XFRM_DEV_OFFLOAD_IN = 1,
 	XFRM_DEV_OFFLOAD_OUT,
+	XFRM_DEV_OFFLOAD_FWD,
 };
 
 enum {
@@ -541,6 +542,8 @@ struct xfrm_policy {
 	struct xfrm_tmpl       	xfrm_vec[XFRM_MAX_DEPTH];
 	struct hlist_node	bydst_inexact_list;
 	struct rcu_head		rcu;
+
+	struct xfrm_dev_offload xdo;
 };
 
 static inline struct net *xp_net(const struct xfrm_policy *xp)
@@ -1585,6 +1588,8 @@ struct xfrm_state *xfrm_find_acq_byseq(struct net *net, u32 mark, u32 seq);
 int xfrm_state_delete(struct xfrm_state *x);
 int xfrm_state_flush(struct net *net, u8 proto, bool task_valid, bool sync);
 int xfrm_dev_state_flush(struct net *net, struct net_device *dev, bool task_valid);
+int xfrm_dev_policy_flush(struct net *net, struct net_device *dev,
+			  bool task_valid);
 void xfrm_sad_getinfo(struct net *net, struct xfrmk_sadinfo *si);
 void xfrm_spd_getinfo(struct net *net, struct xfrmk_spdinfo *si);
 u32 xfrm_replay_seqhi(struct xfrm_state *x, __be32 net_seq);
@@ -1897,6 +1902,9 @@ struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t featur
 int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 		       struct xfrm_user_offload *xuo,
 		       struct netlink_ext_ack *extack);
+int xfrm_dev_policy_add(struct net *net, struct xfrm_policy *xp,
+			struct xfrm_user_offload *xuo, u8 dir,
+			struct netlink_ext_ack *extack);
 bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x);
 
 static inline void xfrm_dev_state_advance_esn(struct xfrm_state *x)
@@ -1945,6 +1953,28 @@ static inline void xfrm_dev_state_free(struct xfrm_state *x)
 		netdev_put(dev, &xso->dev_tracker);
 	}
 }
+
+static inline void xfrm_dev_policy_delete(struct xfrm_policy *x)
+{
+	struct xfrm_dev_offload *xdo = &x->xdo;
+	struct net_device *dev = xdo->dev;
+
+	if (dev && dev->xfrmdev_ops && dev->xfrmdev_ops->xdo_dev_policy_delete)
+		dev->xfrmdev_ops->xdo_dev_policy_delete(x);
+}
+
+static inline void xfrm_dev_policy_free(struct xfrm_policy *x)
+{
+	struct xfrm_dev_offload *xdo = &x->xdo;
+	struct net_device *dev = xdo->dev;
+
+	if (dev && dev->xfrmdev_ops) {
+		if (dev->xfrmdev_ops->xdo_dev_policy_free)
+			dev->xfrmdev_ops->xdo_dev_policy_free(x);
+		xdo->dev = NULL;
+		netdev_put(dev, &xdo->dev_tracker);
+	}
+}
 #else
 static inline void xfrm_dev_resume(struct sk_buff *skb)
 {
@@ -1972,6 +2002,21 @@ static inline void xfrm_dev_state_free(struct xfrm_state *x)
 {
 }
 
+static inline int xfrm_dev_policy_add(struct net *net, struct xfrm_policy *xp,
+				      struct xfrm_user_offload *xuo, u8 dir,
+				      struct netlink_ext_ack *extack)
+{
+	return 0;
+}
+
+static inline void xfrm_dev_policy_delete(struct xfrm_policy *x)
+{
+}
+
+static inline void xfrm_dev_policy_free(struct xfrm_policy *x)
+{
+}
+
 static inline bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x)
 {
 	return false;
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index dc4fb58dd7eb..8e18abc5016f 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -312,6 +312,69 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 }
 EXPORT_SYMBOL_GPL(xfrm_dev_state_add);
 
+int xfrm_dev_policy_add(struct net *net, struct xfrm_policy *xp,
+			struct xfrm_user_offload *xuo, u8 dir,
+			struct netlink_ext_ack *extack)
+{
+	struct xfrm_dev_offload *xdo = &xp->xdo;
+	struct net_device *dev;
+	int err;
+
+	if (!xuo->flags || xuo->flags & ~XFRM_OFFLOAD_PACKET) {
+		/* We support only packet offload mode and it means
+		 * that user must set XFRM_OFFLOAD_PACKET bit.
+		 */
+		NL_SET_ERR_MSG(extack, "Unrecognized flags in offload request");
+		return -EINVAL;
+	}
+
+	dev = dev_get_by_index(net, xuo->ifindex);
+	if (!dev)
+		return -EINVAL;
+
+	if (!dev->xfrmdev_ops || !dev->xfrmdev_ops->xdo_dev_policy_add) {
+		xdo->dev = NULL;
+		dev_put(dev);
+		NL_SET_ERR_MSG(extack, "Policy offload is not supported");
+		return -EINVAL;
+	}
+
+	xdo->dev = dev;
+	netdev_tracker_alloc(dev, &xdo->dev_tracker, GFP_ATOMIC);
+	xdo->real_dev = dev;
+	xdo->type = XFRM_DEV_OFFLOAD_PACKET;
+	switch (dir) {
+	case XFRM_POLICY_IN:
+		xdo->dir = XFRM_DEV_OFFLOAD_IN;
+		break;
+	case XFRM_POLICY_OUT:
+		xdo->dir = XFRM_DEV_OFFLOAD_OUT;
+		break;
+	case XFRM_POLICY_FWD:
+		xdo->dir = XFRM_DEV_OFFLOAD_FWD;
+		break;
+	default:
+		xdo->dev = NULL;
+		dev_put(dev);
+		NL_SET_ERR_MSG(extack, "Unrecognized oflload direction");
+		return -EINVAL;
+	}
+
+	err = dev->xfrmdev_ops->xdo_dev_policy_add(xp);
+	if (err) {
+		xdo->dev = NULL;
+		xdo->real_dev = NULL;
+		xdo->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
+		xdo->dir = 0;
+		netdev_put(dev, &xdo->dev_tracker);
+		NL_SET_ERR_MSG(extack, "Device failed to offload this policy");
+		return err;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xfrm_dev_policy_add);
+
 bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x)
 {
 	int mtu;
@@ -414,8 +477,10 @@ static int xfrm_api_check(struct net_device *dev)
 
 static int xfrm_dev_down(struct net_device *dev)
 {
-	if (dev->features & NETIF_F_HW_ESP)
+	if (dev->features & NETIF_F_HW_ESP) {
 		xfrm_dev_state_flush(dev_net(dev), dev, true);
+		xfrm_dev_policy_flush(dev_net(dev), dev, true);
+	}
 
 	return NOTIFY_DONE;
 }
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index d80519c4e389..07f43729ac4e 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -425,6 +425,7 @@ void xfrm_policy_destroy(struct xfrm_policy *policy)
 	if (del_timer(&policy->timer) || del_timer(&policy->polq.hold_timer))
 		BUG();
 
+	xfrm_dev_policy_free(policy);
 	call_rcu(&policy->rcu, xfrm_policy_destroy_rcu);
 }
 EXPORT_SYMBOL(xfrm_policy_destroy);
@@ -1769,12 +1770,41 @@ xfrm_policy_flush_secctx_check(struct net *net, u8 type, bool task_valid)
 	}
 	return err;
 }
+
+static inline int xfrm_dev_policy_flush_secctx_check(struct net *net,
+						     struct net_device *dev,
+						     bool task_valid)
+{
+	struct xfrm_policy *pol;
+	int err = 0;
+
+	list_for_each_entry(pol, &net->xfrm.policy_all, walk.all) {
+		if (pol->walk.dead ||
+		    xfrm_policy_id2dir(pol->index) >= XFRM_POLICY_MAX ||
+		    pol->xdo.dev != dev)
+			continue;
+
+		err = security_xfrm_policy_delete(pol->security);
+		if (err) {
+			xfrm_audit_policy_delete(pol, 0, task_valid);
+			return err;
+		}
+	}
+	return err;
+}
 #else
 static inline int
 xfrm_policy_flush_secctx_check(struct net *net, u8 type, bool task_valid)
 {
 	return 0;
 }
+
+static inline int xfrm_dev_policy_flush_secctx_check(struct net *net,
+						     struct net_device *dev,
+						     bool task_valid)
+{
+	return 0;
+}
 #endif
 
 int xfrm_policy_flush(struct net *net, u8 type, bool task_valid)
@@ -1814,6 +1844,44 @@ int xfrm_policy_flush(struct net *net, u8 type, bool task_valid)
 }
 EXPORT_SYMBOL(xfrm_policy_flush);
 
+int xfrm_dev_policy_flush(struct net *net, struct net_device *dev,
+			  bool task_valid)
+{
+	int dir, err = 0, cnt = 0;
+	struct xfrm_policy *pol;
+
+	spin_lock_bh(&net->xfrm.xfrm_policy_lock);
+
+	err = xfrm_dev_policy_flush_secctx_check(net, dev, task_valid);
+	if (err)
+		goto out;
+
+again:
+	list_for_each_entry(pol, &net->xfrm.policy_all, walk.all) {
+		dir = xfrm_policy_id2dir(pol->index);
+		if (pol->walk.dead ||
+		    dir >= XFRM_POLICY_MAX ||
+		    pol->xdo.dev != dev)
+			continue;
+
+		__xfrm_policy_unlink(pol, dir);
+		spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
+		cnt++;
+		xfrm_audit_policy_delete(pol, 1, task_valid);
+		xfrm_policy_kill(pol);
+		spin_lock_bh(&net->xfrm.xfrm_policy_lock);
+		goto again;
+	}
+	if (cnt)
+		__xfrm_policy_inexact_flush(net);
+	else
+		err = -ESRCH;
+out:
+	spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
+	return err;
+}
+EXPORT_SYMBOL(xfrm_dev_policy_flush);
+
 int xfrm_policy_walk(struct net *net, struct xfrm_policy_walk *walk,
 		     int (*func)(struct xfrm_policy *, int, int, void*),
 		     void *data)
@@ -2245,6 +2313,7 @@ int xfrm_policy_delete(struct xfrm_policy *pol, int dir)
 	pol = __xfrm_policy_unlink(pol, dir);
 	spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
 	if (pol) {
+		xfrm_dev_policy_delete(pol);
 		xfrm_policy_kill(pol);
 		return 0;
 	}
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 573b60873b60..e2b563395656 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1869,6 +1869,15 @@ static struct xfrm_policy *xfrm_policy_construct(struct net *net,
 	if (attrs[XFRMA_IF_ID])
 		xp->if_id = nla_get_u32(attrs[XFRMA_IF_ID]);
 
+	/* configure the hardware if offload is requested */
+	if (attrs[XFRMA_OFFLOAD_DEV]) {
+		err = xfrm_dev_policy_add(net, xp,
+					  nla_data(attrs[XFRMA_OFFLOAD_DEV]),
+					  p->dir, extack);
+		if (err)
+			goto error;
+	}
+
 	return xp;
  error:
 	*errp = err;
@@ -1908,6 +1917,7 @@ static int xfrm_add_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
 	xfrm_audit_policy_add(xp, err ? 0 : 1, true);
 
 	if (err) {
+		xfrm_dev_policy_delete(xp);
 		security_xfrm_policy_free(xp->security);
 		kfree(xp);
 		return err;
@@ -2020,6 +2030,8 @@ static int dump_one_policy(struct xfrm_policy *xp, int dir, int count, void *ptr
 		err = xfrm_mark_put(skb, &xp->mark);
 	if (!err)
 		err = xfrm_if_id_put(skb, xp->if_id);
+	if (!err && xp->xdo.dev)
+		err = copy_user_offload(&xp->xdo, skb);
 	if (err) {
 		nlmsg_cancel(skb, nlh);
 		return err;
@@ -3343,6 +3355,8 @@ static int build_acquire(struct sk_buff *skb, struct xfrm_state *x,
 		err = xfrm_mark_put(skb, &xp->mark);
 	if (!err)
 		err = xfrm_if_id_put(skb, xp->if_id);
+	if (!err && xp->xdo.dev)
+		err = copy_user_offload(&xp->xdo, skb);
 	if (err) {
 		nlmsg_cancel(skb, nlh);
 		return err;
@@ -3461,6 +3475,8 @@ static int build_polexpire(struct sk_buff *skb, struct xfrm_policy *xp,
 		err = xfrm_mark_put(skb, &xp->mark);
 	if (!err)
 		err = xfrm_if_id_put(skb, xp->if_id);
+	if (!err && xp->xdo.dev)
+		err = copy_user_offload(&xp->xdo, skb);
 	if (err) {
 		nlmsg_cancel(skb, nlh);
 		return err;
@@ -3544,6 +3560,8 @@ static int xfrm_notify_policy(struct xfrm_policy *xp, int dir, const struct km_e
 		err = xfrm_mark_put(skb, &xp->mark);
 	if (!err)
 		err = xfrm_if_id_put(skb, xp->if_id);
+	if (!err && xp->xdo.dev)
+		err = copy_user_offload(&xp->xdo, skb);
 	if (err)
 		goto out_free_skb;
 
-- 
2.38.1

[PATCH xfrm-next v7 4/8] xfrm: add TX datapath support for IPsec packet offload mode

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

In IPsec packet mode, the device is going to encrypt and encapsulate
packets that are associated with offloaded policy. After successful
policy lookup to indicate if packets should be offloaded or not,
the stack forwards packets to the device to do the magic.

Signed-off-by: Raed Salem <raeds@nvidia.com>
Signed-off-by: Huy Nguyen <huyn@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 net/xfrm/xfrm_device.c | 15 +++++++++++++--
 net/xfrm/xfrm_output.c | 12 +++++++++++-
 net/xfrm/xfrm_policy.c | 21 ++++++++++++++++++++-
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 8e18abc5016f..6affb3d1e204 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -120,6 +120,16 @@ struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t featur
 	if (xo->flags & XFRM_GRO || x->xso.dir == XFRM_DEV_OFFLOAD_IN)
 		return skb;
 
+	/* The packet was sent to HW IPsec packet offload engine,
+	 * but to wrong device. Drop the packet, so it won't skip
+	 * XFRM stack.
+	 */
+	if (x->xso.type == XFRM_DEV_OFFLOAD_PACKET && x->xso.dev != dev) {
+		kfree_skb(skb);
+		dev_core_stats_tx_dropped_inc(dev);
+		return NULL;
+	}
+
 	/* This skb was already validated on the upper/virtual dev */
 	if ((x->xso.dev != dev) && (x->xso.real_dev == dev))
 		return skb;
@@ -385,8 +395,9 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x)
 	if (!x->type_offload || x->encap)
 		return false;
 
-	if ((!dev || (dev == xfrm_dst_path(dst)->dev)) &&
-	    (!xdst->child->xfrm)) {
+	if (x->xso.type == XFRM_DEV_OFFLOAD_PACKET ||
+	    ((!dev || (dev == xfrm_dst_path(dst)->dev)) &&
+	     !xdst->child->xfrm)) {
 		mtu = xfrm_state_mtu(x, xdst->child_mtu_cached);
 		if (skb->len <= mtu)
 			goto ok;
diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
index 9a5e79a38c67..ce9e360a96e2 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -494,7 +494,7 @@ static int xfrm_output_one(struct sk_buff *skb, int err)
 	struct xfrm_state *x = dst->xfrm;
 	struct net *net = xs_net(x);
 
-	if (err <= 0)
+	if (err <= 0 || x->xso.type == XFRM_DEV_OFFLOAD_PACKET)
 		goto resume;
 
 	do {
@@ -718,6 +718,16 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
 		break;
 	}
 
+	if (x->xso.type == XFRM_DEV_OFFLOAD_PACKET) {
+		if (!xfrm_dev_offload_ok(skb, x)) {
+			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
+			kfree_skb(skb);
+			return -EHOSTUNREACH;
+		}
+
+		return xfrm_output_resume(sk, skb, 0);
+	}
+
 	secpath_reset(skb);
 
 	if (xfrm_dev_offload_ok(skb, x)) {
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 07f43729ac4e..06226942a152 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2619,6 +2619,7 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	int tos;
 	int family = policy->selector.family;
 	xfrm_address_t saddr, daddr;
+	struct dst_entry *dst1;
 
 	xfrm_flowi_addr_get(fl, &saddr, &daddr, family);
 
@@ -2628,7 +2629,8 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 
 	for (; i < nx; i++) {
 		struct xfrm_dst *xdst = xfrm_alloc_dst(net, family);
-		struct dst_entry *dst1 = &xdst->u.dst;
+
+		dst1 = &xdst->u.dst;
 
 		err = PTR_ERR(xdst);
 		if (IS_ERR(xdst)) {
@@ -2708,6 +2710,23 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	if (!dev)
 		goto free_dst;
 
+	dst1 = &xdst0->u.dst;
+	/* Packet offload: both policy and SA should be offloaded */
+	if ((policy->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
+	     dst1->xfrm->xso.type != XFRM_DEV_OFFLOAD_PACKET) ||
+	    (policy->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
+	     dst1->xfrm->xso.type == XFRM_DEV_OFFLOAD_PACKET)) {
+		err = -EINVAL;
+		goto free_dst;
+	}
+
+	/* Packet offload: both policy and SA should have same device */
+	if (policy->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
+	    policy->xdo.dev != dst1->xfrm->xso.dev) {
+		err = -EINVAL;
+		goto free_dst;
+	}
+
 	xfrm_init_path(xdst0, dst, nfheader_len);
 	xfrm_init_pmtu(bundle, nx);
 
-- 
2.38.1

[PATCH xfrm-next v7 5/8] xfrm: add RX datapath protection for IPsec packet offload mode

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Traffic received by device with enabled IPsec packet offload should
be forwarded to the stack only after decryption, packet headers and
trailers removed.

Such packets are expected to be seen as normal (non-XFRM) ones, while
not-supported packets should be dropped by the HW.

Reviewed-by: Raed Salem <raeds@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 include/net/xfrm.h | 55 +++++++++++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 23 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index e9c0cc245623..00ce7a68bf3c 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1102,6 +1102,29 @@ xfrm_state_addr_cmp(const struct xfrm_tmpl *tmpl, const struct xfrm_state *x, un
 	return !0;
 }
 
+#ifdef CONFIG_XFRM
+static inline struct xfrm_state *xfrm_input_state(struct sk_buff *skb)
+{
+	struct sec_path *sp = skb_sec_path(skb);
+
+	return sp->xvec[sp->len - 1];
+}
+#endif
+
+static inline struct xfrm_offload *xfrm_offload(struct sk_buff *skb)
+{
+#ifdef CONFIG_XFRM
+	struct sec_path *sp = skb_sec_path(skb);
+
+	if (!sp || !sp->olen || sp->len != sp->olen)
+		return NULL;
+
+	return &sp->ovec[sp->olen - 1];
+#else
+	return NULL;
+#endif
+}
+
 #ifdef CONFIG_XFRM
 int __xfrm_policy_check(struct sock *, int dir, struct sk_buff *skb,
 			unsigned short family);
@@ -1133,10 +1156,19 @@ static inline int __xfrm_policy_check2(struct sock *sk, int dir,
 {
 	struct net *net = dev_net(skb->dev);
 	int ndir = dir | (reverse ? XFRM_POLICY_MASK + 1 : 0);
+	struct xfrm_offload *xo = xfrm_offload(skb);
+	struct xfrm_state *x;
 
 	if (sk && sk->sk_policy[XFRM_POLICY_IN])
 		return __xfrm_policy_check(sk, ndir, skb, family);
 
+	if (xo) {
+		x = xfrm_input_state(skb);
+		if (x->xso.type == XFRM_DEV_OFFLOAD_PACKET)
+			return (xo->flags & CRYPTO_DONE) &&
+			       (xo->status & CRYPTO_SUCCESS);
+	}
+
 	return __xfrm_check_nopolicy(net, skb, dir) ||
 	       __xfrm_check_dev_nopolicy(skb, dir, family) ||
 	       __xfrm_policy_check(sk, ndir, skb, family);
@@ -1870,29 +1902,6 @@ static inline void xfrm_states_delete(struct xfrm_state **states, int n)
 }
 #endif
 
-#ifdef CONFIG_XFRM
-static inline struct xfrm_state *xfrm_input_state(struct sk_buff *skb)
-{
-	struct sec_path *sp = skb_sec_path(skb);
-
-	return sp->xvec[sp->len - 1];
-}
-#endif
-
-static inline struct xfrm_offload *xfrm_offload(struct sk_buff *skb)
-{
-#ifdef CONFIG_XFRM
-	struct sec_path *sp = skb_sec_path(skb);
-
-	if (!sp || !sp->olen || sp->len != sp->olen)
-		return NULL;
-
-	return &sp->ovec[sp->olen - 1];
-#else
-	return NULL;
-#endif
-}
-
 void __init xfrm_dev_init(void);
 
 #ifdef CONFIG_XFRM_OFFLOAD
-- 
2.38.1

[PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Devices that implement IPsec packet offload mode should offload SA and
policies too. In RX path, it causes to the situation that HW will always
have higher priority over any SW policies.

It means that we don't need to perform any search of inexact policies
and/or priority checks if HW policy was discovered. In such situation,
the HW will catch the packets anyway and HW can still implement inexact
lookups.

In case specific policy is not found, we will continue with packet lookup and
check for existence of HW policies in inexact list.

HW policies are added to the head of SPD to ensure fast lookup, as XFRM
iterates over all policies in the loop.

The same solution of adding HW SAs at the begging of the list is applied
to SA database too. However, we don't need to change lookups as they are
sorted by insertion order and not priority.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 net/xfrm/xfrm_policy.c | 16 ++++++----
 net/xfrm/xfrm_state.c  | 66 ++++++++++++++++++++++++++++++++----------
 2 files changed, 62 insertions(+), 20 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 06226942a152..93a4a9149f8c 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -536,7 +536,7 @@ static void xfrm_dst_hash_transfer(struct net *net,
 		__get_hash_thresh(net, pol->family, dir, &dbits, &sbits);
 		h = __addr_hash(&pol->selector.daddr, &pol->selector.saddr,
 				pol->family, nhashmask, dbits, sbits);
-		if (!entry0) {
+		if (!entry0 || pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {
 			hlist_del_rcu(&pol->bydst);
 			hlist_add_head_rcu(&pol->bydst, ndsttable + h);
 			h0 = h;
@@ -867,7 +867,7 @@ static void xfrm_policy_inexact_list_reinsert(struct net *net,
 				break;
 		}
 
-		if (newpos)
+		if (newpos && policy->xdo.type != XFRM_DEV_OFFLOAD_PACKET)
 			hlist_add_behind_rcu(&policy->bydst, newpos);
 		else
 			hlist_add_head_rcu(&policy->bydst, &n->hhead);
@@ -1348,7 +1348,7 @@ static void xfrm_hash_rebuild(struct work_struct *work)
 			else
 				break;
 		}
-		if (newpos)
+		if (newpos && policy->xdo.type != XFRM_DEV_OFFLOAD_PACKET)
 			hlist_add_behind_rcu(&policy->bydst, newpos);
 		else
 			hlist_add_head_rcu(&policy->bydst, chain);
@@ -1525,7 +1525,7 @@ static void xfrm_policy_insert_inexact_list(struct hlist_head *chain,
 			break;
 	}
 
-	if (newpos)
+	if (newpos && policy->xdo.type != XFRM_DEV_OFFLOAD_PACKET)
 		hlist_add_behind_rcu(&policy->bydst_inexact_list, newpos);
 	else
 		hlist_add_head_rcu(&policy->bydst_inexact_list, chain);
@@ -1562,9 +1562,12 @@ static struct xfrm_policy *xfrm_policy_insert_list(struct hlist_head *chain,
 			break;
 	}
 
-	if (newpos)
+	if (newpos && policy->xdo.type != XFRM_DEV_OFFLOAD_PACKET)
 		hlist_add_behind_rcu(&policy->bydst, &newpos->bydst);
 	else
+		/* Packet offload policies enter to the head
+		 * to speed-up lookups.
+		 */
 		hlist_add_head_rcu(&policy->bydst, chain);
 
 	return delpol;
@@ -2181,6 +2184,9 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
 			break;
 		}
 	}
+	if (ret && ret->xdo.type == XFRM_DEV_OFFLOAD_PACKET)
+		goto skip_inexact;
+
 	bin = xfrm_policy_inexact_lookup_rcu(net, type, family, dir, if_id);
 	if (!bin || !xfrm_policy_find_inexact_candidates(&cand, bin, saddr,
 							 daddr))
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 3d2fe7712ac5..cfc8c72b173d 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -84,6 +84,25 @@ static unsigned int xfrm_seq_hash(struct net *net, u32 seq)
 	return __xfrm_seq_hash(seq, net->xfrm.state_hmask);
 }
 
+#define XFRM_STATE_INSERT(by, _n, _h, _type)                               \
+	{                                                                  \
+		struct xfrm_state *_x = NULL;                              \
+									   \
+		if (_type != XFRM_DEV_OFFLOAD_PACKET) {                    \
+			hlist_for_each_entry_rcu(_x, _h, by) {             \
+				if (_x->xso.type == XFRM_DEV_OFFLOAD_PACKET) \
+					continue;                          \
+				break;                                     \
+			}                                                  \
+		}                                                          \
+									   \
+		if (!_x || _x->xso.type == XFRM_DEV_OFFLOAD_PACKET)        \
+			/* SAD is empty or consist from HW SAs only */     \
+			hlist_add_head_rcu(_n, _h);                        \
+		else                                                       \
+			hlist_add_before_rcu(_n, &_x->by);                 \
+	}
+
 static void xfrm_hash_transfer(struct hlist_head *list,
 			       struct hlist_head *ndsttable,
 			       struct hlist_head *nsrctable,
@@ -100,23 +119,25 @@ static void xfrm_hash_transfer(struct hlist_head *list,
 		h = __xfrm_dst_hash(&x->id.daddr, &x->props.saddr,
 				    x->props.reqid, x->props.family,
 				    nhashmask);
-		hlist_add_head_rcu(&x->bydst, ndsttable + h);
+		XFRM_STATE_INSERT(bydst, &x->bydst, ndsttable + h, x->xso.type);
 
 		h = __xfrm_src_hash(&x->id.daddr, &x->props.saddr,
 				    x->props.family,
 				    nhashmask);
-		hlist_add_head_rcu(&x->bysrc, nsrctable + h);
+		XFRM_STATE_INSERT(bysrc, &x->bysrc, nsrctable + h, x->xso.type);
 
 		if (x->id.spi) {
 			h = __xfrm_spi_hash(&x->id.daddr, x->id.spi,
 					    x->id.proto, x->props.family,
 					    nhashmask);
-			hlist_add_head_rcu(&x->byspi, nspitable + h);
+			XFRM_STATE_INSERT(byspi, &x->byspi, nspitable + h,
+					  x->xso.type);
 		}
 
 		if (x->km.seq) {
 			h = __xfrm_seq_hash(x->km.seq, nhashmask);
-			hlist_add_head_rcu(&x->byseq, nseqtable + h);
+			XFRM_STATE_INSERT(byseq, &x->byseq, nseqtable + h,
+					  x->xso.type);
 		}
 	}
 }
@@ -1166,16 +1187,24 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 			spin_lock_bh(&net->xfrm.xfrm_state_lock);
 			x->km.state = XFRM_STATE_ACQ;
 			list_add(&x->km.all, &net->xfrm.state_all);
-			hlist_add_head_rcu(&x->bydst, net->xfrm.state_bydst + h);
+			XFRM_STATE_INSERT(bydst, &x->bydst,
+					  net->xfrm.state_bydst + h,
+					  x->xso.type);
 			h = xfrm_src_hash(net, daddr, saddr, encap_family);
-			hlist_add_head_rcu(&x->bysrc, net->xfrm.state_bysrc + h);
+			XFRM_STATE_INSERT(bysrc, &x->bysrc,
+					  net->xfrm.state_bysrc + h,
+					  x->xso.type);
 			if (x->id.spi) {
 				h = xfrm_spi_hash(net, &x->id.daddr, x->id.spi, x->id.proto, encap_family);
-				hlist_add_head_rcu(&x->byspi, net->xfrm.state_byspi + h);
+				XFRM_STATE_INSERT(byspi, &x->byspi,
+						  net->xfrm.state_byspi + h,
+						  x->xso.type);
 			}
 			if (x->km.seq) {
 				h = xfrm_seq_hash(net, x->km.seq);
-				hlist_add_head_rcu(&x->byseq, net->xfrm.state_byseq + h);
+				XFRM_STATE_INSERT(byseq, &x->byseq,
+						  net->xfrm.state_byseq + h,
+						  x->xso.type);
 			}
 			x->lft.hard_add_expires_seconds = net->xfrm.sysctl_acq_expires;
 			hrtimer_start(&x->mtimer,
@@ -1280,22 +1309,26 @@ static void __xfrm_state_insert(struct xfrm_state *x)
 
 	h = xfrm_dst_hash(net, &x->id.daddr, &x->props.saddr,
 			  x->props.reqid, x->props.family);
-	hlist_add_head_rcu(&x->bydst, net->xfrm.state_bydst + h);
+	XFRM_STATE_INSERT(bydst, &x->bydst, net->xfrm.state_bydst + h,
+			  x->xso.type);
 
 	h = xfrm_src_hash(net, &x->id.daddr, &x->props.saddr, x->props.family);
-	hlist_add_head_rcu(&x->bysrc, net->xfrm.state_bysrc + h);
+	XFRM_STATE_INSERT(bysrc, &x->bysrc, net->xfrm.state_bysrc + h,
+			  x->xso.type);
 
 	if (x->id.spi) {
 		h = xfrm_spi_hash(net, &x->id.daddr, x->id.spi, x->id.proto,
 				  x->props.family);
 
-		hlist_add_head_rcu(&x->byspi, net->xfrm.state_byspi + h);
+		XFRM_STATE_INSERT(byspi, &x->byspi, net->xfrm.state_byspi + h,
+				  x->xso.type);
 	}
 
 	if (x->km.seq) {
 		h = xfrm_seq_hash(net, x->km.seq);
 
-		hlist_add_head_rcu(&x->byseq, net->xfrm.state_byseq + h);
+		XFRM_STATE_INSERT(byseq, &x->byseq, net->xfrm.state_byseq + h,
+				  x->xso.type);
 	}
 
 	hrtimer_start(&x->mtimer, ktime_set(1, 0), HRTIMER_MODE_REL_SOFT);
@@ -1409,9 +1442,11 @@ static struct xfrm_state *__find_acq_core(struct net *net,
 			      ktime_set(net->xfrm.sysctl_acq_expires, 0),
 			      HRTIMER_MODE_REL_SOFT);
 		list_add(&x->km.all, &net->xfrm.state_all);
-		hlist_add_head_rcu(&x->bydst, net->xfrm.state_bydst + h);
+		XFRM_STATE_INSERT(bydst, &x->bydst, net->xfrm.state_bydst + h,
+				  x->xso.type);
 		h = xfrm_src_hash(net, daddr, saddr, family);
-		hlist_add_head_rcu(&x->bysrc, net->xfrm.state_bysrc + h);
+		XFRM_STATE_INSERT(bysrc, &x->bysrc, net->xfrm.state_bysrc + h,
+				  x->xso.type);
 
 		net->xfrm.state_num++;
 
@@ -2085,7 +2120,8 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high)
 		spin_lock_bh(&net->xfrm.xfrm_state_lock);
 		x->id.spi = newspi;
 		h = xfrm_spi_hash(net, &x->id.daddr, x->id.spi, x->id.proto, x->props.family);
-		hlist_add_head_rcu(&x->byspi, net->xfrm.state_byspi + h);
+		XFRM_STATE_INSERT(byspi, &x->byspi, net->xfrm.state_byspi + h,
+				  x->xso.type);
 		spin_unlock_bh(&net->xfrm.xfrm_state_lock);
 
 		err = 0;
-- 
2.38.1

[PATCH xfrm-next v7 7/8] xfrm: add support to HW update soft and hard limits

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Both in RX and TX, the traffic that performs IPsec packet offload
transformation is accounted by HW. It is needed to properly handle
hard limits that require to drop the packet.

It means that XFRM core needs to update internal counters with the one
that accounted by the HW, so new callbacks are introduced in this patch.

In case of soft or hard limit is occurred, the driver should call to
xfrm_state_check_expire() that will perform key rekeying exactly as
done by XFRM core.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 include/linux/netdevice.h |  1 +
 include/net/xfrm.h        | 17 +++++++++++++++++
 net/xfrm/xfrm_output.c    |  1 -
 net/xfrm/xfrm_state.c     |  4 ++++
 4 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5eb25f2b082f..2160dc77211d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1033,6 +1033,7 @@ struct xfrmdev_ops {
 	bool	(*xdo_dev_offload_ok) (struct sk_buff *skb,
 				       struct xfrm_state *x);
 	void	(*xdo_dev_state_advance_esn) (struct xfrm_state *x);
+	void	(*xdo_dev_state_update_curlft) (struct xfrm_state *x);
 	int	(*xdo_dev_policy_add) (struct xfrm_policy *x);
 	void	(*xdo_dev_policy_delete) (struct xfrm_policy *x);
 	void	(*xdo_dev_policy_free) (struct xfrm_policy *x);
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 00ce7a68bf3c..3982c43117d0 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1571,6 +1571,23 @@ struct xfrm_state *xfrm_stateonly_find(struct net *net, u32 mark, u32 if_id,
 struct xfrm_state *xfrm_state_lookup_byspi(struct net *net, __be32 spi,
 					      unsigned short family);
 int xfrm_state_check_expire(struct xfrm_state *x);
+#ifdef CONFIG_XFRM_OFFLOAD
+static inline void xfrm_dev_state_update_curlft(struct xfrm_state *x)
+{
+	struct xfrm_dev_offload *xdo = &x->xso;
+	struct net_device *dev = xdo->dev;
+
+	if (x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
+		return;
+
+	if (dev && dev->xfrmdev_ops &&
+	    dev->xfrmdev_ops->xdo_dev_state_update_curlft)
+		dev->xfrmdev_ops->xdo_dev_state_update_curlft(x);
+
+}
+#else
+static inline void xfrm_dev_state_update_curlft(struct xfrm_state *x) {}
+#endif
 void xfrm_state_insert(struct xfrm_state *x);
 int xfrm_state_add(struct xfrm_state *x);
 int xfrm_state_update(struct xfrm_state *x);
diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
index ce9e360a96e2..819c7cd87d6b 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -560,7 +560,6 @@ static int xfrm_output_one(struct sk_buff *skb, int err)
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTSTATEPROTOERROR);
 			goto error_nolock;
 		}
-
 		dst = skb_dst_pop(skb);
 		if (!dst) {
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index cfc8c72b173d..5076f9d7a752 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -570,6 +570,8 @@ static enum hrtimer_restart xfrm_timer_handler(struct hrtimer *me)
 	int err = 0;
 
 	spin_lock(&x->lock);
+	xfrm_dev_state_update_curlft(x);
+
 	if (x->km.state == XFRM_STATE_DEAD)
 		goto out;
 	if (x->km.state == XFRM_STATE_EXPIRED)
@@ -1821,6 +1823,8 @@ EXPORT_SYMBOL(xfrm_state_update);
 
 int xfrm_state_check_expire(struct xfrm_state *x)
 {
+	xfrm_dev_state_update_curlft(x);
+
 	if (!x->curlft.use_time)
 		x->curlft.use_time = ktime_get_real_seconds();
 
-- 
2.38.1

[PATCH xfrm-next v7 8/8] xfrm: document IPsec packet offload mode

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Extend XFRM device offload API description with newly
added packet offload mode.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 Documentation/networking/xfrm_device.rst | 62 ++++++++++++++++++++----
 1 file changed, 53 insertions(+), 9 deletions(-)

diff --git a/Documentation/networking/xfrm_device.rst b/Documentation/networking/xfrm_device.rst
index 01391dfd37d9..40ea565812f6 100644
--- a/Documentation/networking/xfrm_device.rst
+++ b/Documentation/networking/xfrm_device.rst
@@ -5,6 +5,7 @@ XFRM device - offloading the IPsec computations
 ===============================================
 
 Shannon Nelson <shannon.nelson@oracle.com>
+Leon Romanovsky <leonro@nvidia.com>
 
 
 Overview
@@ -18,10 +19,21 @@ can radically increase throughput and decrease CPU utilization.  The XFRM
 Device interface allows NIC drivers to offer to the stack access to the
 hardware offload.
 
+Right now, there are two types of hardware offload that kernel supports.
+ * IPsec crypto offload:
+   * NIC performs encrypt/decrypt
+   * Kernel does everything else
+ * IPsec packet offload:
+   * NIC performs encrypt/decrypt
+   * NIC does encapsulation
+   * Kernel and NIC have SA and policy in-sync
+   * NIC handles the SA and policies states
+   * The Kernel talks to the keymanager
+
 Userland access to the offload is typically through a system such as
 libreswan or KAME/raccoon, but the iproute2 'ip xfrm' command set can
 be handy when experimenting.  An example command might look something
-like this::
+like this for crypto offload:
 
   ip x s add proto esp dst 14.0.0.70 src 14.0.0.52 spi 0x07 mode transport \
      reqid 0x07 replay-window 32 \
@@ -29,6 +41,17 @@ like this::
      sel src 14.0.0.52/24 dst 14.0.0.70/24 proto tcp \
      offload dev eth4 dir in
 
+and for packetoffload
+
+  ip x s add proto esp dst 14.0.0.70 src 14.0.0.52 spi 0x07 mode transport \
+     reqid 0x07 replay-window 32 \
+     aead 'rfc4106(gcm(aes))' 0x44434241343332312423222114131211f4f3f2f1 128 \
+     sel src 14.0.0.52/24 dst 14.0.0.70/24 proto tcp \
+     offload packet dev eth4 dir in
+
+  ip x p add src 14.0.0.70 dst 14.0.0.52 offload packet dev eth4 dir in
+  tmpl src 14.0.0.70 dst 14.0.0.52 proto esp reqid 10000 mode transport
+
 Yes, that's ugly, but that's what shell scripts and/or libreswan are for.
 
 
@@ -40,17 +63,24 @@ Callbacks to implement
 
   /* from include/linux/netdevice.h */
   struct xfrmdev_ops {
+        /* Crypto and Full offload callbacks */
 	int	(*xdo_dev_state_add) (struct xfrm_state *x);
 	void	(*xdo_dev_state_delete) (struct xfrm_state *x);
 	void	(*xdo_dev_state_free) (struct xfrm_state *x);
 	bool	(*xdo_dev_offload_ok) (struct sk_buff *skb,
 				       struct xfrm_state *x);
 	void    (*xdo_dev_state_advance_esn) (struct xfrm_state *x);
+
+        /* Solely packet offload callbacks */
+	void    (*xdo_dev_state_update_curlft) (struct xfrm_state *x);
+	int	(*xdo_dev_policy_add) (struct xfrm_policy *x);
+	void	(*xdo_dev_policy_delete) (struct xfrm_policy *x);
+	void	(*xdo_dev_policy_free) (struct xfrm_policy *x);
   };
 
-The NIC driver offering ipsec offload will need to implement these
-callbacks to make the offload available to the network stack's
-XFRM subsystem.  Additionally, the feature bits NETIF_F_HW_ESP and
+The NIC driver offering ipsec offload will need to implement callbacks
+relevant to supported offload to make the offload available to the network
+stack's XFRM subsystem. Additionally, the feature bits NETIF_F_HW_ESP and
 NETIF_F_HW_ESP_TX_CSUM will signal the availability of the offload.
 
 
@@ -79,7 +109,8 @@ and an indication of whether it is for Rx or Tx.  The driver should
 
 		===========   ===================================
 		0             success
-		-EOPNETSUPP   offload not supported, try SW IPsec
+		-EOPNETSUPP   offload not supported, try SW IPsec,
+                              not applicable for packet offload mode
 		other         fail the request
 		===========   ===================================
 
@@ -96,6 +127,7 @@ will serviceable.  This can check the packet information to be sure the
 offload can be supported (e.g. IPv4 or IPv6, no IPv4 options, etc) and
 return true of false to signify its support.
 
+Crypto offload mode:
 When ready to send, the driver needs to inspect the Tx packet for the
 offload information, including the opaque context, and set up the packet
 send accordingly::
@@ -139,13 +171,25 @@ the stack in xfrm_input().
 In ESN mode, xdo_dev_state_advance_esn() is called from xfrm_replay_advance_esn().
 Driver will check packet seq number and update HW ESN state machine if needed.
 
+Full offload mode:
+HW adds and deletes XFRM headers. So in RX path, XFRM stack is bypassed if HW
+reported success. In TX path, the packet lefts kernel without extra header
+and not encrypted, the HW is responsible to perform it.
+
 When the SA is removed by the user, the driver's xdo_dev_state_delete()
-is asked to disable the offload.  Later, xdo_dev_state_free() is called
-from a garbage collection routine after all reference counts to the state
+and xdo_dev_policy_delete() are asked to disable the offload.  Later,
+xdo_dev_state_free() and xdo_dev_policy_free() are called from a garbage
+collection routine after all reference counts to the state and policy
 have been removed and any remaining resources can be cleared for the
 offload state.  How these are used by the driver will depend on specific
 hardware needs.
 
 As a netdev is set to DOWN the XFRM stack's netdev listener will call
-xdo_dev_state_delete() and xdo_dev_state_free() on any remaining offloaded
-states.
+xdo_dev_state_delete(), xdo_dev_policy_delete(), xdo_dev_state_free() and
+xdo_dev_policy_free() on any remaining offloaded states.
+
+Outcome of HW handling packets, the XFRM core can't count hard, soft limits.
+The HW/driver are responsible to perform it and provide accurate data when
+xdo_dev_state_update_curlft() is called. In case of one of these limits
+occuried, the driver needs to call to xfrm_state_check_expire() to make sure
+that XFRM performs rekeying sequence.
-- 
2.38.1

Re: [PATCH xfrm-next v7 0/8] Extend XFRM core to allow packet offload configuration

[Leon Romanovsky]

On Wed, Nov 09, 2022 at 02:54:28PM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Changelog:
> v7: As was discussed in IPsec workshop:

Steffen, can we please merge the series so we won't miss another kernel
release?

Thanks

Re: [PATCH xfrm-next v7 0/8] Extend XFRM core to allow packet offload configuration

[Steffen Klassert]

On Tue, Nov 15, 2022 at 08:09:48PM +0200, Leon Romanovsky wrote:
> On Wed, Nov 09, 2022 at 02:54:28PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > Changelog:
> > v7: As was discussed in IPsec workshop:
> 
> Steffen, can we please merge the series so we won't miss another kernel
> release?

I'm already reviewing the patchset. But as I said at the
IPsec workshop, there is no guarantee that it will make
it during this release cycle.

Re: [PATCH xfrm-next v7 0/8] Extend XFRM core to allow packet offload configuration

[Leon Romanovsky]

On Tue, Nov 15, 2022 at 07:30:20PM +0100, Steffen Klassert wrote:
> On Tue, Nov 15, 2022 at 08:09:48PM +0200, Leon Romanovsky wrote:
> > On Wed, Nov 09, 2022 at 02:54:28PM +0200, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@nvidia.com>
> > > 
> > > Changelog:
> > > v7: As was discussed in IPsec workshop:
> > 
> > Steffen, can we please merge the series so we won't miss another kernel
> > release?
> 
> I'm already reviewing the patchset. But as I said at the
> IPsec workshop, there is no guarantee that it will make
> it during this release cycle.

Of course, there is no guarantee, but let's not skip it if it is ready.

Thanks

Re: [PATCH xfrm-next v7 0/8] Extend XFRM core to allow packet offload configuration

[Saeed Mahameed]

On 15 Nov 21:00, Leon Romanovsky wrote:
>On Tue, Nov 15, 2022 at 07:30:20PM +0100, Steffen Klassert wrote:
>> On Tue, Nov 15, 2022 at 08:09:48PM +0200, Leon Romanovsky wrote:
>> > On Wed, Nov 09, 2022 at 02:54:28PM +0200, Leon Romanovsky wrote:
>> > > From: Leon Romanovsky <leonro@nvidia.com>
>> > >
>> > > Changelog:
>> > > v7: As was discussed in IPsec workshop:
>> >
>> > Steffen, can we please merge the series so we won't miss another kernel
>> > release?
>>
>> I'm already reviewing the patchset. But as I said at the
>> IPsec workshop, there is no guarantee that it will make
>> it during this release cycle.
>

BTW mlx5 patches are almost ready and fully reviewed, will be ready by
early next week.

Steffen in case you will be ready, how do you want to receive the whole
package? I guess we will need to post all the patches for a final time,
including mlx5 driver.

Re: [PATCH xfrm-next v7 4/8] xfrm: add TX datapath support for IPsec packet offload mode

[Steffen Klassert]

On Wed, Nov 09, 2022 at 02:54:32PM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>

> @@ -2708,6 +2710,23 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
>  	if (!dev)
>  		goto free_dst;
>  
> +	dst1 = &xdst0->u.dst;
> +	/* Packet offload: both policy and SA should be offloaded */
> +	if ((policy->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
> +	     dst1->xfrm->xso.type != XFRM_DEV_OFFLOAD_PACKET) ||
> +	    (policy->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
> +	     dst1->xfrm->xso.type == XFRM_DEV_OFFLOAD_PACKET)) {
> +		err = -EINVAL;
> +		goto free_dst;
> +	}
> +
> +	/* Packet offload: both policy and SA should have same device */
> +	if (policy->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
> +	    policy->xdo.dev != dst1->xfrm->xso.dev) {
> +		err = -EINVAL;
> +		goto free_dst;
> +	}
> +

This is the wrong place for these checks. Things went already wrong
in the lookup if policy and state do not match here.

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Steffen Klassert]

On Wed, Nov 09, 2022 at 02:54:34PM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> @@ -1166,16 +1187,24 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
>  			spin_lock_bh(&net->xfrm.xfrm_state_lock);
>  			x->km.state = XFRM_STATE_ACQ;
>  			list_add(&x->km.all, &net->xfrm.state_all);
> -			hlist_add_head_rcu(&x->bydst, net->xfrm.state_bydst + h);
> +			XFRM_STATE_INSERT(bydst, &x->bydst,
> +					  net->xfrm.state_bydst + h,
> +					  x->xso.type);
>  			h = xfrm_src_hash(net, daddr, saddr, encap_family);
> -			hlist_add_head_rcu(&x->bysrc, net->xfrm.state_bysrc + h);
> +			XFRM_STATE_INSERT(bysrc, &x->bysrc,
> +					  net->xfrm.state_bysrc + h,
> +					  x->xso.type);
>  			if (x->id.spi) {
>  				h = xfrm_spi_hash(net, &x->id.daddr, x->id.spi, x->id.proto, encap_family);
> -				hlist_add_head_rcu(&x->byspi, net->xfrm.state_byspi + h);
> +				XFRM_STATE_INSERT(byspi, &x->byspi,
> +						  net->xfrm.state_byspi + h,
> +						  x->xso.type);
>  			}
>  			if (x->km.seq) {
>  				h = xfrm_seq_hash(net, x->km.seq);
> -				hlist_add_head_rcu(&x->byseq, net->xfrm.state_byseq + h);
> +				XFRM_STATE_INSERT(byseq, &x->byseq,
> +						  net->xfrm.state_byseq + h,
> +						  x->xso.type);
>  			}

This does not work. A larval state will never have a x->xso.type set.
So this raises the question how to handle acquires with this packet
offload. You could place the type and offload device to the template,
but we also have to make sure not to mess too much with the non
offloaded codepath.

This is yet another corner case where the concept of doing policy and
state lookup in software for a HW offload does not work so well. I
fear this is not the last corner case that comes up once you put this
into a real network.

Re: [PATCH xfrm-next v7 7/8] xfrm: add support to HW update soft and hard limits

[Steffen Klassert]

On Wed, Nov 09, 2022 at 02:54:35PM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
> index ce9e360a96e2..819c7cd87d6b 100644
> --- a/net/xfrm/xfrm_output.c
> +++ b/net/xfrm/xfrm_output.c
> @@ -560,7 +560,6 @@ static int xfrm_output_one(struct sk_buff *skb, int err)
>  			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTSTATEPROTOERROR);
>  			goto error_nolock;
>  		}
> -

Nit: No need to remove that empty line.

Re: [PATCH xfrm-next v7 8/8] xfrm: document IPsec packet offload mode

[Steffen Klassert]

On Wed, Nov 09, 2022 at 02:54:36PM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
>  
> +and for packetoffload

Maybe better 'packet offload'

>  
> +Full offload mode:

This is now 'Packet offload mode'

Re: [PATCH xfrm-next v7 0/8] Extend XFRM core to allow packet offload configuration

[Steffen Klassert]

On Wed, Nov 16, 2022 at 03:07:13PM -0800, Saeed Mahameed wrote:
> On 15 Nov 21:00, Leon Romanovsky wrote:
> > On Tue, Nov 15, 2022 at 07:30:20PM +0100, Steffen Klassert wrote:
> > > On Tue, Nov 15, 2022 at 08:09:48PM +0200, Leon Romanovsky wrote:
> > > > On Wed, Nov 09, 2022 at 02:54:28PM +0200, Leon Romanovsky wrote:
> > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > >
> > > > > Changelog:
> > > > > v7: As was discussed in IPsec workshop:
> > > >
> > > > Steffen, can we please merge the series so we won't miss another kernel
> > > > release?
> > > 
> > > I'm already reviewing the patchset. But as I said at the
> > > IPsec workshop, there is no guarantee that it will make
> > > it during this release cycle.
> > 
> 
> BTW mlx5 patches are almost ready and fully reviewed, will be ready by
> early next week.
> 
> Steffen in case you will be ready, how do you want to receive the whole
> package? I guess we will need to post all the patches for a final time,
> including mlx5 driver.

I guess you want me to take both, the xfrm and mlx5 patches, right?
I'm ok with that if nobody else objects to it.

Please split it anyways into a xfrm and a mlx5 pachset. I'll
merge both separate once we are ready.


Thanks!

Re: [PATCH xfrm-next v7 0/8] Extend XFRM core to allow packet offload configuration

[Leon Romanovsky]

On Thu, Nov 17, 2022 at 01:20:43PM +0100, Steffen Klassert wrote:
> On Wed, Nov 16, 2022 at 03:07:13PM -0800, Saeed Mahameed wrote:
> > On 15 Nov 21:00, Leon Romanovsky wrote:
> > > On Tue, Nov 15, 2022 at 07:30:20PM +0100, Steffen Klassert wrote:
> > > > On Tue, Nov 15, 2022 at 08:09:48PM +0200, Leon Romanovsky wrote:
> > > > > On Wed, Nov 09, 2022 at 02:54:28PM +0200, Leon Romanovsky wrote:
> > > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > > >
> > > > > > Changelog:
> > > > > > v7: As was discussed in IPsec workshop:
> > > > >
> > > > > Steffen, can we please merge the series so we won't miss another kernel
> > > > > release?
> > > > 
> > > > I'm already reviewing the patchset. But as I said at the
> > > > IPsec workshop, there is no guarantee that it will make
> > > > it during this release cycle.
> > > 
> > 
> > BTW mlx5 patches are almost ready and fully reviewed, will be ready by
> > early next week.
> > 
> > Steffen in case you will be ready, how do you want to receive the whole
> > package? I guess we will need to post all the patches for a final time,
> > including mlx5 driver.
> 
> I guess you want me to take both, the xfrm and mlx5 patches, right?

Yes

> I'm ok with that if nobody else objects to it.
> 
> Please split it anyways into a xfrm and a mlx5 pachset. I'll
> merge both separate once we are ready.
> 
> 
> Thanks!

Re: [PATCH xfrm-next v7 4/8] xfrm: add TX datapath support for IPsec packet offload mode

[Leon Romanovsky]

On Thu, Nov 17, 2022 at 12:59:39PM +0100, Steffen Klassert wrote:
> On Wed, Nov 09, 2022 at 02:54:32PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> 
> > @@ -2708,6 +2710,23 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
> >  	if (!dev)
> >  		goto free_dst;
> >  
> > +	dst1 = &xdst0->u.dst;
> > +	/* Packet offload: both policy and SA should be offloaded */
> > +	if ((policy->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
> > +	     dst1->xfrm->xso.type != XFRM_DEV_OFFLOAD_PACKET) ||
> > +	    (policy->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
> > +	     dst1->xfrm->xso.type == XFRM_DEV_OFFLOAD_PACKET)) {
> > +		err = -EINVAL;
> > +		goto free_dst;
> > +	}
> > +
> > +	/* Packet offload: both policy and SA should have same device */
> > +	if (policy->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
> > +	    policy->xdo.dev != dst1->xfrm->xso.dev) {
> > +		err = -EINVAL;
> > +		goto free_dst;
> > +	}
> > +
> 
> This is the wrong place for these checks. Things went already wrong
> in the lookup if policy and state do not match here.

Where do you think we should put such checks?

We need to make sure that both policy and SA are offloaded when handle
packet, It prevents various corner cases where we will mix SW and HW
paths.

xfrm_bundle_create() is called when we perform XFRM lookup to create dst_entry.

Thanks

Re: [PATCH xfrm-next v7 7/8] xfrm: add support to HW update soft and hard limits

[Leon Romanovsky]

On Thu, Nov 17, 2022 at 01:13:57PM +0100, Steffen Klassert wrote:
> On Wed, Nov 09, 2022 at 02:54:35PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
> > index ce9e360a96e2..819c7cd87d6b 100644
> > --- a/net/xfrm/xfrm_output.c
> > +++ b/net/xfrm/xfrm_output.c
> > @@ -560,7 +560,6 @@ static int xfrm_output_one(struct sk_buff *skb, int err)
> >  			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTSTATEPROTOERROR);
> >  			goto error_nolock;
> >  		}
> > -
> 
> Nit: No need to remove that empty line.
>

Sure, will fix in v8.

Re: [PATCH xfrm-next v7 8/8] xfrm: document IPsec packet offload mode

[Leon Romanovsky]

On Thu, Nov 17, 2022 at 01:15:44PM +0100, Steffen Klassert wrote:
> On Wed, Nov 09, 2022 at 02:54:36PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> >  
> > +and for packetoffload
> 
> Maybe better 'packet offload'
> 
> >  
> > +Full offload mode:
> 
> This is now 'Packet offload mode'


I'll fix.

Thanks

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Leon Romanovsky]

On Thu, Nov 17, 2022 at 01:12:43PM +0100, Steffen Klassert wrote:
> On Wed, Nov 09, 2022 at 02:54:34PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > @@ -1166,16 +1187,24 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> >  			spin_lock_bh(&net->xfrm.xfrm_state_lock);
> >  			x->km.state = XFRM_STATE_ACQ;
> >  			list_add(&x->km.all, &net->xfrm.state_all);
> > -			hlist_add_head_rcu(&x->bydst, net->xfrm.state_bydst + h);
> > +			XFRM_STATE_INSERT(bydst, &x->bydst,
> > +					  net->xfrm.state_bydst + h,
> > +					  x->xso.type);
> >  			h = xfrm_src_hash(net, daddr, saddr, encap_family);
> > -			hlist_add_head_rcu(&x->bysrc, net->xfrm.state_bysrc + h);
> > +			XFRM_STATE_INSERT(bysrc, &x->bysrc,
> > +					  net->xfrm.state_bysrc + h,
> > +					  x->xso.type);
> >  			if (x->id.spi) {
> >  				h = xfrm_spi_hash(net, &x->id.daddr, x->id.spi, x->id.proto, encap_family);
> > -				hlist_add_head_rcu(&x->byspi, net->xfrm.state_byspi + h);
> > +				XFRM_STATE_INSERT(byspi, &x->byspi,
> > +						  net->xfrm.state_byspi + h,
> > +						  x->xso.type);
> >  			}
> >  			if (x->km.seq) {
> >  				h = xfrm_seq_hash(net, x->km.seq);
> > -				hlist_add_head_rcu(&x->byseq, net->xfrm.state_byseq + h);
> > +				XFRM_STATE_INSERT(byseq, &x->byseq,
> > +						  net->xfrm.state_byseq + h,
> > +						  x->xso.type);
> >  			}
> 
> This does not work. A larval state will never have a x->xso.type set.

x->xso.type always exists. Default is 0, which is XFRM_DEV_OFFLOAD_UNSPECIFIED.
It means this XFRM_STATE_INSERT() will behave exactly as hlist_add_head_rcu() before.

> So this raises the question how to handle acquires with this packet
> offload. 

We handle acquires as SW policies and don't offload them.


> You could place the type and offload device to the template,
> but we also have to make sure not to mess too much with the non
> offloaded codepath.
> 
> This is yet another corner case where the concept of doing policy and
> state lookup in software for a HW offload does not work so well. I
> fear this is not the last corner case that comes up once you put this
> into a real network.
> 

It is not different from any other kernel code, bugs will be fixed.

BTW, IPsec packet offload mode is in use for almost two years already
in real networks.
https://docs.nvidia.com/networking/display/OFEDv521040/Changes+and+New+Features

Thanks

Re: [PATCH xfrm-next v7 4/8] xfrm: add TX datapath support for IPsec packet offload mode

[Steffen Klassert]

On Thu, Nov 17, 2022 at 02:32:10PM +0200, Leon Romanovsky wrote:
> On Thu, Nov 17, 2022 at 12:59:39PM +0100, Steffen Klassert wrote:
> > On Wed, Nov 09, 2022 at 02:54:32PM +0200, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > > @@ -2708,6 +2710,23 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
> > >  	if (!dev)
> > >  		goto free_dst;
> > >  
> > > +	dst1 = &xdst0->u.dst;
> > > +	/* Packet offload: both policy and SA should be offloaded */
> > > +	if ((policy->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
> > > +	     dst1->xfrm->xso.type != XFRM_DEV_OFFLOAD_PACKET) ||
> > > +	    (policy->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
> > > +	     dst1->xfrm->xso.type == XFRM_DEV_OFFLOAD_PACKET)) {
> > > +		err = -EINVAL;
> > > +		goto free_dst;
> > > +	}
> > > +
> > > +	/* Packet offload: both policy and SA should have same device */
> > > +	if (policy->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
> > > +	    policy->xdo.dev != dst1->xfrm->xso.dev) {
> > > +		err = -EINVAL;
> > > +		goto free_dst;
> > > +	}
> > > +
> > 
> > This is the wrong place for these checks. Things went already wrong
> > in the lookup if policy and state do not match here.
> 
> Where do you think we should put such checks?

You need to create a new lookup key for this and match the policies
template against the TS of the state. This happens in xfrm_state_find.
Unfortunately this affects also the SW datapath even without HW
policies/states. So please try to make it a NOP if there are no HW
policies/states.

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Steffen Klassert]

On Thu, Nov 17, 2022 at 02:51:33PM +0200, Leon Romanovsky wrote:
> On Thu, Nov 17, 2022 at 01:12:43PM +0100, Steffen Klassert wrote:
> > On Wed, Nov 09, 2022 at 02:54:34PM +0200, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > This does not work. A larval state will never have a x->xso.type set.
> 
> x->xso.type always exists. Default is 0, which is XFRM_DEV_OFFLOAD_UNSPECIFIED.
> It means this XFRM_STATE_INSERT() will behave exactly as hlist_add_head_rcu() before.

Sure it exists, and is always 0 here.

> 
> > So this raises the question how to handle acquires with this packet
> > offload. 
> 
> We handle acquires as SW policies and don't offload them.

We trigger acquires with states, not policies. The thing is,
we might match a HW policy but create a SW acquire state.
This will not match anymore as soon as the lookup is
implemented correctly.

> > You could place the type and offload device to the template,
> > but we also have to make sure not to mess too much with the non
> > offloaded codepath.
> > 
> > This is yet another corner case where the concept of doing policy and
> > state lookup in software for a HW offload does not work so well. I
> > fear this is not the last corner case that comes up once you put this
> > into a real network.
> > 
> 
> It is not different from any other kernel code, bugs will be fixed.

The thing that is different here is, that the concept is already
broken. We can't split the datapath to be partially handled in
SW and HW in any sane way, this becomes clearer and clearer.

The full protocol offload simply does not fit well into HW,
but we try to make it fit with a hammer. This is the problem
why I do not really like this, and is also the reason why this
is still not merged. We might be much better of by doing a
HW frindly redesign of the protocol and offload this then.
But, yes that takes time and will have issues too.

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Leon Romanovsky]

On Fri, Nov 18, 2022 at 11:49:07AM +0100, Steffen Klassert wrote:
> On Thu, Nov 17, 2022 at 02:51:33PM +0200, Leon Romanovsky wrote:
> > On Thu, Nov 17, 2022 at 01:12:43PM +0100, Steffen Klassert wrote:
> > > On Wed, Nov 09, 2022 at 02:54:34PM +0200, Leon Romanovsky wrote:
> > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > 
> > > This does not work. A larval state will never have a x->xso.type set.
> > 
> > x->xso.type always exists. Default is 0, which is XFRM_DEV_OFFLOAD_UNSPECIFIED.
> > It means this XFRM_STATE_INSERT() will behave exactly as hlist_add_head_rcu() before.
> 
> Sure it exists, and is always 0 here.
> 
> > 
> > > So this raises the question how to handle acquires with this packet
> > > offload. 
> > 
> > We handle acquires as SW policies and don't offload them.
> 
> We trigger acquires with states, not policies. The thing is,
> we might match a HW policy but create a SW acquire state.
> This will not match anymore as soon as the lookup is
> implemented correctly.

For now, all such packets will be dropped as we have offlaoded
policy but not SA.

Like we said in one of our IPsec coffee hours, this flow will be
supported later. Right now, it is not important.

> 
> > > You could place the type and offload device to the template,
> > > but we also have to make sure not to mess too much with the non
> > > offloaded codepath.
> > > 
> > > This is yet another corner case where the concept of doing policy and
> > > state lookup in software for a HW offload does not work so well. I
> > > fear this is not the last corner case that comes up once you put this
> > > into a real network.
> > > 
> > 
> > It is not different from any other kernel code, bugs will be fixed.
> 
> The thing that is different here is, that the concept is already
> broken. We can't split the datapath to be partially handled in
> SW and HW in any sane way, this becomes clearer and clearer.
> 
> The full protocol offload simply does not fit well into HW,
> but we try to make it fit with a hammer. This is the problem
> why I do not really like this, and is also the reason why this
> is still not merged. We might be much better of by doing a
> HW frindly redesign of the protocol and offload this then.
> But, yes that takes time and will have issues too.

When you say "protocol", what do you mean? Many users, who have
deployed IPsec solutions, just want to have same look and feel
but much faster.

I truly believe that this packet offload fits SW model and the
(small) amount of changes supports it. There are almost no changes
to the stack to natively support this offload.

As long as HW involved, you will never have solution without issues,
and like you said even redesign "will have issues".

Plus, we called it packet offload, better and redesigned version will
be called something else. My patches which enhance XFRM to support more
than type of offload are exactly for that.

Thanks 

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Steffen Klassert]

On Sun, Nov 20, 2022 at 09:17:02PM +0200, Leon Romanovsky wrote:
> On Fri, Nov 18, 2022 at 11:49:07AM +0100, Steffen Klassert wrote:
> > On Thu, Nov 17, 2022 at 02:51:33PM +0200, Leon Romanovsky wrote:
> > > On Thu, Nov 17, 2022 at 01:12:43PM +0100, Steffen Klassert wrote:
> > > > On Wed, Nov 09, 2022 at 02:54:34PM +0200, Leon Romanovsky wrote:
> > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > 
> > > > So this raises the question how to handle acquires with this packet
> > > > offload. 
> > > 
> > > We handle acquires as SW policies and don't offload them.
> > 
> > We trigger acquires with states, not policies. The thing is,
> > we might match a HW policy but create a SW acquire state.
> > This will not match anymore as soon as the lookup is
> > implemented correctly.
> 
> For now, all such packets will be dropped as we have offlaoded
> policy but not SA.

I think you missed my point. If the HW policy does not match
the SW acquire state, then each packet will geneate a new
acquire. So you need to make sure that policy and acquire
state will match to send the acquire just once to userspace.

> > > It is not different from any other kernel code, bugs will be fixed.
> > 
> > The thing that is different here is, that the concept is already
> > broken. We can't split the datapath to be partially handled in
> > SW and HW in any sane way, this becomes clearer and clearer.
> > 
> > The full protocol offload simply does not fit well into HW,
> > but we try to make it fit with a hammer. This is the problem
> > why I do not really like this, and is also the reason why this
> > is still not merged. We might be much better of by doing a
> > HW frindly redesign of the protocol and offload this then.
> > But, yes that takes time and will have issues too.
> 
> When you say "protocol", what do you mean? Many users, who have
> deployed IPsec solutions, just want to have same look and feel
> but much faster.
> 
> I truly believe that this packet offload fits SW model and the
> (small) amount of changes supports it. There are almost no changes
> to the stack to natively support this offload.
> 
> As long as HW involved, you will never have solution without issues,
> and like you said even redesign "will have issues".

Things would be much easier, if we don't need to add HW policies
and states to SW databases. But yes, a redesign might have issues
too. That's why we are still working on the current soluion :)

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Leon Romanovsky]

On Mon, Nov 21, 2022 at 10:44:04AM +0100, Steffen Klassert wrote:
> On Sun, Nov 20, 2022 at 09:17:02PM +0200, Leon Romanovsky wrote:
> > On Fri, Nov 18, 2022 at 11:49:07AM +0100, Steffen Klassert wrote:
> > > On Thu, Nov 17, 2022 at 02:51:33PM +0200, Leon Romanovsky wrote:
> > > > On Thu, Nov 17, 2022 at 01:12:43PM +0100, Steffen Klassert wrote:
> > > > > On Wed, Nov 09, 2022 at 02:54:34PM +0200, Leon Romanovsky wrote:
> > > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > 
> > > > > So this raises the question how to handle acquires with this packet
> > > > > offload. 
> > > > 
> > > > We handle acquires as SW policies and don't offload them.
> > > 
> > > We trigger acquires with states, not policies. The thing is,
> > > we might match a HW policy but create a SW acquire state.
> > > This will not match anymore as soon as the lookup is
> > > implemented correctly.
> > 
> > For now, all such packets will be dropped as we have offlaoded
> > policy but not SA.
> 
> I think you missed my point. If the HW policy does not match
> the SW acquire state, then each packet will geneate a new
> acquire. So you need to make sure that policy and acquire
> state will match to send the acquire just once to userspace.

I think that I'm still missing the point.

We require both policy and SA to be offloaded. It means that once
we hit HW policy, we must hit SA too (at least this is how mlx5 part
is implemented).

If it doesn't hit HW policy, it means that whole path is not offloaded,
so no harm will be if we create SW SA for this path.

> 
> > > > It is not different from any other kernel code, bugs will be fixed.
> > > 
> > > The thing that is different here is, that the concept is already
> > > broken. We can't split the datapath to be partially handled in
> > > SW and HW in any sane way, this becomes clearer and clearer.
> > > 
> > > The full protocol offload simply does not fit well into HW,
> > > but we try to make it fit with a hammer. This is the problem
> > > why I do not really like this, and is also the reason why this
> > > is still not merged. We might be much better of by doing a
> > > HW frindly redesign of the protocol and offload this then.
> > > But, yes that takes time and will have issues too.
> > 
> > When you say "protocol", what do you mean? Many users, who have
> > deployed IPsec solutions, just want to have same look and feel
> > but much faster.
> > 
> > I truly believe that this packet offload fits SW model and the
> > (small) amount of changes supports it. There are almost no changes
> > to the stack to natively support this offload.
> > 
> > As long as HW involved, you will never have solution without issues,
> > and like you said even redesign "will have issues".
> 
> Things would be much easier, if we don't need to add HW policies
> and states to SW databases. But yes, a redesign might have issues
> too. That's why we are still working on the current soluion :)
> 

There will be PSP for that, but it is not IPsec.

Thanks

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Steffen Klassert]

On Mon, Nov 21, 2022 at 12:27:01PM +0200, Leon Romanovsky wrote:
> On Mon, Nov 21, 2022 at 10:44:04AM +0100, Steffen Klassert wrote:
> > On Sun, Nov 20, 2022 at 09:17:02PM +0200, Leon Romanovsky wrote:
> > > On Fri, Nov 18, 2022 at 11:49:07AM +0100, Steffen Klassert wrote:
> > > > On Thu, Nov 17, 2022 at 02:51:33PM +0200, Leon Romanovsky wrote:
> > > > > On Thu, Nov 17, 2022 at 01:12:43PM +0100, Steffen Klassert wrote:
> > > > > > On Wed, Nov 09, 2022 at 02:54:34PM +0200, Leon Romanovsky wrote:
> > > > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > > 
> > > > > > So this raises the question how to handle acquires with this packet
> > > > > > offload. 
> > > > > 
> > > > > We handle acquires as SW policies and don't offload them.
> > > > 
> > > > We trigger acquires with states, not policies. The thing is,
> > > > we might match a HW policy but create a SW acquire state.
> > > > This will not match anymore as soon as the lookup is
> > > > implemented correctly.
> > > 
> > > For now, all such packets will be dropped as we have offlaoded
> > > policy but not SA.
> > 
> > I think you missed my point. If the HW policy does not match
> > the SW acquire state, then each packet will geneate a new
> > acquire. So you need to make sure that policy and acquire
> > state will match to send the acquire just once to userspace.
> 
> I think that I'm still missing the point.
> 
> We require both policy and SA to be offloaded. It means that once
> we hit HW policy, we must hit SA too (at least this is how mlx5 part
> is implemented).

Let's assume a packet hits a HW policy. Then this HW policy must match
a HW state. In case there is no matching HW state, we generate an acquire
and insert a larval state. Currently, larval states are never marked as HW.

Now, the next packet from the same flow maches again this HW policy,
but it does not find the larval state because it is not marked as
a HW state. So we generate another acquire and insert another
larval state. Same happens for packets 3,4,5...

Expected behaviour for subsequent packets is that the lookup will
find a matching HW larval state and the packet is dropped without
creating another acquire + larval state for the same flow.

Re: [PATCH xfrm-next v7 4/8] xfrm: add TX datapath support for IPsec packet offload mode

[Leon Romanovsky]

On Fri, Nov 18, 2022 at 11:23:10AM +0100, Steffen Klassert wrote:
> On Thu, Nov 17, 2022 at 02:32:10PM +0200, Leon Romanovsky wrote:
> > On Thu, Nov 17, 2022 at 12:59:39PM +0100, Steffen Klassert wrote:
> > > On Wed, Nov 09, 2022 at 02:54:32PM +0200, Leon Romanovsky wrote:
> > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > 
> > > > @@ -2708,6 +2710,23 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
> > > >  	if (!dev)
> > > >  		goto free_dst;
> > > >  
> > > > +	dst1 = &xdst0->u.dst;
> > > > +	/* Packet offload: both policy and SA should be offloaded */
> > > > +	if ((policy->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
> > > > +	     dst1->xfrm->xso.type != XFRM_DEV_OFFLOAD_PACKET) ||
> > > > +	    (policy->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
> > > > +	     dst1->xfrm->xso.type == XFRM_DEV_OFFLOAD_PACKET)) {
> > > > +		err = -EINVAL;
> > > > +		goto free_dst;
> > > > +	}
> > > > +
> > > > +	/* Packet offload: both policy and SA should have same device */
> > > > +	if (policy->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
> > > > +	    policy->xdo.dev != dst1->xfrm->xso.dev) {
> > > > +		err = -EINVAL;
> > > > +		goto free_dst;
> > > > +	}
> > > > +
> > > 
> > > This is the wrong place for these checks. Things went already wrong
> > > in the lookup if policy and state do not match here.
> > 
> > Where do you think we should put such checks?
> 
> You need to create a new lookup key for this and match the policies
> template against the TS of the state. This happens in xfrm_state_find.
> Unfortunately this affects also the SW datapath even without HW
> policies/states. So please try to make it a NOP if there are no HW
> policies/states.

Do you think that this will be enough?

+static bool xfrm_state_and_policy_mixed(struct xfrm_state *x,
+                                       struct xfrm_policy *p)
+{
+       /* Packet offload: both policy and SA should be offloaded */
+       if (p->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
+           x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
+               return true;
+
+       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
+           x->xso.type == XFRM_DEV_OFFLOAD_PACKET)
+               return true;
+
+       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET)
+               return false;
+
+       /* Packet offload: both policy and SA should have same device */
+       if (p->xdo.dev != x->xso.dev)
+               return true;
+
+       return false;
+}
+
 struct xfrm_state *
 xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
                const struct flowi *fl, struct xfrm_tmpl *tmpl,
@@ -1228,6 +1250,10 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
                        *err = -EAGAIN;
                        x = NULL;
                }
+               if (x && xfrm_state_and_policy_mixed(x, pol)) {
+                       *err = -EINVAL;
+                       x = NULL;
+               }
        } else {
                *err = acquire_in_progress ? -EAGAIN : error;
        }
(END)


> 

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Leon Romanovsky]

On Mon, Nov 21, 2022 at 12:09:26PM +0100, Steffen Klassert wrote:
> On Mon, Nov 21, 2022 at 12:27:01PM +0200, Leon Romanovsky wrote:
> > On Mon, Nov 21, 2022 at 10:44:04AM +0100, Steffen Klassert wrote:
> > > On Sun, Nov 20, 2022 at 09:17:02PM +0200, Leon Romanovsky wrote:
> > > > On Fri, Nov 18, 2022 at 11:49:07AM +0100, Steffen Klassert wrote:
> > > > > On Thu, Nov 17, 2022 at 02:51:33PM +0200, Leon Romanovsky wrote:
> > > > > > On Thu, Nov 17, 2022 at 01:12:43PM +0100, Steffen Klassert wrote:
> > > > > > > On Wed, Nov 09, 2022 at 02:54:34PM +0200, Leon Romanovsky wrote:
> > > > > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > > > 
> > > > > > > So this raises the question how to handle acquires with this packet
> > > > > > > offload. 
> > > > > > 
> > > > > > We handle acquires as SW policies and don't offload them.
> > > > > 
> > > > > We trigger acquires with states, not policies. The thing is,
> > > > > we might match a HW policy but create a SW acquire state.
> > > > > This will not match anymore as soon as the lookup is
> > > > > implemented correctly.
> > > > 
> > > > For now, all such packets will be dropped as we have offlaoded
> > > > policy but not SA.
> > > 
> > > I think you missed my point. If the HW policy does not match
> > > the SW acquire state, then each packet will geneate a new
> > > acquire. So you need to make sure that policy and acquire
> > > state will match to send the acquire just once to userspace.
> > 
> > I think that I'm still missing the point.
> > 
> > We require both policy and SA to be offloaded. It means that once
> > we hit HW policy, we must hit SA too (at least this is how mlx5 part
> > is implemented).
> 
> Let's assume a packet hits a HW policy. Then this HW policy must match
> a HW state. In case there is no matching HW state, we generate an acquire
> and insert a larval state. Currently, larval states are never marked as HW.

And this is there our views are different. If HW (in RX) sees policy but
doesn't have state, this packet will be dropped in HW. It won't get to
stack and no acquire request will be issues.

> 
> Now, the next packet from the same flow maches again this HW policy,
> but it does not find the larval state because it is not marked as
> a HW state. So we generate another acquire and insert another
> larval state. Same happens for packets 3,4,5...
> 
> Expected behaviour for subsequent packets is that the lookup will
> find a matching HW larval state and the packet is dropped without
> creating another acquire + larval state for the same flow.
> 

This is why we don't support acquire for now as it will require mixing
HW and SW paths which we don't want for now.

Thanks

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Steffen Klassert]

On Mon, Nov 21, 2022 at 01:15:36PM +0200, Leon Romanovsky wrote:
> On Mon, Nov 21, 2022 at 12:09:26PM +0100, Steffen Klassert wrote:
> > On Mon, Nov 21, 2022 at 12:27:01PM +0200, Leon Romanovsky wrote:
> > > On Mon, Nov 21, 2022 at 10:44:04AM +0100, Steffen Klassert wrote:
> > > > On Sun, Nov 20, 2022 at 09:17:02PM +0200, Leon Romanovsky wrote:
> > > > > On Fri, Nov 18, 2022 at 11:49:07AM +0100, Steffen Klassert wrote:
> > > > > > On Thu, Nov 17, 2022 at 02:51:33PM +0200, Leon Romanovsky wrote:
> > > > > > > On Thu, Nov 17, 2022 at 01:12:43PM +0100, Steffen Klassert wrote:
> > > > > > > > On Wed, Nov 09, 2022 at 02:54:34PM +0200, Leon Romanovsky wrote:
> > > > > > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > > > > 
> > > > > > > > So this raises the question how to handle acquires with this packet
> > > > > > > > offload. 
> > > > > > > 
> > > > > > > We handle acquires as SW policies and don't offload them.
> > > > > > 
> > > > > > We trigger acquires with states, not policies. The thing is,
> > > > > > we might match a HW policy but create a SW acquire state.
> > > > > > This will not match anymore as soon as the lookup is
> > > > > > implemented correctly.
> > > > > 
> > > > > For now, all such packets will be dropped as we have offlaoded
> > > > > policy but not SA.
> > > > 
> > > > I think you missed my point. If the HW policy does not match
> > > > the SW acquire state, then each packet will geneate a new
> > > > acquire. So you need to make sure that policy and acquire
> > > > state will match to send the acquire just once to userspace.
> > > 
> > > I think that I'm still missing the point.
> > > 
> > > We require both policy and SA to be offloaded. It means that once
> > > we hit HW policy, we must hit SA too (at least this is how mlx5 part
> > > is implemented).
> > 
> > Let's assume a packet hits a HW policy. Then this HW policy must match
> > a HW state. In case there is no matching HW state, we generate an acquire
> > and insert a larval state. Currently, larval states are never marked as HW.
> 
> And this is there our views are different. If HW (in RX) sees policy but
> doesn't have state, this packet will be dropped in HW. It won't get to
> stack and no acquire request will be issues.

This makes no sense. Acquires are always generated at TX, never at RX.

On RX, the state lookup happens first, the policy must match to the
decapsulated packet.

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Leon Romanovsky]

On Mon, Nov 21, 2022 at 12:25:21PM +0100, Steffen Klassert wrote:
> On Mon, Nov 21, 2022 at 01:15:36PM +0200, Leon Romanovsky wrote:
> > On Mon, Nov 21, 2022 at 12:09:26PM +0100, Steffen Klassert wrote:
> > > On Mon, Nov 21, 2022 at 12:27:01PM +0200, Leon Romanovsky wrote:
> > > > On Mon, Nov 21, 2022 at 10:44:04AM +0100, Steffen Klassert wrote:
> > > > > On Sun, Nov 20, 2022 at 09:17:02PM +0200, Leon Romanovsky wrote:
> > > > > > On Fri, Nov 18, 2022 at 11:49:07AM +0100, Steffen Klassert wrote:
> > > > > > > On Thu, Nov 17, 2022 at 02:51:33PM +0200, Leon Romanovsky wrote:
> > > > > > > > On Thu, Nov 17, 2022 at 01:12:43PM +0100, Steffen Klassert wrote:
> > > > > > > > > On Wed, Nov 09, 2022 at 02:54:34PM +0200, Leon Romanovsky wrote:
> > > > > > > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > > > > > 
> > > > > > > > > So this raises the question how to handle acquires with this packet
> > > > > > > > > offload. 
> > > > > > > > 
> > > > > > > > We handle acquires as SW policies and don't offload them.
> > > > > > > 
> > > > > > > We trigger acquires with states, not policies. The thing is,
> > > > > > > we might match a HW policy but create a SW acquire state.
> > > > > > > This will not match anymore as soon as the lookup is
> > > > > > > implemented correctly.
> > > > > > 
> > > > > > For now, all such packets will be dropped as we have offlaoded
> > > > > > policy but not SA.
> > > > > 
> > > > > I think you missed my point. If the HW policy does not match
> > > > > the SW acquire state, then each packet will geneate a new
> > > > > acquire. So you need to make sure that policy and acquire
> > > > > state will match to send the acquire just once to userspace.
> > > > 
> > > > I think that I'm still missing the point.
> > > > 
> > > > We require both policy and SA to be offloaded. It means that once
> > > > we hit HW policy, we must hit SA too (at least this is how mlx5 part
> > > > is implemented).
> > > 
> > > Let's assume a packet hits a HW policy. Then this HW policy must match
> > > a HW state. In case there is no matching HW state, we generate an acquire
> > > and insert a larval state. Currently, larval states are never marked as HW.
> > 
> > And this is there our views are different. If HW (in RX) sees policy but
> > doesn't have state, this packet will be dropped in HW. It won't get to
> > stack and no acquire request will be issues.
> 
> This makes no sense. Acquires are always generated at TX, never at RX.

Sorry, my bad. But why can't we drop all packets that don't have HW
state? Why do we need to add larval?

> 
> On RX, the state lookup happens first, the policy must match to the
> decapsulated packet.
> 

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Leon Romanovsky]

On Mon, Nov 21, 2022 at 01:34:30PM +0200, Leon Romanovsky wrote:
> On Mon, Nov 21, 2022 at 12:25:21PM +0100, Steffen Klassert wrote:
> > On Mon, Nov 21, 2022 at 01:15:36PM +0200, Leon Romanovsky wrote:
> > > On Mon, Nov 21, 2022 at 12:09:26PM +0100, Steffen Klassert wrote:
> > > > On Mon, Nov 21, 2022 at 12:27:01PM +0200, Leon Romanovsky wrote:
> > > > > On Mon, Nov 21, 2022 at 10:44:04AM +0100, Steffen Klassert wrote:
> > > > > > On Sun, Nov 20, 2022 at 09:17:02PM +0200, Leon Romanovsky wrote:
> > > > > > > On Fri, Nov 18, 2022 at 11:49:07AM +0100, Steffen Klassert wrote:
> > > > > > > > On Thu, Nov 17, 2022 at 02:51:33PM +0200, Leon Romanovsky wrote:
> > > > > > > > > On Thu, Nov 17, 2022 at 01:12:43PM +0100, Steffen Klassert wrote:
> > > > > > > > > > On Wed, Nov 09, 2022 at 02:54:34PM +0200, Leon Romanovsky wrote:
> > > > > > > > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > > > > > > 
> > > > > > > > > > So this raises the question how to handle acquires with this packet
> > > > > > > > > > offload. 
> > > > > > > > > 
> > > > > > > > > We handle acquires as SW policies and don't offload them.
> > > > > > > > 
> > > > > > > > We trigger acquires with states, not policies. The thing is,
> > > > > > > > we might match a HW policy but create a SW acquire state.
> > > > > > > > This will not match anymore as soon as the lookup is
> > > > > > > > implemented correctly.
> > > > > > > 
> > > > > > > For now, all such packets will be dropped as we have offlaoded
> > > > > > > policy but not SA.
> > > > > > 
> > > > > > I think you missed my point. If the HW policy does not match
> > > > > > the SW acquire state, then each packet will geneate a new
> > > > > > acquire. So you need to make sure that policy and acquire
> > > > > > state will match to send the acquire just once to userspace.
> > > > > 
> > > > > I think that I'm still missing the point.
> > > > > 
> > > > > We require both policy and SA to be offloaded. It means that once
> > > > > we hit HW policy, we must hit SA too (at least this is how mlx5 part
> > > > > is implemented).
> > > > 
> > > > Let's assume a packet hits a HW policy. Then this HW policy must match
> > > > a HW state. In case there is no matching HW state, we generate an acquire
> > > > and insert a larval state. Currently, larval states are never marked as HW.
> > > 
> > > And this is there our views are different. If HW (in RX) sees policy but
> > > doesn't have state, this packet will be dropped in HW. It won't get to
> > > stack and no acquire request will be issues.
> > 
> > This makes no sense. Acquires are always generated at TX, never at RX.
> 
> Sorry, my bad. But why can't we drop all packets that don't have HW
> state? Why do we need to add larval?

I think that something like this will do the trick.

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 5076f9d7a752..d1c9ef857755 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1090,6 +1090,28 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
        }
 }

+static bool xfrm_state_and_policy_mixed(struct xfrm_state *x,
+                                       struct xfrm_policy *p)
+{
+       /* Packet offload: both policy and SA should be offloaded */
+       if (p->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
+           x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
+               return true;
+
+       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
+           x->xso.type == XFRM_DEV_OFFLOAD_PACKET)
+               return true;
+
+       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET)
+               return false;
+
+       /* Packet offload: both policy and SA should have same device */
+       if (p->xdo.dev != x->xso.dev)
+               return true;
+
+       return false;
+}
+
 struct xfrm_state *
 xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
                const struct flowi *fl, struct xfrm_tmpl *tmpl,
@@ -1147,7 +1169,8 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,

 found:
        x = best;
-       if (!x && !error && !acquire_in_progress) {
+       if (!x && !error && !acquire_in_progress &&
+           pol->xdo.type != XFRM_DEV_OFFLOAD_PACKET) {
                if (tmpl->id.spi &&
                    (x0 = __xfrm_state_lookup(net, mark, daddr, tmpl->id.spi,
                                              tmpl->id.proto, encap_family)) != NULL) {
@@ -1228,7 +1251,14 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
                        *err = -EAGAIN;
                        x = NULL;
                }
+               if (x && xfrm_state_and_policy_mixed(x, pol)) {
+                       *err = -EINVAL;
+                       x = NULL;
+               }
        } else {
+               if (pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET)
+                       error = -EINVAL;
+
                *err = acquire_in_progress ? -EAGAIN : error;
        }
        rcu_read_unlock();
(END)


> 
> > 
> > On RX, the state lookup happens first, the policy must match to the
> > decapsulated packet.
> > 

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Steffen Klassert]

On Mon, Nov 21, 2022 at 01:34:30PM +0200, Leon Romanovsky wrote:
> 
> Sorry, my bad. But why can't we drop all packets that don't have HW
> state? Why do we need to add larval?

The first packet of a flow tiggers an acquire and inserts a larval
state. On a traffic triggered connection, we need this to get
a state with keys installed.

We need this larval state then, because that tells us we sent already an
acquire to userspace. All subsequent packets of that flow will be
dropped without sending another acquire. Otherwise each subsequent
packet will generate another acquire until the keys are negotiated.
If a flow starts sending on a high rate, this would be not so nice
for userspace :)

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Steffen Klassert]

On Mon, Nov 21, 2022 at 02:02:52PM +0200, Leon Romanovsky wrote:
> 
> I think that something like this will do the trick.
> 
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 5076f9d7a752..d1c9ef857755 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1090,6 +1090,28 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
>         }
>  }
> 
> +static bool xfrm_state_and_policy_mixed(struct xfrm_state *x,
> +                                       struct xfrm_policy *p)
> +{
> +       /* Packet offload: both policy and SA should be offloaded */
> +       if (p->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
> +           x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
> +               return true;
> +
> +       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
> +           x->xso.type == XFRM_DEV_OFFLOAD_PACKET)
> +               return true;
> +
> +       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET)
> +               return false;
> +
> +       /* Packet offload: both policy and SA should have same device */
> +       if (p->xdo.dev != x->xso.dev)
> +               return true;
> +
> +       return false;
> +}
> +
>  struct xfrm_state *
>  xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
>                 const struct flowi *fl, struct xfrm_tmpl *tmpl,
> @@ -1147,7 +1169,8 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> 
>  found:
>         x = best;
> -       if (!x && !error && !acquire_in_progress) {
> +       if (!x && !error && !acquire_in_progress &&
> +           pol->xdo.type != XFRM_DEV_OFFLOAD_PACKET) {
>                 if (tmpl->id.spi &&
>                     (x0 = __xfrm_state_lookup(net, mark, daddr, tmpl->id.spi,
>                                               tmpl->id.proto, encap_family)) != NULL) {
> @@ -1228,7 +1251,14 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
>                         *err = -EAGAIN;
>                         x = NULL;
>                 }
> +               if (x && xfrm_state_and_policy_mixed(x, pol)) {
> +                       *err = -EINVAL;
> +                       x = NULL;

If policy and state do not match here, this means the lookup
returned the wrong state. The correct state might still sit
in the database. At this point, you should either have found
a matching state, or no state at all.

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Leon Romanovsky]

On Mon, Nov 21, 2022 at 01:43:49PM +0100, Steffen Klassert wrote:
> On Mon, Nov 21, 2022 at 02:02:52PM +0200, Leon Romanovsky wrote:
> > 
> > I think that something like this will do the trick.
> > 
> > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > index 5076f9d7a752..d1c9ef857755 100644
> > --- a/net/xfrm/xfrm_state.c
> > +++ b/net/xfrm/xfrm_state.c
> > @@ -1090,6 +1090,28 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
> >         }
> >  }
> > 
> > +static bool xfrm_state_and_policy_mixed(struct xfrm_state *x,
> > +                                       struct xfrm_policy *p)
> > +{
> > +       /* Packet offload: both policy and SA should be offloaded */
> > +       if (p->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
> > +           x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
> > +               return true;
> > +
> > +       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
> > +           x->xso.type == XFRM_DEV_OFFLOAD_PACKET)
> > +               return true;
> > +
> > +       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET)
> > +               return false;
> > +
> > +       /* Packet offload: both policy and SA should have same device */
> > +       if (p->xdo.dev != x->xso.dev)
> > +               return true;
> > +
> > +       return false;
> > +}
> > +
> >  struct xfrm_state *
> >  xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> >                 const struct flowi *fl, struct xfrm_tmpl *tmpl,
> > @@ -1147,7 +1169,8 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> > 
> >  found:
> >         x = best;
> > -       if (!x && !error && !acquire_in_progress) {
> > +       if (!x && !error && !acquire_in_progress &&
> > +           pol->xdo.type != XFRM_DEV_OFFLOAD_PACKET) {
> >                 if (tmpl->id.spi &&
> >                     (x0 = __xfrm_state_lookup(net, mark, daddr, tmpl->id.spi,
> >                                               tmpl->id.proto, encap_family)) != NULL) {
> > @@ -1228,7 +1251,14 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> >                         *err = -EAGAIN;
> >                         x = NULL;
> >                 }
> > +               if (x && xfrm_state_and_policy_mixed(x, pol)) {
> > +                       *err = -EINVAL;
> > +                       x = NULL;
> 
> If policy and state do not match here, this means the lookup
> returned the wrong state. The correct state might still sit
> in the database. At this point, you should either have found
> a matching state, or no state at all.

I check for "x" because of "x = NULL" above.

Thanks

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Leon Romanovsky]

On Mon, Nov 21, 2022 at 01:10:40PM +0100, Steffen Klassert wrote:
> On Mon, Nov 21, 2022 at 01:34:30PM +0200, Leon Romanovsky wrote:
> > 
> > Sorry, my bad. But why can't we drop all packets that don't have HW
> > state? Why do we need to add larval?
> 
> The first packet of a flow tiggers an acquire and inserts a larval
> state. On a traffic triggered connection, we need this to get
> a state with keys installed.
> 
> We need this larval state then, because that tells us we sent already an
> acquire to userspace. All subsequent packets of that flow will be
> dropped without sending another acquire. Otherwise each subsequent
> packet will generate another acquire until the keys are negotiated.
> If a flow starts sending on a high rate, this would be not so nice
> for userspace :)

The thing is that this SW acquire flow is a fraction case, as it applies
to locally generated traffic.

In my mind, there are other cases, like eswitch mode and tunnel mode. In
these cases, the packets are arrived to HW without even passing SW stack.

What we want to do is to catch in HW all TX packets, which don't have SAs
(applicable for all types of traffic), mark them and route back to the
driver. The driver will be responsible to talk with XFRM core to
generate acquire.

The same logic of rerouting packets is required for audit and will be done
later. Right now, we rely on *swan implementations which configure everything
in advance.

Also larval is default to 1 (drop) in all distros.

I hope that this larval/acquire is not must for this series to be merged.
And it is going to be implemented later as I'm assigned to work on this
offload feature till feature complete :).

Thanks

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Herbert Xu]

On Mon, Nov 21, 2022 at 03:21:45PM +0200, Leon Romanovsky wrote:
>
> The thing is that this SW acquire flow is a fraction case, as it applies
> to locally generated traffic.

A router can trigger an acquire on forwarded packets too.  Without
larvals this could quickly overwhelm the router.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Leon Romanovsky]

On Tue, Nov 22, 2022 at 12:29:12PM +0800, Herbert Xu wrote:
> On Mon, Nov 21, 2022 at 03:21:45PM +0200, Leon Romanovsky wrote:
> >
> > The thing is that this SW acquire flow is a fraction case, as it applies
> > to locally generated traffic.
> 
> A router can trigger an acquire on forwarded packets too.  Without
> larvals this could quickly overwhelm the router.

This series doesn't support tunnel mode yet.

Maybe I was not clear, but I wanted to say what in eswitch case and
tunnel mode, the packets will be handled purely by HW without raising
into SW core.

It is so called transparent IPsec, where all configuration is done on
hypervisor, so VMs connected through eswitch will get already decrypted
traffic which is routed through eswitch NIC logic without passing
hypervisor data path.

Steffen expected to see changes to acquire logic as part of this series
and in my explanation, I tried to explain why it is not needed now and
how will it be implemented later.

Thanks

> 
> Cheers,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Steffen Klassert]

On Tue, Nov 22, 2022 at 08:27:48AM +0200, Leon Romanovsky wrote:
> On Tue, Nov 22, 2022 at 12:29:12PM +0800, Herbert Xu wrote:
> > On Mon, Nov 21, 2022 at 03:21:45PM +0200, Leon Romanovsky wrote:
> > >
> > > The thing is that this SW acquire flow is a fraction case, as it applies
> > > to locally generated traffic.
> > 
> > A router can trigger an acquire on forwarded packets too.  Without
> > larvals this could quickly overwhelm the router.
> 
> This series doesn't support tunnel mode yet.

It does not matter if tunnel or transport mode, acquires must
work as expected. This is a fundamental concept of IPsec, there
is no way to tell userspace that we don't support this.

> Maybe I was not clear, but I wanted to say what in eswitch case and
> tunnel mode, the packets will be handled purely by HW without raising
> into SW core.

Can you please explain why we need host interaction for
transport, but not for tunnel mode?

Staying away with HW policies and states from SW databases is what
I wanted to have from the beginning. If that is possible for tunnel
mode, it should be possible for transport mode too.

And as said already, I want to see the full picture (transport
+ tunnel mode) before we merge it.

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Steffen Klassert]

On Mon, Nov 21, 2022 at 03:01:42PM +0200, Leon Romanovsky wrote:
> On Mon, Nov 21, 2022 at 01:43:49PM +0100, Steffen Klassert wrote:
> > On Mon, Nov 21, 2022 at 02:02:52PM +0200, Leon Romanovsky wrote:
> > > 
> > > I think that something like this will do the trick.
> > > 
> > > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > > index 5076f9d7a752..d1c9ef857755 100644
> > > --- a/net/xfrm/xfrm_state.c
> > > +++ b/net/xfrm/xfrm_state.c
> > > @@ -1090,6 +1090,28 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
> > >         }
> > >  }
> > > 
> > > +static bool xfrm_state_and_policy_mixed(struct xfrm_state *x,
> > > +                                       struct xfrm_policy *p)
> > > +{
> > > +       /* Packet offload: both policy and SA should be offloaded */
> > > +       if (p->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
> > > +           x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
> > > +               return true;
> > > +
> > > +       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
> > > +           x->xso.type == XFRM_DEV_OFFLOAD_PACKET)
> > > +               return true;
> > > +
> > > +       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET)
> > > +               return false;
> > > +
> > > +       /* Packet offload: both policy and SA should have same device */
> > > +       if (p->xdo.dev != x->xso.dev)
> > > +               return true;
> > > +
> > > +       return false;
> > > +}
> > > +
> > >  struct xfrm_state *
> > >  xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> > >                 const struct flowi *fl, struct xfrm_tmpl *tmpl,
> > > @@ -1147,7 +1169,8 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> > > 
> > >  found:
> > >         x = best;
> > > -       if (!x && !error && !acquire_in_progress) {
> > > +       if (!x && !error && !acquire_in_progress &&
> > > +           pol->xdo.type != XFRM_DEV_OFFLOAD_PACKET) {
> > >                 if (tmpl->id.spi &&
> > >                     (x0 = __xfrm_state_lookup(net, mark, daddr, tmpl->id.spi,
> > >                                               tmpl->id.proto, encap_family)) != NULL) {
> > > @@ -1228,7 +1251,14 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> > >                         *err = -EAGAIN;
> > >                         x = NULL;
> > >                 }
> > > +               if (x && xfrm_state_and_policy_mixed(x, pol)) {
> > > +                       *err = -EINVAL;
> > > +                       x = NULL;
> > 
> > If policy and state do not match here, this means the lookup
> > returned the wrong state. The correct state might still sit
> > in the database. At this point, you should either have found
> > a matching state, or no state at all.
> 
> I check for "x" because of "x = NULL" above.

This does not change the fact that the lookup returned the wrong state.

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Leon Romanovsky]

On Tue, Nov 22, 2022 at 02:00:02PM +0100, Steffen Klassert wrote:
> On Tue, Nov 22, 2022 at 08:27:48AM +0200, Leon Romanovsky wrote:
> > On Tue, Nov 22, 2022 at 12:29:12PM +0800, Herbert Xu wrote:
> > > On Mon, Nov 21, 2022 at 03:21:45PM +0200, Leon Romanovsky wrote:
> > > >
> > > > The thing is that this SW acquire flow is a fraction case, as it applies
> > > > to locally generated traffic.
> > > 
> > > A router can trigger an acquire on forwarded packets too.  Without
> > > larvals this could quickly overwhelm the router.
> > 
> > This series doesn't support tunnel mode yet.
> 
> It does not matter if tunnel or transport mode, acquires must
> work as expected. This is a fundamental concept of IPsec, there
> is no way to tell userspace that we don't support this.
> 
> > Maybe I was not clear, but I wanted to say what in eswitch case and
> > tunnel mode, the packets will be handled purely by HW without raising
> > into SW core.
> 
> Can you please explain why we need host interaction for
> transport, but not for tunnel mode?

The main difference is that in transport mode, you must bring packet
to the kernel in which you configured SA/policy. It means that we must
ensure that such packets won't be checked again in SW because all packets
(encrypted and not) pass XFRM logic.

 - wire -> RX NIC -> kernel -> XFRM stack (we need HW DB here to skip this stage) -> ....
 ... -> kernel -> XFRM stack (skip for HW SA/policies) -> TX NIC -> wire.

In tunnel mode, we arrive to XFRM when nothing IPsec related is configured.

 - wire -> RX PF NIC -> eswitch NIC logic -> TX uplink NIC -> RX
   representors -> XFRM stack in VM (nothing configured here) -> kernel

The most troublesome part is in TX path, where you must skip "double check"
before NIC. This check doesn't exist in tunnel mode.

In RX, there is also difference between modes due to how we are supposed
to treat headers.

Raed will add more details.

> 
> And as said already, I want to see the full picture (transport
> + tunnel mode) before we merge it.

It looks like we already have this picture.

Thanks

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Leon Romanovsky]

On Tue, Nov 22, 2022 at 02:10:02PM +0100, Steffen Klassert wrote:
> On Mon, Nov 21, 2022 at 03:01:42PM +0200, Leon Romanovsky wrote:
> > On Mon, Nov 21, 2022 at 01:43:49PM +0100, Steffen Klassert wrote:
> > > On Mon, Nov 21, 2022 at 02:02:52PM +0200, Leon Romanovsky wrote:
> > > > 
> > > > I think that something like this will do the trick.
> > > > 
> > > > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > > > index 5076f9d7a752..d1c9ef857755 100644
> > > > --- a/net/xfrm/xfrm_state.c
> > > > +++ b/net/xfrm/xfrm_state.c
> > > > @@ -1090,6 +1090,28 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
> > > >         }
> > > >  }
> > > > 
> > > > +static bool xfrm_state_and_policy_mixed(struct xfrm_state *x,
> > > > +                                       struct xfrm_policy *p)
> > > > +{
> > > > +       /* Packet offload: both policy and SA should be offloaded */
> > > > +       if (p->xdo.type == XFRM_DEV_OFFLOAD_PACKET &&
> > > > +           x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
> > > > +               return true;
> > > > +
> > > > +       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET &&
> > > > +           x->xso.type == XFRM_DEV_OFFLOAD_PACKET)
> > > > +               return true;
> > > > +
> > > > +       if (p->xdo.type != XFRM_DEV_OFFLOAD_PACKET)
> > > > +               return false;
> > > > +
> > > > +       /* Packet offload: both policy and SA should have same device */
> > > > +       if (p->xdo.dev != x->xso.dev)
> > > > +               return true;
> > > > +
> > > > +       return false;
> > > > +}
> > > > +
> > > >  struct xfrm_state *
> > > >  xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> > > >                 const struct flowi *fl, struct xfrm_tmpl *tmpl,
> > > > @@ -1147,7 +1169,8 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> > > > 
> > > >  found:
> > > >         x = best;
> > > > -       if (!x && !error && !acquire_in_progress) {
> > > > +       if (!x && !error && !acquire_in_progress &&
> > > > +           pol->xdo.type != XFRM_DEV_OFFLOAD_PACKET) {
> > > >                 if (tmpl->id.spi &&
> > > >                     (x0 = __xfrm_state_lookup(net, mark, daddr, tmpl->id.spi,
> > > >                                               tmpl->id.proto, encap_family)) != NULL) {
> > > > @@ -1228,7 +1251,14 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> > > >                         *err = -EAGAIN;
> > > >                         x = NULL;
> > > >                 }
> > > > +               if (x && xfrm_state_and_policy_mixed(x, pol)) {
> > > > +                       *err = -EINVAL;
> > > > +                       x = NULL;
> > > 
> > > If policy and state do not match here, this means the lookup
> > > returned the wrong state. The correct state might still sit
> > > in the database. At this point, you should either have found
> > > a matching state, or no state at all.
> > 
> > I check for "x" because of "x = NULL" above.
> 
> This does not change the fact that the lookup returned the wrong state.

Steffen, but this is exactly why we added this check - to catch wrong
states and configurations. 

If lookup was successful, we know that HW handles this packet, if lookup
failed to find SA and we have HW policy, we should drop such packet.

Thanks

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Steffen Klassert]

On Tue, Nov 22, 2022 at 03:54:42PM +0200, Leon Romanovsky wrote:
> On Tue, Nov 22, 2022 at 02:00:02PM +0100, Steffen Klassert wrote:
> > On Tue, Nov 22, 2022 at 08:27:48AM +0200, Leon Romanovsky wrote:
> > > On Tue, Nov 22, 2022 at 12:29:12PM +0800, Herbert Xu wrote:
> > > > On Mon, Nov 21, 2022 at 03:21:45PM +0200, Leon Romanovsky wrote:
> > 
> > Can you please explain why we need host interaction for
> > transport, but not for tunnel mode?
> 
> The main difference is that in transport mode, you must bring packet
> to the kernel in which you configured SA/policy. It means that we must
> ensure that such packets won't be checked again in SW because all packets
> (encrypted and not) pass XFRM logic.
> 
>  - wire -> RX NIC -> kernel -> XFRM stack (we need HW DB here to skip this stage) -> ....
>  ... -> kernel -> XFRM stack (skip for HW SA/policies) -> TX NIC -> wire.
> 
> In tunnel mode, we arrive to XFRM when nothing IPsec related is configured.
> 
>  - wire -> RX PF NIC -> eswitch NIC logic -> TX uplink NIC -> RX
>    representors -> XFRM stack in VM (nothing configured here) -> kernel

Forget about eswitch, VM, etc. for a moment. I'm interested how the
simplest possible tunnel mode cases will work.

Forwarding:

wire -> random NIC RX -> kernel -> IPsec tunnel offload NIC TX -> wire
wire -> IPsec tunnel offload NIC RX -> kernel -> random NIC TX -> wire

Local endpoints:

Application -> kernel -> IPsec tunnel offload NIC TX -> wire
wire -> IPsec tunnel offload NIC RX -> kernel -> Application

These two must work, so how are these cases handled?

If you can do more fancy things with tunnel mode and special NICs
at TX and RX, that's fine but not absolutely required.

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Steffen Klassert]

On Tue, Nov 22, 2022 at 03:57:43PM +0200, Leon Romanovsky wrote:
> On Tue, Nov 22, 2022 at 02:10:02PM +0100, Steffen Klassert wrote:
> > On Mon, Nov 21, 2022 at 03:01:42PM +0200, Leon Romanovsky wrote:
> > > On Mon, Nov 21, 2022 at 01:43:49PM +0100, Steffen Klassert wrote:
> > > > On Mon, Nov 21, 2022 at 02:02:52PM +0200, Leon Romanovsky wrote:
> > > > 
> > > > If policy and state do not match here, this means the lookup
> > > > returned the wrong state. The correct state might still sit
> > > > in the database. At this point, you should either have found
> > > > a matching state, or no state at all.
> > > 
> > > I check for "x" because of "x = NULL" above.
> > 
> > This does not change the fact that the lookup returned the wrong state.
> 
> Steffen, but this is exactly why we added this check - to catch wrong
> states and configurations. 

No, you have to adjust the lookup so that this can't happen.
This is not a missconfiguration, The lookup found the wrong
SA, this is a difference.

Use the offload type and dev as a lookup key and don't consider
SAs that don't match this in the lookup.

This is really not too hard to do. The thing that could be a bit
more difficult is that the lookup should be only adjusted when
we really have HW policies installed. Otherwise this affects
even systems that don't use this kind of offload.

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Leon Romanovsky]

On Wed, Nov 23, 2022 at 09:37:20AM +0100, Steffen Klassert wrote:
> On Tue, Nov 22, 2022 at 03:57:43PM +0200, Leon Romanovsky wrote:
> > On Tue, Nov 22, 2022 at 02:10:02PM +0100, Steffen Klassert wrote:
> > > On Mon, Nov 21, 2022 at 03:01:42PM +0200, Leon Romanovsky wrote:
> > > > On Mon, Nov 21, 2022 at 01:43:49PM +0100, Steffen Klassert wrote:
> > > > > On Mon, Nov 21, 2022 at 02:02:52PM +0200, Leon Romanovsky wrote:
> > > > > 
> > > > > If policy and state do not match here, this means the lookup
> > > > > returned the wrong state. The correct state might still sit
> > > > > in the database. At this point, you should either have found
> > > > > a matching state, or no state at all.
> > > > 
> > > > I check for "x" because of "x = NULL" above.
> > > 
> > > This does not change the fact that the lookup returned the wrong state.
> > 
> > Steffen, but this is exactly why we added this check - to catch wrong
> > states and configurations. 
> 
> No, you have to adjust the lookup so that this can't happen.
> This is not a missconfiguration, The lookup found the wrong
> SA, this is a difference.
> 
> Use the offload type and dev as a lookup key and don't consider
> SAs that don't match this in the lookup.
> 
> This is really not too hard to do. The thing that could be a bit
> more difficult is that the lookup should be only adjusted when
> we really have HW policies installed. Otherwise this affects
> even systems that don't use this kind of offload.

Thanks for an explanation, trying it now.

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Leon Romanovsky]

On Wed, Nov 23, 2022 at 09:23:58AM +0100, Steffen Klassert wrote:
> On Tue, Nov 22, 2022 at 03:54:42PM +0200, Leon Romanovsky wrote:
> > On Tue, Nov 22, 2022 at 02:00:02PM +0100, Steffen Klassert wrote:
> > > On Tue, Nov 22, 2022 at 08:27:48AM +0200, Leon Romanovsky wrote:
> > > > On Tue, Nov 22, 2022 at 12:29:12PM +0800, Herbert Xu wrote:
> > > > > On Mon, Nov 21, 2022 at 03:21:45PM +0200, Leon Romanovsky wrote:
> > > 
> > > Can you please explain why we need host interaction for
> > > transport, but not for tunnel mode?
> > 
> > The main difference is that in transport mode, you must bring packet
> > to the kernel in which you configured SA/policy. It means that we must
> > ensure that such packets won't be checked again in SW because all packets
> > (encrypted and not) pass XFRM logic.
> > 
> >  - wire -> RX NIC -> kernel -> XFRM stack (we need HW DB here to skip this stage) -> ....
> >  ... -> kernel -> XFRM stack (skip for HW SA/policies) -> TX NIC -> wire.
> > 
> > In tunnel mode, we arrive to XFRM when nothing IPsec related is configured.
> > 
> >  - wire -> RX PF NIC -> eswitch NIC logic -> TX uplink NIC -> RX
> >    representors -> XFRM stack in VM (nothing configured here) -> kernel
> 
> Forget about eswitch, VM, etc. for a moment. I'm interested how the
> simplest possible tunnel mode cases will work.
> 
> Forwarding:
> 
> wire -> random NIC RX -> kernel -> IPsec tunnel offload NIC TX -> wire
> wire -> IPsec tunnel offload NIC RX -> kernel -> random NIC TX -> wire
> 
> Local endpoints:
> 
> Application -> kernel -> IPsec tunnel offload NIC TX -> wire
> wire -> IPsec tunnel offload NIC RX -> kernel -> Application
> 
> These two must work, so how are these cases handled?

These two cases conceptually no different from transport modes.
The difference is how HW handles IP packets.

If packet comes from RX, it will be received as plain packet in the
kernel. If packet goes to TX, it must be skipped in the XFRM.

For all "wire -> IPsec tunnel offload NIC RX ...", everything works
as you would expect. HW handles everything, and feeds the kernel with
plain packet. These packets will have CRYPTO_DONE and status so they
can skip all XFRM logic.

All this complexity is For "... kernel -> IPsec tunnel offload NIC TX -> wire"
flow. You need a way to say to the kernel that XFRM should be skipped.


In TX path, we will need to perform neighbor resolution to fill proper
MAC address for outer IP header.
In RX path, once the packet is decrypted, there is a need to change MAC
address for the inner IP header. This will be done by kernel as HW
doesn't have such knowledge.

Of course, there are many possible implementations of how to have right
MAC address (static during SA creations, or dynamic if we listen to ARP
events), but it is not XFRM related.

Thanks

> 
> If you can do more fancy things with tunnel mode and special NICs
> at TX and RX, that's fine but not absolutely required.

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Leon Romanovsky]

On Wed, Nov 23, 2022 at 11:36:19AM +0200, Leon Romanovsky wrote:
> On Wed, Nov 23, 2022 at 09:37:20AM +0100, Steffen Klassert wrote:
> > On Tue, Nov 22, 2022 at 03:57:43PM +0200, Leon Romanovsky wrote:
> > > On Tue, Nov 22, 2022 at 02:10:02PM +0100, Steffen Klassert wrote:
> > > > On Mon, Nov 21, 2022 at 03:01:42PM +0200, Leon Romanovsky wrote:
> > > > > On Mon, Nov 21, 2022 at 01:43:49PM +0100, Steffen Klassert wrote:
> > > > > > On Mon, Nov 21, 2022 at 02:02:52PM +0200, Leon Romanovsky wrote:
> > > > > > 
> > > > > > If policy and state do not match here, this means the lookup
> > > > > > returned the wrong state. The correct state might still sit
> > > > > > in the database. At this point, you should either have found
> > > > > > a matching state, or no state at all.
> > > > > 
> > > > > I check for "x" because of "x = NULL" above.
> > > > 
> > > > This does not change the fact that the lookup returned the wrong state.
> > > 
> > > Steffen, but this is exactly why we added this check - to catch wrong
> > > states and configurations. 
> > 
> > No, you have to adjust the lookup so that this can't happen.
> > This is not a missconfiguration, The lookup found the wrong
> > SA, this is a difference.
> > 
> > Use the offload type and dev as a lookup key and don't consider
> > SAs that don't match this in the lookup.
> > 
> > This is really not too hard to do. The thing that could be a bit
> > more difficult is that the lookup should be only adjusted when
> > we really have HW policies installed. Otherwise this affects
> > even systems that don't use this kind of offload.
> 
> Thanks for an explanation, trying it now.

Something like that?

The code is untested yet.

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 5076f9d7a752..5819023c32ba 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1115,6 +1115,19 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 	rcu_read_lock();
 	h = xfrm_dst_hash(net, daddr, saddr, tmpl->reqid, encap_family);
 	hlist_for_each_entry_rcu(x, net->xfrm.state_bydst + h, bydst) {
+		if (IS_ENABLED(CONFIG_XFRM_OFFLOAD) &&
+		    pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {
+			if (x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
+				/* HW states are in the head of list, there is no need
+				 * to iterate further.
+				 */
+				break;
+
+			/* Packet offload: both policy and SA should have same device */
+			if (pol->xdo.dev != x->xso.dev)
+				continue;
+		}
+
 		if (x->props.family == encap_family &&
 		    x->props.reqid == tmpl->reqid &&
 		    (mark & x->mark.m) == x->mark.v &&
@@ -1132,6 +1145,19 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 
 	h_wildcard = xfrm_dst_hash(net, daddr, &saddr_wildcard, tmpl->reqid, encap_family);
 	hlist_for_each_entry_rcu(x, net->xfrm.state_bydst + h_wildcard, bydst) {
+		if (IS_ENABLED(CONFIG_XFRM_OFFLOAD) &&
+		    pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {
+			if (x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
+				/* HW states are in the head of list, there is no need
+				 * to iterate further.
+				 */
+				break;
+
+			/* Packet offload: both policy and SA should have same device */
+			if (pol->xdo.dev != x->xso.dev)
+				continue;
+		}
+
 		if (x->props.family == encap_family &&
 		    x->props.reqid == tmpl->reqid &&
 		    (mark & x->mark.m) == x->mark.v &&
@@ -1185,6 +1211,17 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 			goto out;
 		}
 
+		if (pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {
+			memcpy(&x->xso, &pol->xdo, sizeof(x->xso));
+			error = pol->xdo.dev->xfrmdev_ops->xdo_dev_state_add(x);
+			if (error) {
+				x->km.state = XFRM_STATE_DEAD;
+				to_put = x;
+				x = NULL;
+				goto out;
+			}
+		}
+
 		if (km_query(x, tmpl, pol) == 0) {
 			spin_lock_bh(&net->xfrm.xfrm_state_lock);
 			x->km.state = XFRM_STATE_ACQ;

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Steffen Klassert]

On Wed, Nov 23, 2022 at 02:53:10PM +0200, Leon Romanovsky wrote:
> On Wed, Nov 23, 2022 at 11:36:19AM +0200, Leon Romanovsky wrote:
> > Thanks for an explanation, trying it now.
> 
> Something like that?

Yes :)

> 
> The code is untested yet.
> 
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 5076f9d7a752..5819023c32ba 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1115,6 +1115,19 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
>  	rcu_read_lock();
>  	h = xfrm_dst_hash(net, daddr, saddr, tmpl->reqid, encap_family);
>  	hlist_for_each_entry_rcu(x, net->xfrm.state_bydst + h, bydst) {
> +		if (IS_ENABLED(CONFIG_XFRM_OFFLOAD) &&
> +		    pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {

Please try to avoid that check for every state in the list.
Maybe enable this code with a static key if packet offload
is used?

> +			if (x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
> +				/* HW states are in the head of list, there is no need
> +				 * to iterate further.
> +				 */
> +				break;
> +
> +			/* Packet offload: both policy and SA should have same device */
> +			if (pol->xdo.dev != x->xso.dev)
> +				continue;
> +		}
> +
>  		if (x->props.family == encap_family &&
>  		    x->props.reqid == tmpl->reqid &&
>  		    (mark & x->mark.m) == x->mark.v &&
> @@ -1132,6 +1145,19 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
>  
>  	h_wildcard = xfrm_dst_hash(net, daddr, &saddr_wildcard, tmpl->reqid, encap_family);
>  	hlist_for_each_entry_rcu(x, net->xfrm.state_bydst + h_wildcard, bydst) {
> +		if (IS_ENABLED(CONFIG_XFRM_OFFLOAD) &&
> +		    pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {
> +			if (x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
> +				/* HW states are in the head of list, there is no need
> +				 * to iterate further.
> +				 */
> +				break;
> +
> +			/* Packet offload: both policy and SA should have same device */
> +			if (pol->xdo.dev != x->xso.dev)
> +				continue;
> +		}
> +
>  		if (x->props.family == encap_family &&
>  		    x->props.reqid == tmpl->reqid &&
>  		    (mark & x->mark.m) == x->mark.v &&
> @@ -1185,6 +1211,17 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
>  			goto out;
>  		}
>  
> +		if (pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {
> +			memcpy(&x->xso, &pol->xdo, sizeof(x->xso));
> +			error = pol->xdo.dev->xfrmdev_ops->xdo_dev_state_add(x);
> +			if (error) {
> +				x->km.state = XFRM_STATE_DEAD;
> +				to_put = x;
> +				x = NULL;
> +				goto out;
> +			}
> +		}

I guess that is to handle acquires, right?
What is the idea behind that? xdo_dev_state_add sets
offload type and dev?

Re: [PATCH xfrm-next v7 6/8] xfrm: speed-up lookup of HW policies

[Leon Romanovsky]

On Thu, Nov 24, 2022 at 12:07:48PM +0100, Steffen Klassert wrote:
> On Wed, Nov 23, 2022 at 02:53:10PM +0200, Leon Romanovsky wrote:
> > On Wed, Nov 23, 2022 at 11:36:19AM +0200, Leon Romanovsky wrote:
> > > Thanks for an explanation, trying it now.
> > 
> > Something like that?
> 
> Yes :)

Great, will send proper version on Sunday.

> 
> > 
> > The code is untested yet.
> > 
> > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > index 5076f9d7a752..5819023c32ba 100644
> > --- a/net/xfrm/xfrm_state.c
> > +++ b/net/xfrm/xfrm_state.c
> > @@ -1115,6 +1115,19 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> >  	rcu_read_lock();
> >  	h = xfrm_dst_hash(net, daddr, saddr, tmpl->reqid, encap_family);
> >  	hlist_for_each_entry_rcu(x, net->xfrm.state_bydst + h, bydst) {
> > +		if (IS_ENABLED(CONFIG_XFRM_OFFLOAD) &&
> > +		    pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {
> 
> Please try to avoid that check for every state in the list.
> Maybe enable this code with a static key if packet offload
> is used?

I assumed that it will be optimized by compiled because "pol->xdo.type ==
XFRM_DEV_OFFLOAD_PACKET)" is constant here. I'll take a look for more fancy
solutions, but I have serious doubts if they give any benefits.

> 
> > +			if (x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
> > +				/* HW states are in the head of list, there is no need
> > +				 * to iterate further.
> > +				 */
> > +				break;
> > +
> > +			/* Packet offload: both policy and SA should have same device */
> > +			if (pol->xdo.dev != x->xso.dev)
> > +				continue;
> > +		}
> > +
> >  		if (x->props.family == encap_family &&
> >  		    x->props.reqid == tmpl->reqid &&
> >  		    (mark & x->mark.m) == x->mark.v &&
> > @@ -1132,6 +1145,19 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> >  
> >  	h_wildcard = xfrm_dst_hash(net, daddr, &saddr_wildcard, tmpl->reqid, encap_family);
> >  	hlist_for_each_entry_rcu(x, net->xfrm.state_bydst + h_wildcard, bydst) {
> > +		if (IS_ENABLED(CONFIG_XFRM_OFFLOAD) &&
> > +		    pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {
> > +			if (x->xso.type != XFRM_DEV_OFFLOAD_PACKET)
> > +				/* HW states are in the head of list, there is no need
> > +				 * to iterate further.
> > +				 */
> > +				break;
> > +
> > +			/* Packet offload: both policy and SA should have same device */
> > +			if (pol->xdo.dev != x->xso.dev)
> > +				continue;
> > +		}
> > +
> >  		if (x->props.family == encap_family &&
> >  		    x->props.reqid == tmpl->reqid &&
> >  		    (mark & x->mark.m) == x->mark.v &&
> > @@ -1185,6 +1211,17 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
> >  			goto out;
> >  		}
> >  
> > +		if (pol->xdo.type == XFRM_DEV_OFFLOAD_PACKET) {
> > +			memcpy(&x->xso, &pol->xdo, sizeof(x->xso));
> > +			error = pol->xdo.dev->xfrmdev_ops->xdo_dev_state_add(x);
> > +			if (error) {
> > +				x->km.state = XFRM_STATE_DEAD;
> > +				to_put = x;
> > +				x = NULL;
> > +				goto out;
> > +			}
> > +		}
> 
> I guess that is to handle acquires, right?

Yes

> What is the idea behind that?

In previous patches, I made sure that policy and SA uses same
struct xfrm_dev_offload and set fields exactly the same.

This line sets all properties::
memcpy(&x->xso, &pol->xdo, sizeof(x->xso));

And .xdo_dev_state_add() gets everything pre-configured.

But yes, it will be different in final patch to make sure that
offload_handle is cleared and dev_tracker is valid.

Thanks