diff mbox series

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

Message ID f0148001c77867d288251a96f6d838a16a6dbdc4.1667997522.git.leonro@nvidia.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Extend XFRM core to allow packet offload configuration | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 13 this patch: 13
netdev/cc_maintainers warning 1 maintainers not CCed: pabeni@redhat.com
netdev/build_clang success Errors and warnings before: 5 this patch: 5
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 13 this patch: 13
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 90 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Leon Romanovsky Nov. 9, 2022, 12:54 p.m. UTC 
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(-)

Comments

Steffen Klassert Nov. 17, 2022, 11:59 a.m. UTC | #1 
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.
Leon Romanovsky Nov. 17, 2022, 12:32 p.m. UTC | #2 
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
Steffen Klassert Nov. 18, 2022, 10:23 a.m. UTC | #3 
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.
Leon Romanovsky Nov. 21, 2022, 11:10 a.m. UTC | #4 
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)


>
diff mbox series

Patch

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);