[PATCH 1/2] xfrm: add SA information to the offloaded packet

[PATCH 1/2] xfrm: add SA information to the offloaded packet

[Feng Wang]

From: wangfe <wangfe@google.com>

In packet offload mode, append Security Association (SA) information
to each packet, replicating the crypto offload implementation.
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.

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.

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

Signed-off-by: wangfe <wangfe@google.com>
---
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     | 24 +++++++++++++++++++++++-
 drivers/net/netdevsim/netdevsim.h |  1 +
 net/xfrm/xfrm_output.c            | 21 +++++++++++++++++++++
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/net/netdevsim/ipsec.c b/drivers/net/netdevsim/ipsec.c
index f0d58092e7e9..1ce7447dd269 100644
--- a/drivers/net/netdevsim/ipsec.c
+++ b/drivers/net/netdevsim/ipsec.c
@@ -149,7 +149,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;
 	}
@@ -165,6 +166,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;
@@ -224,10 +226,24 @@ 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)
@@ -272,6 +288,12 @@ bool nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
 		return false;
 	}
 
+	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;
+	}
+
 	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..a12588e7b060 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -706,6 +706,8 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
 	struct xfrm_state *x = skb_dst(skb)->xfrm;
 	int family;
 	int err;
+	struct xfrm_offload *xo;
+	struct sec_path *sp;
 
 	family = (x->xso.type != XFRM_DEV_OFFLOAD_PACKET) ? x->outer_mode.family
 		: skb_dst(skb)->ops->family;
@@ -728,6 +730,25 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
 			kfree_skb(skb);
 			return -EHOSTUNREACH;
 		}
+		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.199.ga7371fff76-goog

Re: [PATCH 1/2] xfrm: add SA information to the offloaded packet

[Leon Romanovsky]

On Mon, Nov 04, 2024 at 03:32:51PM -0800, Feng Wang wrote:
> From: wangfe <wangfe@google.com>
> 
> In packet offload mode, append Security Association (SA) information
> to each packet, replicating the crypto offload implementation.
> 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.
> 
> 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.
> 
> Enable packet offload mode on netdevsim and add code to check the XFRM
> interface ID.
> 
> Signed-off-by: wangfe <wangfe@google.com>

Something wrong with your git setup, lore shows only one patch.
https://lore.kernel.org/all/20241104233251.3387719-1-wangfe@google.com/

> ---
> 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     | 24 +++++++++++++++++++++++-
>  drivers/net/netdevsim/netdevsim.h |  1 +
>  net/xfrm/xfrm_output.c            | 21 +++++++++++++++++++++
>  3 files changed, 45 insertions(+), 1 deletion(-)

Don't you need to update current drivers who support packet offload with
if(x->if_id) check in their SA/policy validator to return an error to
the user? They don't support that flow.

> 
> diff --git a/drivers/net/netdevsim/ipsec.c b/drivers/net/netdevsim/ipsec.c
> index f0d58092e7e9..1ce7447dd269 100644
> --- a/drivers/net/netdevsim/ipsec.c
> +++ b/drivers/net/netdevsim/ipsec.c
> @@ -149,7 +149,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;
>  	}
> @@ -165,6 +166,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;
> @@ -224,10 +226,24 @@ 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)
> @@ -272,6 +288,12 @@ bool nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
>  		return false;
>  	}
>  
> +	if (xs->if_id != tsa->if_id) {

if_id exists in policy and state, but you are checking only state.

> +		netdev_err(ns->netdev, "unmatched if_id %d %d\n",
> +			   xs->if_id, tsa->if_id);
> +		return false;
> +	}

It is worth to add check for having XFRM_XMIT flag here.

> +
>  	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..a12588e7b060 100644
> --- a/net/xfrm/xfrm_output.c
> +++ b/net/xfrm/xfrm_output.c
> @@ -706,6 +706,8 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
>  	struct xfrm_state *x = skb_dst(skb)->xfrm;
>  	int family;
>  	int err;
> +	struct xfrm_offload *xo;
> +	struct sec_path *sp;
>  
>  	family = (x->xso.type != XFRM_DEV_OFFLOAD_PACKET) ? x->outer_mode.family
>  		: skb_dst(skb)->ops->family;
> @@ -728,6 +730,25 @@ int xfrm_output(struct sock *sk, struct sk_buff *skb)
>  			kfree_skb(skb);
>  			return -EHOSTUNREACH;
>  		}

I think that code below should be set under "if (x->if_id)".

Thanks

> +		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.199.ga7371fff76-goog
> 
> 

Re: [PATCH 1/2] xfrm: add SA information to the offloaded packet

[Feng Wang]

Hi Leon,
I checked the current tree and there are no drivers who support packet
offload. Even for the mlx5 driver, it only supports crypto offload
mode. If I miss anything, please let me know.
Since the driver only requires the Security Association (SA) to
perform the necessary transformations,  policy information is not
needed. Storing policy information, matching the policy and checking
the if_id within the driver wouldn't provide much benefit. It would
increase CPU and memory usage without a clear advantage.
For all other suggestions, I totally agree with you.

Thanks,
Feng

Re: [PATCH 1/2] xfrm: add SA information to the offloaded packet

[Leon Romanovsky]

On Tue, Nov 05, 2024 at 03:41:15PM -0800, Feng Wang wrote:
> Hi Leon,
> I checked the current tree and there are no drivers who support packet
> offload. Even for the mlx5 driver, it only supports crypto offload
> mode. 

I don't know what to add here. We already had this discussion for more
than once.
https://lore.kernel.org/all/ZfpnCIv+8eYd7CpO@gauss3.secunet.de/
Let's me cite Steffen:

"There are 'packet offload drivers' in the kernel, that's why we
support this kind of offload."

> If I miss anything, please let me know.
> Since the driver only requires the Security Association (SA) to
> perform the necessary transformations,  policy information is not
> needed. Storing policy information, matching the policy and checking
> the if_id within the driver wouldn't provide much benefit. 

You need to make sure that policy and SA has match in their selectors,
and IMHO you can't add support to SA without adding same support to
policy.

> It would increase CPU and memory usage without a clear advantage.
> For all other suggestions, I totally agree with you.
> 
> Thanks,
> Feng

Re: [PATCH 1/2] xfrm: add SA information to the offloaded packet

[Antony Antony]

> On Tue, Nov 05, 2024 at 03:41:15PM -0800, Feng Wang wrote:
> > Hi Leon,
> > I checked the current tree and there are no drivers who support packet
> > offload. Even for the mlx5 driver, it only supports crypto offload
> > mode. 
> 
> I don't know what to add here. We already had this discussion for more
> than once.
> https://lore.kernel.org/all/ZfpnCIv+8eYd7CpO@gauss3.secunet.de/
> Let's me cite Steffen:
> 
> "There are 'packet offload drivers' in the kernel, that's why we
> support this kind of offload."
> 
> > If I miss anything, please let me know.
> > Since the driver only requires the Security Association (SA) to
> > perform the necessary transformations,  policy information is not
> > needed. Storing policy information, matching the policy and checking
> > the if_id within the driver wouldn't provide much benefit. 
> 
> You need to make sure that policy and SA has match in their selectors,
> and IMHO you can't add support to SA without adding same support to
> policy.

when a packet enters an XFRMi,  xfrm_lookup_with_ifid() is called? 
What does that call return, incase of packet offload? 

My guess is that call is returning NULL and that is why Feng is trying hard code
state here? 

I imagine the policy may not match because that policy has
offload packet?  May be this look up need to handle packet offload?

> 
> > It would increase CPU and memory usage without a clear advantage.
> > For all other suggestions, I totally agree with you.
> > 
> > Thanks,
> > Feng

Re: [PATCH 1/2] xfrm: add SA information to the offloaded packet

[Feng Wang]

Antony brought out an important function xfrm_lookup_with_ifid(), this
function returns the next dst_entry.

The xfrm_lookup_with_ifid() function utilizes xfrm_sk_policy_lookup()
to find a matching policy based on the given if_id. The if_id checking
is handled in it.
Once the policy is found, xfrm_resolve_and_create_bundle() determines
the correct Security Association (SA) and associates it with the
destination entry (dst->xfrm).
This SA information is then passed directly to the driver. Since the
kernel has already performed the necessary if_id checks for policy,
there's no need for the driver to duplicate this effort.

Re: [PATCH 1/2] xfrm: add SA information to the offloaded packet

[Antony Antony]

On Wed, Nov 06, 2024 at 16:14:36 -0800, Feng Wang wrote:
> Antony brought out an important function xfrm_lookup_with_ifid(), this
> function returns the next dst_entry.
> 
> The xfrm_lookup_with_ifid() function utilizes xfrm_sk_policy_lookup()

would the output packet, looked up using xfrm_lookup_with_ifid ,
match xfrm policy with "offload packet" set?
When lookup is in the xfrm device.

ip xfrm policy .... offload packet dev <if-name>


> to find a matching policy based on the given if_id. The if_id checking
> is handled in it.
> Once the policy is found, xfrm_resolve_and_create_bundle() determines
> the correct Security Association (SA) and associates it with the
> destination entry (dst->xfrm).

If the output packet got this far, dst is set in skb?
And when the packet reach the driver dst = skb_dst(skb);
dst->xfrm is the state?
If this is the case  why add state to skb as your patch proose?
May be I am missing something in the packet path.

> This SA information is then passed directly to the driver. Since the
> kernel has already performed the necessary if_id checks for policy,
> there's no need for the driver to duplicate this effort.

Is this how packet offload would work? My guess was in packet offload
policy look happens in the driver. 

Re: [PATCH 1/2] xfrm: add SA information to the offloaded packet

[Leon Romanovsky]

On Thu, Nov 07, 2024 at 09:52:09AM +0100, Antony Antony wrote:
> On Wed, Nov 06, 2024 at 16:14:36 -0800, Feng Wang wrote:
> > Antony brought out an important function xfrm_lookup_with_ifid(), this
> > function returns the next dst_entry.
> > 
> > The xfrm_lookup_with_ifid() function utilizes xfrm_sk_policy_lookup()
> 
> would the output packet, looked up using xfrm_lookup_with_ifid ,
> match xfrm policy with "offload packet" set?

According to my understanding, no.

> When lookup is in the xfrm device.
> 
> ip xfrm policy .... offload packet dev <if-name>
> 
> 
> > to find a matching policy based on the given if_id. The if_id checking
> > is handled in it.
> > Once the policy is found, xfrm_resolve_and_create_bundle() determines
> > the correct Security Association (SA) and associates it with the
> > destination entry (dst->xfrm).
> 
> If the output packet got this far, dst is set in skb?
> And when the packet reach the driver dst = skb_dst(skb);
> dst->xfrm is the state?
> If this is the case  why add state to skb as your patch proose?
> May be I am missing something in the packet path.
> 
> > This SA information is then passed directly to the driver. Since the
> > kernel has already performed the necessary if_id checks for policy,
> > there's no need for the driver to duplicate this effort.
> 
> Is this how packet offload would work? My guess was in packet offload
> policy look happens in the driver.

You are right, this is not how packet offload works.
The expectation is that HW sees and catches same selectors as SW.
It ensures that if SW finds policy/SA, HW will find the same.

Thanks

Re: [PATCH 1/2] xfrm: add SA information to the offloaded packet

[Feng Wang]

Based on current kernel implementation,  the packet offload does match
policy and state on the TX side in the kernel.  For the RX side, the
driver is responsible for the selector policy and SA match etc.
I added some logs in the kernel, and listed the kernel function call
flow as below for your reference, this is for TX side only.

 (1) ip_output -> neighbor_output -> dev_queue_xmit ->
netdev_start_xmit -> ops->ndo_start_xmit -> xfrmi_xmit -> xfrmi_xmit2
 (2) In the xfrmi_xmit2  {  first call xfrm_lookup_with_ifid() to get
next destination(dst),  then call dst_output() to send the packet,
the dst_output in fact is xfrm_output() }
 (3) To trace xfrm_lookup_with_ifid()   xfrm_lookup_with_ifid() ->
xfrm_bundle_lookup() ->
        {      first call xfrm_policy_lookup() to find the policy, the
policy id is checked here.
                Then call xfrm_resolve_and_create_bundle() to find the
SA and generate the next dst
                return dst;
        }
(4)To trace xfrm_output(),  now we know the next dst, calls
xfrm_output to send the packet.  xfrm_output() -> xfrm_output_resume()
 ->
        {
              first call xfrm_output_one() which goes to resume part
directly without packet transformation(this is special handling for
packet offload) but it calls skb_dst_pop to get final dst. This dst
has no xfrm(SA) so the driver can't access xfrm(SA) using skb_dst.
              Then calls dst_output() with final dst, this will route
the packet to stack again, going through ip_output() etc to pass the
packet to the driver transmit function, and send out.
         }

I did make a mistake on calling xfrm_sk_policy_lookup in the
xfrm_lookup_with_ifid function.  In fact,  it is NOT called.

Thanks,
Feng