[PATCH ipsec-next 0/6] Extend XFRM core to allow full offload configuration

[PATCH ipsec-next 0/6] Extend XFRM core to allow full offload configuration

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

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

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

The mlx5 part is coming.

Thanks

Leon Romanovsky (6):
  xfrm: add new full offload flag
  xfrm: allow state full offload mode
  xfrm: add an interface to offload policy
  xfrm: add TX datapath support for IPsec full offload mode
  xfrm: add RX datapath protection for IPsec full offload mode
  xfrm: enforce separation between priorities of HW/SW policies

 .../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                     |   3 +
 include/net/netns/xfrm.h                      |   8 +-
 include/net/xfrm.h                            | 102 ++++++++++++----
 include/uapi/linux/xfrm.h                     |   6 +
 net/xfrm/xfrm_device.c                        |  84 ++++++++++++-
 net/xfrm/xfrm_output.c                        |  19 +++
 net/xfrm/xfrm_policy.c                        | 115 ++++++++++++++++++
 net/xfrm/xfrm_user.c                          |  11 ++
 13 files changed, 342 insertions(+), 29 deletions(-)

-- 
2.35.1

[PATCH ipsec-next 1/6] xfrm: add new full 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 - full 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 736c349de8bf..77e06e8208d8 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_FULL,
+};
+
 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 65e13a099b1a..9caec9b562e1 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_FULL	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 35c7e89b2e7d..8eb100162863 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -270,12 +270,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;
 		dev_put_track(dev, &xso->dev_tracker);
+		xso->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
 
 		if (err != -EOPNOTSUPP)
 			return err;
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 6a58fec6a1fb..0df71449abe9 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -854,6 +854,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_FULL)
+		xuo->flags |= XFRM_OFFLOAD_FULL;
 
 	return 0;
 }
-- 
2.35.1

[PATCH ipsec-next 2/6] xfrm: allow state full offload mode

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Allow users to configure xfrm states with full offload mode.
The full 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 full mode can't
be configured.

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 2a8fd7020622..c182b640b80d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
@@ -256,6 +256,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 8eb100162863..c4c469a85663 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -215,6 +215,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_full_offload;
 
 	if (!x->type_offload)
 		return -EINVAL;
@@ -223,9 +224,11 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 	if (x->encap || x->tfcpad)
 		return -EINVAL;
 
-	if (xuo->flags & ~(XFRM_OFFLOAD_IPV6 | XFRM_OFFLOAD_INBOUND))
+	if (xuo->flags &
+	    ~(XFRM_OFFLOAD_IPV6 | XFRM_OFFLOAD_INBOUND | XFRM_OFFLOAD_FULL))
 		return -EINVAL;
 
+	is_full_offload = xuo->flags & XFRM_OFFLOAD_FULL;
 	dev = dev_get_by_index(net, xuo->ifindex);
 	if (!dev) {
 		if (!(xuo->flags & XFRM_OFFLOAD_INBOUND)) {
@@ -240,7 +243,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_full_offload) ? -EINVAL : 0;
 
 		dev = dst->dev;
 
@@ -251,7 +254,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_full_offload) ? -EINVAL : 0;
 	}
 
 	if (x->props.flags & XFRM_STATE_ESN &&
@@ -270,7 +273,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_full_offload)
+		xso->type = XFRM_DEV_OFFLOAD_FULL;
+	else
+		xso->type = XFRM_DEV_OFFLOAD_CRYPTO;
 
 	err = dev->xfrmdev_ops->xdo_dev_state_add(x);
 	if (err) {
@@ -280,7 +286,15 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 		dev_put_track(dev, &xso->dev_tracker);
 		xso->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
 
-		if (err != -EOPNOTSUPP)
+		/* User explicitly requested full 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 full offload mode.
+		 */
+		WARN_ON(err == -EOPNOTSUPP && is_full_offload);
+		if (err != -EOPNOTSUPP || is_full_offload)
 			return err;
 	}
 
-- 
2.35.1

[PATCH ipsec-next 3/6] 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 full IPsec offload solution.

Reviewed-by: Raed Salem <raeds@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 include/linux/netdevice.h |  3 +++
 include/net/xfrm.h        | 40 +++++++++++++++++++++++++++
 net/xfrm/xfrm_device.c    | 57 +++++++++++++++++++++++++++++++++++++++
 net/xfrm/xfrm_policy.c    |  2 ++
 net/xfrm/xfrm_user.c      |  9 +++++++
 5 files changed, 111 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index eaf66e57d891..e93a5cf8cf7d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1011,6 +1011,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 77e06e8208d8..21be19ece4f7 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 {
@@ -534,6 +535,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)
@@ -1873,6 +1876,8 @@ void xfrm_dev_backlog(struct softnet_data *sd);
 struct sk_buff *validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t features, bool *again);
 int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 		       struct xfrm_user_offload *xuo);
+int xfrm_dev_policy_add(struct net *net, struct xfrm_policy *xp,
+		       struct xfrm_user_offload *xuo, u8 dir);
 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)
@@ -1921,6 +1926,27 @@ static inline void xfrm_dev_state_free(struct xfrm_state *x)
 		dev_put_track(dev, &xso->dev_tracker);
 	}
 }
+
+static inline void xfrm_dev_policy_delete(struct xfrm_policy *x)
+{
+	struct xfrm_dev_offload *xdo = &x->xdo;
+
+	if (xdo->dev)
+		xdo->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;
+		dev_put_track(dev, &xdo->dev_tracker);
+	}
+}
 #else
 static inline void xfrm_dev_resume(struct sk_buff *skb)
 {
@@ -1948,6 +1974,20 @@ 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)
+{
+	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 c4c469a85663..f2d722eb33c3 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -302,6 +302,63 @@ 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 xfrm_dev_offload *xdo = &xp->xdo;
+	struct net_device *dev;
+	int err;
+
+	if (!xuo->flags || xuo->flags & ~XFRM_OFFLOAD_FULL)
+		/* We support only Full offload mode and it means
+		 * that user must set XFRM_OFFLOAD_FULL bit.
+		 */
+		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);
+		return -EINVAL;
+	}
+
+	xdo->dev = dev;
+	netdev_tracker_alloc(dev, &xdo->dev_tracker, GFP_ATOMIC);
+	xdo->real_dev = dev;
+	xdo->type = XFRM_DEV_OFFLOAD_FULL;
+	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);
+		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;
+		dev_put_track(dev, &xdo->dev_tracker);
+		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;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 00bd0ecff5a1..681670f7d50f 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);
@@ -2246,6 +2247,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 0df71449abe9..00474559d079 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1747,6 +1747,14 @@ static struct xfrm_policy *xfrm_policy_construct(struct net *net, struct xfrm_us
 	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);
+		if (err)
+			goto error;
+	}
+
 	return xp;
  error:
 	*errp = err;
@@ -1785,6 +1793,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;
-- 
2.35.1

Re: [PATCH ipsec-next 3/6] xfrm: add an interface to offload policy

[Steffen Klassert]

On Tue, May 10, 2022 at 01:36:54PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
>  
> +int xfrm_dev_policy_add(struct net *net, struct xfrm_policy *xp,
> +			struct xfrm_user_offload *xuo, u8 dir)
> +{
> +	struct xfrm_dev_offload *xdo = &xp->xdo;
> +	struct net_device *dev;
> +	int err;
> +
> +	if (!xuo->flags || xuo->flags & ~XFRM_OFFLOAD_FULL)
> +		/* We support only Full offload mode and it means
> +		 * that user must set XFRM_OFFLOAD_FULL bit.
> +		 */
> +		return -EINVAL;

Minor nit: Please add the comment before the 'if' statement or
use braces.

Re: [PATCH ipsec-next 3/6] xfrm: add an interface to offload policy

[Leon Romanovsky]

On Fri, May 13, 2022 at 04:44:32PM +0200, Steffen Klassert wrote:
> On Tue, May 10, 2022 at 01:36:54PM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> >  
> > +int xfrm_dev_policy_add(struct net *net, struct xfrm_policy *xp,
> > +			struct xfrm_user_offload *xuo, u8 dir)
> > +{
> > +	struct xfrm_dev_offload *xdo = &xp->xdo;
> > +	struct net_device *dev;
> > +	int err;
> > +
> > +	if (!xuo->flags || xuo->flags & ~XFRM_OFFLOAD_FULL)
> > +		/* We support only Full offload mode and it means
> > +		 * that user must set XFRM_OFFLOAD_FULL bit.
> > +		 */
> > +		return -EINVAL;
> 
> Minor nit: Please add the comment before the 'if' statement or
> use braces.

Sure, will do.

[PATCH ipsec-next 4/6] xfrm: add TX datapath support for IPsec full offload mode

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

In IPsec full 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_output.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
index d4935b3b9983..2599f3dbac08 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -718,6 +718,25 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
 		break;
 	}
 
+	if (x->xso.type == XFRM_DEV_OFFLOAD_FULL) {
+		struct dst_entry *dst = skb_dst_pop(skb);
+
+		if (!dst) {
+			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
+			return -EHOSTUNREACH;
+		}
+
+		skb_dst_set(skb, dst);
+		err = skb_dst(skb)->ops->local_out(net, skb->sk, skb);
+		if (unlikely(err != 1))
+			return err;
+
+		if (!skb_dst(skb)->xfrm)
+			return dst_output(net, skb->sk, skb);
+
+		return 0;
+	}
+
 	secpath_reset(skb);
 
 	if (xfrm_dev_offload_ok(skb, x)) {
-- 
2.35.1

Re: [PATCH ipsec-next 4/6] xfrm: add TX datapath support for IPsec full offload mode

[Steffen Klassert]

On Tue, May 10, 2022 at 01:36:55PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> In IPsec full 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_output.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
> index d4935b3b9983..2599f3dbac08 100644
> --- a/net/xfrm/xfrm_output.c
> +++ b/net/xfrm/xfrm_output.c
> @@ -718,6 +718,25 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
>  		break;
>  	}
>  
> +	if (x->xso.type == XFRM_DEV_OFFLOAD_FULL) {
> +		struct dst_entry *dst = skb_dst_pop(skb);
> +
> +		if (!dst) {
> +			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> +			return -EHOSTUNREACH;
> +		}
> +
> +		skb_dst_set(skb, dst);
> +		err = skb_dst(skb)->ops->local_out(net, skb->sk, skb);
> +		if (unlikely(err != 1))
> +			return err;
> +
> +		if (!skb_dst(skb)->xfrm)
> +			return dst_output(net, skb->sk, skb);
> +
> +		return 0;
> +	}
> +

How do we know that we send the packet really to a device that
supports this type of offload? For crypto offload, we check that
in xfrm_dev_offload_ok() and I think something similar is required
here too.

Also, the offload type still requires software policies and states.
What if a device comes up that can do a real full offload, i.e.
in a way that the kernel acts just as a stub layer between IKE
and the device. Are we going to create XFRM_DEV_OFFLOAD_FULL_2
then? We need to make sure that this case cann be supported with
the new API too.

Re: [PATCH ipsec-next 4/6] xfrm: add TX datapath support for IPsec full offload mode

[Leon Romanovsky]

On Fri, May 13, 2022 at 04:56:58PM +0200, Steffen Klassert wrote:
> On Tue, May 10, 2022 at 01:36:55PM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > In IPsec full 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_output.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
> > index d4935b3b9983..2599f3dbac08 100644
> > --- a/net/xfrm/xfrm_output.c
> > +++ b/net/xfrm/xfrm_output.c
> > @@ -718,6 +718,25 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
> >  		break;
> >  	}
> >  
> > +	if (x->xso.type == XFRM_DEV_OFFLOAD_FULL) {
> > +		struct dst_entry *dst = skb_dst_pop(skb);
> > +
> > +		if (!dst) {
> > +			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> > +			return -EHOSTUNREACH;
> > +		}
> > +
> > +		skb_dst_set(skb, dst);
> > +		err = skb_dst(skb)->ops->local_out(net, skb->sk, skb);
> > +		if (unlikely(err != 1))
> > +			return err;
> > +
> > +		if (!skb_dst(skb)->xfrm)
> > +			return dst_output(net, skb->sk, skb);
> > +
> > +		return 0;
> > +	}
> > +
> 
> How do we know that we send the packet really to a device that
> supports this type of offload? For crypto offload, we check that
> in xfrm_dev_offload_ok() and I think something similar is required
> here too.

I think that function is needed to make sure that we will have SW
fallback. It is not needed in full offload, anything that is not
supported/wrong should be dropped by HW.

> 
> Also, the offload type still requires software policies and states.
> What if a device comes up that can do a real full offload, i.e.
> in a way that the kernel acts just as a stub layer between IKE
> and the device. Are we going to create XFRM_DEV_OFFLOAD_FULL_2
> then? We need to make sure that this case cann be supported with
> the new API too.

Yes, I think that it is supported by this API.

From user perspective, all flavours of full offload are the same, the
difference is in-kernel API, where we will be able differentiate with
some sort of features flag.

Thanks

Re: [PATCH ipsec-next 4/6] xfrm: add TX datapath support for IPsec full offload mode

[Steffen Klassert]

On Mon, May 16, 2022 at 08:44:58AM +0300, Leon Romanovsky wrote:
> On Fri, May 13, 2022 at 04:56:58PM +0200, Steffen Klassert wrote:
> > On Tue, May 10, 2022 at 01:36:55PM +0300, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@nvidia.com>
> > > 
> > > In IPsec full 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_output.c | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
> > > index d4935b3b9983..2599f3dbac08 100644
> > > --- a/net/xfrm/xfrm_output.c
> > > +++ b/net/xfrm/xfrm_output.c
> > > @@ -718,6 +718,25 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
> > >  		break;
> > >  	}
> > >  
> > > +	if (x->xso.type == XFRM_DEV_OFFLOAD_FULL) {
> > > +		struct dst_entry *dst = skb_dst_pop(skb);
> > > +
> > > +		if (!dst) {
> > > +			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> > > +			return -EHOSTUNREACH;
> > > +		}
> > > +
> > > +		skb_dst_set(skb, dst);
> > > +		err = skb_dst(skb)->ops->local_out(net, skb->sk, skb);
> > > +		if (unlikely(err != 1))
> > > +			return err;
> > > +
> > > +		if (!skb_dst(skb)->xfrm)
> > > +			return dst_output(net, skb->sk, skb);
> > > +
> > > +		return 0;
> > > +	}
> > > +
> > 
> > How do we know that we send the packet really to a device that
> > supports this type of offload? For crypto offload, we check that
> > in xfrm_dev_offload_ok() and I think something similar is required
> > here too.
> 
> I think that function is needed to make sure that we will have SW
> fallback. It is not needed in full offload, anything that is not
> supported/wrong should be dropped by HW.

Yes, but only if the final output device really supports this kind
of offload. How can we be sure that this is the case? Packets can be
rerouted etc. We need to make sure that the outgoing device supports
full offload, and I think this check is missing somewhere.

Re: [PATCH ipsec-next 4/6] xfrm: add TX datapath support for IPsec full offload mode

[Leon Romanovsky]

On Wed, May 18, 2022 at 09:49:14AM +0200, Steffen Klassert wrote:
> On Mon, May 16, 2022 at 08:44:58AM +0300, Leon Romanovsky wrote:
> > On Fri, May 13, 2022 at 04:56:58PM +0200, Steffen Klassert wrote:
> > > On Tue, May 10, 2022 at 01:36:55PM +0300, Leon Romanovsky wrote:
> > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > 
> > > > In IPsec full 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_output.c | 19 +++++++++++++++++++
> > > >  1 file changed, 19 insertions(+)
> > > > 
> > > > diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
> > > > index d4935b3b9983..2599f3dbac08 100644
> > > > --- a/net/xfrm/xfrm_output.c
> > > > +++ b/net/xfrm/xfrm_output.c
> > > > @@ -718,6 +718,25 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
> > > >  		break;
> > > >  	}
> > > >  
> > > > +	if (x->xso.type == XFRM_DEV_OFFLOAD_FULL) {
> > > > +		struct dst_entry *dst = skb_dst_pop(skb);
> > > > +
> > > > +		if (!dst) {
> > > > +			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> > > > +			return -EHOSTUNREACH;
> > > > +		}
> > > > +
> > > > +		skb_dst_set(skb, dst);
> > > > +		err = skb_dst(skb)->ops->local_out(net, skb->sk, skb);
> > > > +		if (unlikely(err != 1))
> > > > +			return err;
> > > > +
> > > > +		if (!skb_dst(skb)->xfrm)
> > > > +			return dst_output(net, skb->sk, skb);
> > > > +
> > > > +		return 0;
> > > > +	}
> > > > +
> > > 
> > > How do we know that we send the packet really to a device that
> > > supports this type of offload? For crypto offload, we check that
> > > in xfrm_dev_offload_ok() and I think something similar is required
> > > here too.
> > 
> > I think that function is needed to make sure that we will have SW
> > fallback. It is not needed in full offload, anything that is not
> > supported/wrong should be dropped by HW.
> 
> Yes, but only if the final output device really supports this kind
> of offload. How can we be sure that this is the case? Packets can be
> rerouted etc. We need to make sure that the outgoing device supports
> full offload, and I think this check is missing somewhere.

I think that something like this is missing (on top of the original patch):

diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
index 2599f3dbac08..a41aef3e8903 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -726,6 +726,9 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
                        return -EHOSTUNREACH;
                }

+               if (!xfrm_dev_offload_ok(skb, x))
+                       return -EOPNOTSUPP;
+
                skb_dst_set(skb, dst);
                err = skb_dst(skb)->ops->local_out(net, skb->sk, skb);
                if (unlikely(err != 1))
(END)

[PATCH ipsec-next 5/6] xfrm: add RX datapath protection for IPsec full offload mode

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Traffic received by device with enabled IPsec full 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.

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 21be19ece4f7..9f9250fe1c4d 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1094,6 +1094,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);
@@ -1113,6 +1136,15 @@ 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 (xo) {
+		x = xfrm_input_state(skb);
+		if (x->xso.type == XFRM_DEV_OFFLOAD_FULL)
+			return (xo->flags & CRYPTO_DONE) &&
+			       (xo->status & CRYPTO_SUCCESS);
+	}
 
 	if (sk && sk->sk_policy[XFRM_POLICY_IN])
 		return __xfrm_policy_check(sk, ndir, skb, family);
@@ -1845,29 +1877,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.35.1

Re: [PATCH ipsec-next 5/6] xfrm: add RX datapath protection for IPsec full offload mode

[Steffen Klassert]

On Tue, May 10, 2022 at 01:36:56PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Traffic received by device with enabled IPsec full 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.
> 
> 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 21be19ece4f7..9f9250fe1c4d 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -1094,6 +1094,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);
> @@ -1113,6 +1136,15 @@ 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 (xo) {
> +		x = xfrm_input_state(skb);
> +		if (x->xso.type == XFRM_DEV_OFFLOAD_FULL)
> +			return (xo->flags & CRYPTO_DONE) &&
> +			       (xo->status & CRYPTO_SUCCESS);
> +	}

We can not exit without doing the policy check here. The inner
packet could still match a block policy in software. Maybe
we can reset the secpath and do the policy check.

Re: [PATCH ipsec-next 5/6] xfrm: add RX datapath protection for IPsec full offload mode

[Leon Romanovsky]

On Fri, May 13, 2022 at 05:02:54PM +0200, Steffen Klassert wrote:
> On Tue, May 10, 2022 at 01:36:56PM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > Traffic received by device with enabled IPsec full 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.
> > 
> > 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 21be19ece4f7..9f9250fe1c4d 100644
> > --- a/include/net/xfrm.h
> > +++ b/include/net/xfrm.h
> > @@ -1094,6 +1094,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);
> > @@ -1113,6 +1136,15 @@ 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 (xo) {
> > +		x = xfrm_input_state(skb);
> > +		if (x->xso.type == XFRM_DEV_OFFLOAD_FULL)
> > +			return (xo->flags & CRYPTO_DONE) &&
> > +			       (xo->status & CRYPTO_SUCCESS);
> > +	}
> 
> We can not exit without doing the policy check here. The inner
> packet could still match a block policy in software. Maybe
> we can reset the secpath and do the policy check.

We checked that both policy and state were offloaded. In such case,
driver returned that everything ok and the packet is handled.

SW policy will be in lower priority, so we won't catch it.

Thanks

Re: [PATCH ipsec-next 5/6] xfrm: add RX datapath protection for IPsec full offload mode

[Steffen Klassert]

On Mon, May 16, 2022 at 08:29:03AM +0300, Leon Romanovsky wrote:
> On Fri, May 13, 2022 at 05:02:54PM +0200, Steffen Klassert wrote:
> > On Tue, May 10, 2022 at 01:36:56PM +0300, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@nvidia.com>
> > > 
> > > Traffic received by device with enabled IPsec full 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.
> > > 
> > > 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 21be19ece4f7..9f9250fe1c4d 100644
> > > --- a/include/net/xfrm.h
> > > +++ b/include/net/xfrm.h
> > > @@ -1094,6 +1094,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);
> > > @@ -1113,6 +1136,15 @@ 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 (xo) {
> > > +		x = xfrm_input_state(skb);
> > > +		if (x->xso.type == XFRM_DEV_OFFLOAD_FULL)
> > > +			return (xo->flags & CRYPTO_DONE) &&
> > > +			       (xo->status & CRYPTO_SUCCESS);
> > > +	}
> > 
> > We can not exit without doing the policy check here. The inner
> > packet could still match a block policy in software. Maybe
> > we can reset the secpath and do the policy check.
> 
> We checked that both policy and state were offloaded. In such case,
> driver returned that everything ok and the packet is handled.
> 
> SW policy will be in lower priority, so we won't catch it.

Ok.

[PATCH ipsec-next 6/6] xfrm: enforce separation between priorities of HW/SW policies

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

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

In order to make sure that users have coherent picture, let's require to
make sure that HW offloaded policies have always (both RX and TX) higher
priorities than SW ones.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 include/net/netns/xfrm.h |   8 ++-
 net/xfrm/xfrm_policy.c   | 113 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
index bd7c3be4af5d..f0cfa0faf611 100644
--- a/include/net/netns/xfrm.h
+++ b/include/net/netns/xfrm.h
@@ -29,6 +29,11 @@ struct xfrm_policy_hthresh {
 	u8			rbits6;
 };
 
+struct xfrm_policy_prio {
+	u32 max_sw_prio;
+	u32 min_hw_prio;
+};
+
 struct netns_xfrm {
 	struct list_head	state_all;
 	/*
@@ -52,7 +57,7 @@ struct netns_xfrm {
 	unsigned int		policy_idx_hmask;
 	struct hlist_head	policy_inexact[XFRM_POLICY_MAX];
 	struct xfrm_policy_hash	policy_bydst[XFRM_POLICY_MAX];
-	unsigned int		policy_count[XFRM_POLICY_MAX * 2];
+	unsigned int		policy_count[XFRM_POLICY_MAX * 3];
 	struct work_struct	policy_hash_work;
 	struct xfrm_policy_hthresh policy_hthresh;
 	struct list_head	inexact_bins;
@@ -67,6 +72,7 @@ struct netns_xfrm {
 	u32			sysctl_acq_expires;
 
 	u8			policy_default[XFRM_POLICY_MAX];
+	struct xfrm_policy_prio	policy_prio[XFRM_POLICY_MAX];
 
 #ifdef CONFIG_SYSCTL
 	struct ctl_table_header	*sysctl_hdr;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 681670f7d50f..9b6f89618ab4 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1570,13 +1570,70 @@ static struct xfrm_policy *xfrm_policy_insert_list(struct hlist_head *chain,
 	return delpol;
 }
 
+static int __xfrm_policy_check_hw_priority(struct net *net,
+					   struct xfrm_policy *policy, int dir)
+{
+	int left, right;
+
+	lockdep_assert_held(&net->xfrm.xfrm_policy_lock);
+
+	if (!net->xfrm.policy_count[dir])
+		/* Adding first policy */
+		return 0;
+
+	if (policy->xdo.type != XFRM_DEV_OFFLOAD_FULL) {
+		/* SW priority */
+		if (!net->xfrm.policy_count[2 * XFRM_POLICY_MAX + dir])
+			/* Special case to allow reuse maximum priority
+			 * (U32_MAX) for SW policies, when no HW policy exist.
+			 */
+			return 0;
+
+		left = policy->priority;
+		right = net->xfrm.policy_prio[dir].min_hw_prio;
+	} else {
+		/* HW priority */
+		left = net->xfrm.policy_prio[dir].max_sw_prio;
+		right = policy->priority;
+	}
+	if (left >= right)
+		return -EINVAL;
+
+	return 0;
+}
+
+static void __xfrm_policy_update_hw_priority(struct net *net,
+					     struct xfrm_policy *policy,
+					     int dir)
+{
+	u32 *hw_prio, *sw_prio;
+
+	lockdep_assert_held(&net->xfrm.xfrm_policy_lock);
+
+	if (policy->xdo.type != XFRM_DEV_OFFLOAD_FULL) {
+		sw_prio = &net->xfrm.policy_prio[dir].max_sw_prio;
+		*sw_prio = max(*sw_prio, policy->priority);
+		return;
+	}
+
+	hw_prio = &net->xfrm.policy_prio[dir].min_hw_prio;
+	*hw_prio = min(*hw_prio, policy->priority);
+}
+
 int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
 {
 	struct net *net = xp_net(policy);
 	struct xfrm_policy *delpol;
 	struct hlist_head *chain;
+	int ret;
 
 	spin_lock_bh(&net->xfrm.xfrm_policy_lock);
+	ret = __xfrm_policy_check_hw_priority(net, policy, dir);
+	if (ret) {
+		spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
+		return ret;
+	}
+
 	chain = policy_hash_bysel(net, &policy->selector, policy->family, dir);
 	if (chain)
 		delpol = xfrm_policy_insert_list(chain, policy, excl);
@@ -1606,6 +1663,7 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
 	policy->curlft.use_time = 0;
 	if (!mod_timer(&policy->timer, jiffies + HZ))
 		xfrm_pol_hold(policy);
+	__xfrm_policy_update_hw_priority(net, policy, dir);
 	spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
 
 	if (delpol)
@@ -2205,6 +2263,8 @@ static void __xfrm_policy_link(struct xfrm_policy *pol, int dir)
 
 	list_add(&pol->walk.all, &net->xfrm.policy_all);
 	net->xfrm.policy_count[dir]++;
+	if (pol->xdo.type == XFRM_DEV_OFFLOAD_FULL)
+		net->xfrm.policy_count[2 * XFRM_POLICY_MAX + dir]++;
 	xfrm_pol_hold(pol);
 }
 
@@ -2224,6 +2284,8 @@ static struct xfrm_policy *__xfrm_policy_unlink(struct xfrm_policy *pol,
 	}
 
 	list_del_init(&pol->walk.all);
+	if (pol->xdo.type == XFRM_DEV_OFFLOAD_FULL)
+		net->xfrm.policy_count[2 * XFRM_POLICY_MAX + dir]--;
 	net->xfrm.policy_count[dir]--;
 
 	return pol;
@@ -2239,12 +2301,58 @@ static void xfrm_sk_policy_unlink(struct xfrm_policy *pol, int dir)
 	__xfrm_policy_unlink(pol, XFRM_POLICY_MAX + dir);
 }
 
+static void __xfrm_policy_delete_prio(struct net *net,
+				      struct xfrm_policy *policy, int dir)
+{
+	struct xfrm_policy *pol;
+	u32 sw_prio = 0;
+
+	lockdep_assert_held(&net->xfrm.xfrm_policy_lock);
+
+	if (!net->xfrm.policy_count[dir]) {
+		net->xfrm.policy_prio[dir].max_sw_prio = sw_prio;
+		net->xfrm.policy_prio[dir].min_hw_prio = U32_MAX;
+		return;
+	}
+
+	if (policy->xdo.type == XFRM_DEV_OFFLOAD_FULL &&
+	    !net->xfrm.policy_count[2 * XFRM_POLICY_MAX + dir]) {
+		net->xfrm.policy_prio[dir].min_hw_prio = U32_MAX;
+		return;
+	}
+
+	list_for_each_entry(pol, &net->xfrm.policy_all, walk.all) {
+		if (pol->walk.dead)
+			continue;
+
+		if (policy->xdo.type != XFRM_DEV_OFFLOAD_FULL) {
+			/* SW priority */
+			if (pol->xdo.type == XFRM_DEV_OFFLOAD_FULL) {
+				net->xfrm.policy_prio[dir].max_sw_prio = sw_prio;
+				return;
+			}
+			sw_prio = pol->priority;
+			continue;
+		}
+		/* HW priority */
+		if (pol->xdo.type != XFRM_DEV_OFFLOAD_FULL)
+			continue;
+
+		net->xfrm.policy_prio[dir].min_hw_prio = pol->priority;
+		return;
+	}
+
+	net->xfrm.policy_prio[dir].max_sw_prio = sw_prio;
+}
+
 int xfrm_policy_delete(struct xfrm_policy *pol, int dir)
 {
 	struct net *net = xp_net(pol);
 
 	spin_lock_bh(&net->xfrm.xfrm_policy_lock);
 	pol = __xfrm_policy_unlink(pol, dir);
+	if (pol)
+		__xfrm_policy_delete_prio(net, pol, dir);
 	spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
 	if (pol) {
 		xfrm_dev_policy_delete(pol);
@@ -4042,6 +4150,7 @@ static int __net_init xfrm_policy_init(struct net *net)
 
 		net->xfrm.policy_count[dir] = 0;
 		net->xfrm.policy_count[XFRM_POLICY_MAX + dir] = 0;
+		net->xfrm.policy_count[2 * XFRM_POLICY_MAX + dir] = 0;
 		INIT_HLIST_HEAD(&net->xfrm.policy_inexact[dir]);
 
 		htab = &net->xfrm.policy_bydst[dir];
@@ -4127,6 +4236,10 @@ static int __net_init xfrm_net_init(struct net *net)
 	net->xfrm.policy_default[XFRM_POLICY_FWD] = XFRM_USERPOLICY_ACCEPT;
 	net->xfrm.policy_default[XFRM_POLICY_OUT] = XFRM_USERPOLICY_ACCEPT;
 
+	net->xfrm.policy_prio[XFRM_POLICY_IN].min_hw_prio = U32_MAX;
+	net->xfrm.policy_prio[XFRM_POLICY_FWD].min_hw_prio = U32_MAX;
+	net->xfrm.policy_prio[XFRM_POLICY_OUT].min_hw_prio = U32_MAX;
+
 	rv = xfrm_statistics_init(net);
 	if (rv < 0)
 		goto out_statistics;
-- 
2.35.1

Re: [PATCH ipsec-next 6/6] xfrm: enforce separation between priorities of HW/SW policies

[Steffen Klassert]

On Tue, May 10, 2022 at 01:36:57PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Devices that implement IPsec full offload mode offload policies too.
> In RX path, it causes to the situation that HW can't effectively handle
> mixed SW and HW priorities unless users make sure that HW offloaded
> policies have higher priorities.
> 
> In order to make sure that users have coherent picture, let's require to
> make sure that HW offloaded policies have always (both RX and TX) higher
> priorities than SW ones.

I'm still not sure whether splitting priorities in software and hardware
is the right way to go. I fear we can get problems with corner cases we
don't think about now. But OTOH I don't have a better idea. So maybe
someone on the list has an opinion on that.

Re: [PATCH ipsec-next 6/6] xfrm: enforce separation between priorities of HW/SW policies

[Leon Romanovsky]

On Fri, May 13, 2022 at 05:07:02PM +0200, Steffen Klassert wrote:
> On Tue, May 10, 2022 at 01:36:57PM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > Devices that implement IPsec full offload mode offload policies too.
> > In RX path, it causes to the situation that HW can't effectively handle
> > mixed SW and HW priorities unless users make sure that HW offloaded
> > policies have higher priorities.
> > 
> > In order to make sure that users have coherent picture, let's require to
> > make sure that HW offloaded policies have always (both RX and TX) higher
> > priorities than SW ones.
> 
> I'm still not sure whether splitting priorities in software and hardware
> is the right way to go. I fear we can get problems with corner cases we
> don't think about now. But OTOH I don't have a better idea. So maybe
> someone on the list has an opinion on that.

I see this patch as aid to catch wrong configurations of policy
priorities, at least for simple users. I didn't have a goal to
create full featured validator to don't add complexity where it
is not necessary.

Thanks