[PATCH v7] xfrm: add SA information to the offloaded packet when if_id is set

[PATCH v7] xfrm: add SA information to the offloaded packet when if_id is set

[Feng Wang]

In packet offload mode, append Security Association (SA) information
to each packet, replicating the crypto offload implementation. This
SA info helps HW offload match packets to their correct security
policies. The XFRM interface ID is included, which is used in setups
with multiple XFRM interfaces where source/destination addresses alone
can't pinpoint the right policy.

The XFRM_XMIT flag is set to enable packet to be returned immediately
from the validate_xmit_xfrm function, thus aligning with the existing
code path for packet offload mode.

Enable packet offload mode on netdevsim and add code to check the XFRM
interface ID.

Signed-off-by: wangfe <wangfe@google.com>
---
v7: https://lore.kernel.org/all/20241121215257.3879343-2-wangfe@google.com/
  - Fix the commit message to explain the reason of adding SA.
v6: https://lore.kernel.org/all/20241119220411.2961121-1-wangfe@google.com/
  - Fix code style to follow reverse x-mas tree order declaration.
v5: https://lore.kernel.org/all/20241112192249.341515-1-wangfe@google.com/
  - Add SA information only when XFRM interface ID is non-zero.
v4: https://lore.kernel.org/all/20241104233251.3387719-1-wangfe@google.com/
  - Add offload flag check and only doing check when XFRM interface
    ID is non-zero.
v3: https://lore.kernel.org/all/20240822200252.472298-1-wangfe@google.com/
  - Add XFRM interface ID checking on netdevsim in the packet offload
    mode.
v2:
  - Add why HW offload requires the SA info to the commit message
v1: https://lore.kernel.org/all/20240812182317.1962756-1-wangfe@google.com/
---
---
 drivers/net/netdevsim/ipsec.c     | 32 ++++++++++++++++++++++++++++++-
 drivers/net/netdevsim/netdevsim.h |  1 +
 net/xfrm/xfrm_output.c            | 22 +++++++++++++++++++++
 3 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/net/netdevsim/ipsec.c b/drivers/net/netdevsim/ipsec.c
index 88187dd4eb2d..5677b2acf9c0 100644
--- a/drivers/net/netdevsim/ipsec.c
+++ b/drivers/net/netdevsim/ipsec.c
@@ -153,7 +153,8 @@ static int nsim_ipsec_add_sa(struct xfrm_state *xs,
 		return -EINVAL;
 	}
 
-	if (xs->xso.type != XFRM_DEV_OFFLOAD_CRYPTO) {
+	if (xs->xso.type != XFRM_DEV_OFFLOAD_CRYPTO &&
+	    xs->xso.type != XFRM_DEV_OFFLOAD_PACKET) {
 		NL_SET_ERR_MSG_MOD(extack, "Unsupported ipsec offload type");
 		return -EINVAL;
 	}
@@ -169,6 +170,7 @@ static int nsim_ipsec_add_sa(struct xfrm_state *xs,
 	memset(&sa, 0, sizeof(sa));
 	sa.used = true;
 	sa.xs = xs;
+	sa.if_id = xs->if_id;
 
 	if (sa.xs->id.proto & IPPROTO_ESP)
 		sa.crypt = xs->ealg || xs->aead;
@@ -227,16 +229,31 @@ static bool nsim_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)
 	return true;
 }
 
+static int nsim_ipsec_add_policy(struct xfrm_policy *policy,
+				 struct netlink_ext_ack *extack)
+{
+	return 0;
+}
+
+static void nsim_ipsec_del_policy(struct xfrm_policy *policy)
+{
+}
+
 static const struct xfrmdev_ops nsim_xfrmdev_ops = {
 	.xdo_dev_state_add	= nsim_ipsec_add_sa,
 	.xdo_dev_state_delete	= nsim_ipsec_del_sa,
 	.xdo_dev_offload_ok	= nsim_ipsec_offload_ok,
+
+	.xdo_dev_policy_add     = nsim_ipsec_add_policy,
+	.xdo_dev_policy_delete  = nsim_ipsec_del_policy,
+
 };
 
 bool nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
 {
 	struct sec_path *sp = skb_sec_path(skb);
 	struct nsim_ipsec *ipsec = &ns->ipsec;
+	struct xfrm_offload *xo;
 	struct xfrm_state *xs;
 	struct nsim_sa *tsa;
 	u32 sa_idx;
@@ -275,6 +292,19 @@ bool nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
 		return false;
 	}
 
+	if (xs->if_id) {
+		if (xs->if_id != tsa->if_id) {
+			netdev_err(ns->netdev, "unmatched if_id %d %d\n",
+				   xs->if_id, tsa->if_id);
+			return false;
+		}
+		xo = xfrm_offload(skb);
+		if (!xo || !(xo->flags & XFRM_XMIT)) {
+			netdev_err(ns->netdev, "offload flag missing or wrong\n");
+			return false;
+		}
+	}
+
 	ipsec->tx++;
 
 	return true;
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index bf02efa10956..4941b6e46d0a 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -41,6 +41,7 @@ struct nsim_sa {
 	__be32 ipaddr[4];
 	u32 key[4];
 	u32 salt;
+	u32 if_id;
 	bool used;
 	bool crypt;
 	bool rx;
diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
index e5722c95b8bb..3e9a1b17f37e 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -704,6 +704,8 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
 {
 	struct net *net = dev_net(skb_dst(skb)->dev);
 	struct xfrm_state *x = skb_dst(skb)->xfrm;
+	struct xfrm_offload *xo;
+	struct sec_path *sp;
 	int family;
 	int err;
 
@@ -728,7 +730,27 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
 			kfree_skb(skb);
 			return -EHOSTUNREACH;
 		}
+		if (x->if_id) {
+			sp = secpath_set(skb);
+			if (!sp) {
+				XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
+				kfree_skb(skb);
+				return -ENOMEM;
+			}
+
+			sp->olen++;
+			sp->xvec[sp->len++] = x;
+			xfrm_state_hold(x);
 
+			xo = xfrm_offload(skb);
+			if (!xo) {
+				secpath_reset(skb);
+				XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
+				kfree_skb(skb);
+				return -EINVAL;
+			}
+			xo->flags |= XFRM_XMIT;
+		}
 		return xfrm_output_resume(sk, skb, 0);
 	}
 
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v7] xfrm: add SA information to the offloaded packet when if_id is set

[Leon Romanovsky]

On Mon, Dec 09, 2024 at 08:28:12PM +0000, Feng Wang wrote:
> In packet offload mode, append Security Association (SA) information
> to each packet, replicating the crypto offload implementation. This
> SA info helps HW offload match packets to their correct security
> policies. The XFRM interface ID is included, which is used in setups
> with multiple XFRM interfaces where source/destination addresses alone
> can't pinpoint the right policy.
> 
> The XFRM_XMIT flag is set to enable packet to be returned immediately
> from the validate_xmit_xfrm function, thus aligning with the existing
> code path for packet offload mode.
> 
> Enable packet offload mode on netdevsim and add code to check the XFRM
> interface ID.
> 
> Signed-off-by: wangfe <wangfe@google.com>
> ---

<...>

> @@ -728,7 +730,27 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
>  			kfree_skb(skb);
>  			return -EHOSTUNREACH;
>  		}
> +		if (x->if_id) {
> +			sp = secpath_set(skb);
> +			if (!sp) {
> +				XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> +				kfree_skb(skb);
> +				return -ENOMEM;
> +			}
> +
> +			sp->olen++;
> +			sp->xvec[sp->len++] = x;
> +			xfrm_state_hold(x);
>  
> +			xo = xfrm_offload(skb);
> +			if (!xo) {
> +				secpath_reset(skb);
> +				XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> +				kfree_skb(skb);
> +				return -EINVAL;
> +			}
> +			xo->flags |= XFRM_XMIT;
> +		}

Steffen,

I would like to ask from you to delay this patch till this "if_id"
support is implemented and tested on real upstreamed device.

I have no confidence that the solution proposed above is the right thing
to do as it doesn't solve the claim "This SA info helps HW offload match
packets to their correct security". HW is going to perform lookup anyway
on the source and destination, so it is unclear how will it "help".

Thanks

>  		return xfrm_output_resume(sk, skb, 0);
>  	}
>  
> -- 
> 2.47.0.338.g60cca15819-goog
> 
> 

Re: [PATCH v7] xfrm: add SA information to the offloaded packet when if_id is set

[Feng Wang]

Hi Steffen,

This patch was done based on our previous discussion.  I did the
changes we agreed on.  This SA info includes the matched encryption
information, so it doesn't need to perform lookup on the source and
destination anymore in the driver.

Thanks,

Feng

On Mon, Dec 9, 2024 at 1:53 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Mon, Dec 09, 2024 at 08:28:12PM +0000, Feng Wang wrote:
> > In packet offload mode, append Security Association (SA) information
> > to each packet, replicating the crypto offload implementation. This
> > SA info helps HW offload match packets to their correct security
> > policies. The XFRM interface ID is included, which is used in setups
> > with multiple XFRM interfaces where source/destination addresses alone
> > can't pinpoint the right policy.
> >
> > The XFRM_XMIT flag is set to enable packet to be returned immediately
> > from the validate_xmit_xfrm function, thus aligning with the existing
> > code path for packet offload mode.
> >
> > Enable packet offload mode on netdevsim and add code to check the XFRM
> > interface ID.
> >
> > Signed-off-by: wangfe <wangfe@google.com>
> > ---
>
> <...>
>
> > @@ -728,7 +730,27 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
> >                       kfree_skb(skb);
> >                       return -EHOSTUNREACH;
> >               }
> > +             if (x->if_id) {
> > +                     sp = secpath_set(skb);
> > +                     if (!sp) {
> > +                             XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> > +                             kfree_skb(skb);
> > +                             return -ENOMEM;
> > +                     }
> > +
> > +                     sp->olen++;
> > +                     sp->xvec[sp->len++] = x;
> > +                     xfrm_state_hold(x);
> >
> > +                     xo = xfrm_offload(skb);
> > +                     if (!xo) {
> > +                             secpath_reset(skb);
> > +                             XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> > +                             kfree_skb(skb);
> > +                             return -EINVAL;
> > +                     }
> > +                     xo->flags |= XFRM_XMIT;
> > +             }
>
> Steffen,
>
> I would like to ask from you to delay this patch till this "if_id"
> support is implemented and tested on real upstreamed device.
>
> I have no confidence that the solution proposed above is the right thing
> to do as it doesn't solve the claim "This SA info helps HW offload match
> packets to their correct security". HW is going to perform lookup anyway
> on the source and destination, so it is unclear how will it "help".
>
> Thanks
>
> >               return xfrm_output_resume(sk, skb, 0);
> >       }
> >
> > --
> > 2.47.0.338.g60cca15819-goog
> >
> >

Re: [PATCH v7] xfrm: add SA information to the offloaded packet when if_id is set

[Steffen Klassert]

On Mon, Dec 09, 2024 at 08:28:12PM +0000, Feng Wang wrote:
>  
> +static int nsim_ipsec_add_policy(struct xfrm_policy *policy,
> +				 struct netlink_ext_ack *extack)
> +{
> +	return 0;
> +}

Returning 0 here is apparently wrong. If you want to add packet offload
support to nsim, you need to implement everything what a real packet
offload driver plus HW will do.

> @@ -728,7 +730,27 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
>  			kfree_skb(skb);
>  			return -EHOSTUNREACH;
>  		}
> +		if (x->if_id) {
> +			sp = secpath_set(skb);

secpath_set is expensive, this will slow down crypto offload
for no reason. This has to go to some driver specific
codepath.

Re: [PATCH v7] xfrm: add SA information to the offloaded packet when if_id is set

[Steffen Klassert]

Hi Feng,

On Mon, Dec 09, 2024 at 03:44:51PM -0800, Feng Wang wrote:
> Hi Steffen,
> 
> This patch was done based on our previous discussion.  I did the
> changes we agreed on.

there is still no real packet offload support for netdev sim.
And as said, this is at most the second best option.

You need to prove that this works. I want a complete API,
but I also want a working one.

The easiest way to prove that this is implemented correctly
is to upstream your driver. Everyting else is controversial
and complicated.

Re: [PATCH v7] xfrm: add SA information to the offloaded packet when if_id is set

[Jakub Kicinski]

On Tue, 10 Dec 2024 10:38:34 +0100 Steffen Klassert wrote:
> > This patch was done based on our previous discussion.  I did the
> > changes we agreed on.  
> 
> there is still no real packet offload support for netdev sim.
> And as said, this is at most the second best option.
> 
> You need to prove that this works. I want a complete API,
> but I also want a working one.
> 
> The easiest way to prove that this is implemented correctly
> is to upstream your driver. Everyting else is controversial
> and complicated.

Yes, I don't have full context but FWIW offload changes accompanied 
by just netdevsim modifications raise a red flag:

Quoting documentation:

  netdevsim
  ~~~~~~~~~
  
  ``netdevsim`` is a test driver which can be used to exercise driver
  configuration APIs without requiring capable hardware.
  Mock-ups and tests based on ``netdevsim`` are strongly encouraged when
  adding new APIs, but ``netdevsim`` in itself is **not** considered
  a use case/user. You must also implement the new APIs in a real driver.
  
  We give no guarantees that ``netdevsim`` won't change in the future
  in a way which would break what would normally be considered uAPI.
  
  ``netdevsim`` is reserved for use by upstream tests only, so any
  new ``netdevsim`` features must be accompanied by selftests under
  ``tools/testing/selftests/``.
  
See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#netdevsim

Re: [PATCH v7] xfrm: add SA information to the offloaded packet when if_id is set

[Steffen Klassert]

On Tue, Dec 10, 2024 at 07:20:48PM -0800, Jakub Kicinski wrote:
> On Tue, 10 Dec 2024 10:38:34 +0100 Steffen Klassert wrote:
> > > This patch was done based on our previous discussion.  I did the
> > > changes we agreed on.  
> > 
> > there is still no real packet offload support for netdev sim.
> > And as said, this is at most the second best option.
> > 
> > You need to prove that this works. I want a complete API,
> > but I also want a working one.
> > 
> > The easiest way to prove that this is implemented correctly
> > is to upstream your driver. Everyting else is controversial
> > and complicated.
> 
> Yes, I don't have full context but FWIW offload changes accompanied 
> by just netdevsim modifications raise a red flag:

Actually, we have the mlx5 driver that supports packet offload.
When that was implemented, xfrm interfaces were no usecase.
Because of that, we forgot to care for the xfrm interface ID as
a lookup key. The problem is that users can still configure
policies/states for xfrm interfaces and packet offload.
The driver then just don't know if a packet was routed via
a xfrm interface, and if so, via which one. So it might happen
that a wrong policy/state is applied to a packet.

My idea was to fix that by supporting xfrm interfaces
with packet offload in the stack. But after looking
a bit closer to the code yesterday, I noticed that we
might not need any stack changes to get this right.

I think the driver should do the following:

If xfrm interfaces are not supported:

- Reject policies and states that have xfrm interfaces
  configured.

If xfrm interfaces are supported:

- Accept policies and states that have xfrm interfaces
  configured.

- On TX: Use the drivers xdo_dev_offload_ok function
  to check if the packet came via the correct xfrm
  interface and/or amend that info by adding the
  xfrm state to the packets secpath.

- On RX: Driver traveses through the list of registered
  xfrm interfaces and match these against the used SAs
  xfrm interface ID.


By doing that, no stack changes should be required.