diff mbox series

[RFC,xfrm-next,v3,4/8] xfrm: add TX datapath support for IPsec full offload mode

Message ID 0a44d3b02479e5b19831038f9dc3a99259fa50f3.1662295929.git.leonro@nvidia.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series Extend XFRM core to allow full 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: 3 this patch: 3
netdev/cc_maintainers success CCed 7 of 7 maintainers
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: 3 this patch: 3
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 51 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 Sept. 4, 2022, 1:15 p.m. UTC 
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_device.c | 15 +++++++++++++--
 net/xfrm/xfrm_output.c | 12 +++++++++++-
 2 files changed, 24 insertions(+), 3 deletions(-)

Comments

Steffen Klassert Sept. 25, 2022, 9:16 a.m. UTC | #1 
On Sun, Sep 04, 2022 at 04:15:38PM +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_device.c | 15 +++++++++++++--
>  net/xfrm/xfrm_output.c | 12 +++++++++++-
>  2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> index 1cc482e9c87d..2d37bb86914a 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 full offload engine,
> +	 * but to wrong device. Drop the packet, so it won't skip
> +	 * XFRM stack.
> +	 */
> +	if (x->xso.type == XFRM_DEV_OFFLOAD_FULL && 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;
> @@ -369,8 +379,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_FULL ||
> +	    ((!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..dde009be8463 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_FULL)
>  		goto resume;

You check here that the state is marked as 'full offload' before
you skip the SW xfrm handling, but I don't see where you check
that the policy that led to this state is offloaded too. Also,
we have to make sure that both, policy and state is offloaded to
the same device. Looks like this part is missing.
Leon Romanovsky Sept. 26, 2022, 6:06 a.m. UTC | #2 
On Sun, Sep 25, 2022 at 11:16:03AM +0200, Steffen Klassert wrote:
> On Sun, Sep 04, 2022 at 04:15:38PM +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_device.c | 15 +++++++++++++--
> >  net/xfrm/xfrm_output.c | 12 +++++++++++-
> >  2 files changed, 24 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> > index 1cc482e9c87d..2d37bb86914a 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 full offload engine,
> > +	 * but to wrong device. Drop the packet, so it won't skip
> > +	 * XFRM stack.
> > +	 */
> > +	if (x->xso.type == XFRM_DEV_OFFLOAD_FULL && 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;
> > @@ -369,8 +379,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_FULL ||
> > +	    ((!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..dde009be8463 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_FULL)
> >  		goto resume;
> 
> You check here that the state is marked as 'full offload' before
> you skip the SW xfrm handling, but I don't see where you check
> that the policy that led to this state is offloaded too. Also,
> we have to make sure that both, policy and state is offloaded to
> the same device. Looks like this part is missing.

In SW flow, users are not required to configure policy. If they don't
have policy, the packet will be encrypted and sent anyway.

The full offload follows same semantic. The missing offloaded policy is
equal to no policy at all.

I don't think that extra checks are needed.

Thanks

>
Steffen Klassert Sept. 27, 2022, 5:04 a.m. UTC | #3 
On Mon, Sep 26, 2022 at 09:06:48AM +0300, Leon Romanovsky wrote:
> On Sun, Sep 25, 2022 at 11:16:03AM +0200, Steffen Klassert wrote:
> > On Sun, Sep 04, 2022 at 04:15:38PM +0300, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@nvidia.com>
> > > diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
> > > index 9a5e79a38c67..dde009be8463 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_FULL)
> > >  		goto resume;
> > 
> > You check here that the state is marked as 'full offload' before
> > you skip the SW xfrm handling, but I don't see where you check
> > that the policy that led to this state is offloaded too. Also,
> > we have to make sure that both, policy and state is offloaded to
> > the same device. Looks like this part is missing.
> 
> In SW flow, users are not required to configure policy. If they don't
> have policy, the packet will be encrypted and sent anyway.

No, it is not! You can't lookup a TX SA without a policy. The lookup
happens in two stages. The packet header is matched against the TS of
the policy. Then the template found at the policy is used to lookup
the SA.

> The full offload follows same semantic. The missing offloaded policy is
> equal to no policy at all.

No policy at all means that the packets are sent out unencrypted in
plaintext, and this is certainly not what you want.

> I don't think that extra checks are needed.

We need this checks. This is one of the reasons why I want to separate
the SW and HW databases.
diff mbox series

Patch

diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 1cc482e9c87d..2d37bb86914a 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 full offload engine,
+	 * but to wrong device. Drop the packet, so it won't skip
+	 * XFRM stack.
+	 */
+	if (x->xso.type == XFRM_DEV_OFFLOAD_FULL && 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;
@@ -369,8 +379,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_FULL ||
+	    ((!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..dde009be8463 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_FULL)
 		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_FULL) {
+		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)) {