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

[PATCH] 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 crucial in setups
with multiple XFRM interfaces where source/destination addresses alone
can't pinpoint the right policy.

Signed-off-by: wangfe <wangfe@google.com>
---
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/
---
 net/xfrm/xfrm_output.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

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.46.0.295.g3b9ea8a38a-goog

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

[Steffen Klassert]

On Thu, Aug 22, 2024 at 01:02:52PM -0700, 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 crucial in setups
> with multiple XFRM interfaces where source/destination addresses alone
> can't pinpoint the right policy.
> 
> Signed-off-by: wangfe <wangfe@google.com>

Applied to ipsec-next, thanks Feng!

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

[Leon Romanovsky]

On Wed, Aug 28, 2024 at 07:32:47AM +0200, Steffen Klassert wrote:
> On Thu, Aug 22, 2024 at 01:02:52PM -0700, 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 crucial in setups
> > with multiple XFRM interfaces where source/destination addresses alone
> > can't pinpoint the right policy.
> > 
> > Signed-off-by: wangfe <wangfe@google.com>
> 
> Applied to ipsec-next, thanks Feng!

Stephen, can you please explain why do you think that this is correct
thing to do?

There are no in-tree any drivers which is using this information, and it
is unclear to me how state is released and it has controversial code
around validity of xfrm_offload() too.

For example:
+		sp->olen++;
+		sp->xvec[sp->len++] = x;
+		xfrm_state_hold(x);
+
+		xo = xfrm_offload(skb);
+		if (!xo) { <--- previous code handled this case perfectly in validate_xmit_xfrm
+			secpath_reset(skb);
+			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
+			kfree_skb(skb);
+			return -EINVAL; <--- xfrm state leak
+		}


Can you please revert/drop this patch for now?

Thanks

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

[Feng Wang]

Hi Leon,

Thank you for your insightful questions and comments.

Just like in crypto offload mode, now pass SA (Security Association)
information to the driver in packet offload mode. This helps the
driver quickly identify the packet's source and destination, rather
than having to parse the packet itself. The xfrm interface ID is
especially useful here. As you can see in the kernel code
(https://elixir.bootlin.com/linux/v6.10/source/net/xfrm/xfrm_policy.c#L1993),
it's used to differentiate between various policies in complex network
setups.

During my testing of packet offload mode without the patch, I observed
that the sec_path was NULL. Consequently, xo was also NULL when
validate_xmit_xfrm was called, leading to an immediate return at line
125 (https://elixir.bootlin.com/linux/v6.10/source/net/xfrm/xfrm_device.c#L125).

Regarding the potential xfrm_state leak, upon further investigation,
it may not be the case. It seems that both secpath_reset and kfree_skb
invoke the xfrm_state_put function, which should be responsible for
releasing the state. The call flow appears to be as follows: kfree_skb
-> skb_release_all -> skb_release_head_state -> skb_ext_put ->
skb_ext_put_sp -> xfrm_state_put.

Please let me know if you have any further questions or concerns. I
appreciate your valuable feedback!

Feng

On Wed, Aug 28, 2024 at 4:26 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Wed, Aug 28, 2024 at 07:32:47AM +0200, Steffen Klassert wrote:
> > On Thu, Aug 22, 2024 at 01:02:52PM -0700, 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 crucial in setups
> > > with multiple XFRM interfaces where source/destination addresses alone
> > > can't pinpoint the right policy.
> > >
> > > Signed-off-by: wangfe <wangfe@google.com>
> >
> > Applied to ipsec-next, thanks Feng!
>
> Stephen, can you please explain why do you think that this is correct
> thing to do?
>
> There are no in-tree any drivers which is using this information, and it
> is unclear to me how state is released and it has controversial code
> around validity of xfrm_offload() too.
>
> For example:
> +               sp->olen++;
> +               sp->xvec[sp->len++] = x;
> +               xfrm_state_hold(x);
> +
> +               xo = xfrm_offload(skb);
> +               if (!xo) { <--- previous code handled this case perfectly in validate_xmit_xfrm
> +                       secpath_reset(skb);
> +                       XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> +                       kfree_skb(skb);
> +                       return -EINVAL; <--- xfrm state leak
> +               }
>
>
> Can you please revert/drop this patch for now?
>
> Thanks

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

[Leon Romanovsky]

On Wed, Aug 28, 2024 at 02:25:22PM -0700, Feng Wang wrote:
> Hi Leon,

Please don't top-post your replies when you are replying to a mailing
list. It makes it hard to follow the conversation.

> 
> Thank you for your insightful questions and comments.
> 
> Just like in crypto offload mode, now pass SA (Security Association)
> information to the driver in packet offload mode. This helps the
> driver quickly identify the packet's source and destination, rather
> than having to parse the packet itself. The xfrm interface ID is
> especially useful here. As you can see in the kernel code
> (https://elixir.bootlin.com/linux/v6.10/source/net/xfrm/xfrm_policy.c#L1993),
> it's used to differentiate between various policies in complex network
> setups.

Which in-kernel driver use this information?

> 
> During my testing of packet offload mode without the patch, I observed
> that the sec_path was NULL. Consequently, xo was also NULL when
> validate_xmit_xfrm was called, leading to an immediate return at line
> 125 (https://elixir.bootlin.com/linux/v6.10/source/net/xfrm/xfrm_device.c#L125).

It is intentional, because the packet is forwarded to HW as plain text
and it is not offloaded (doesn't have xfrm_offload()).

> 
> Regarding the potential xfrm_state leak, upon further investigation,
> it may not be the case. It seems that both secpath_reset and kfree_skb
> invoke the xfrm_state_put function, which should be responsible for
> releasing the state. The call flow appears to be as follows: kfree_skb
> -> skb_release_all -> skb_release_head_state -> skb_ext_put ->
> skb_ext_put_sp -> xfrm_state_put.

You are trying to make same flow as for crypto, but it is not the same,
in crypto case secpath_reset() was called to release SKB extensions and
perform cleanup, first and only after that new xfrm_state_hold() was
called, but in new code SKB is not reset.

Thanks

> 
> Please let me know if you have any further questions or concerns. I
> appreciate your valuable feedback!
> 
> Feng
> 
> On Wed, Aug 28, 2024 at 4:26 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Wed, Aug 28, 2024 at 07:32:47AM +0200, Steffen Klassert wrote:
> > > On Thu, Aug 22, 2024 at 01:02:52PM -0700, 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 crucial in setups
> > > > with multiple XFRM interfaces where source/destination addresses alone
> > > > can't pinpoint the right policy.
> > > >
> > > > Signed-off-by: wangfe <wangfe@google.com>
> > >
> > > Applied to ipsec-next, thanks Feng!
> >
> > Stephen, can you please explain why do you think that this is correct
> > thing to do?
> >
> > There are no in-tree any drivers which is using this information, and it
> > is unclear to me how state is released and it has controversial code
> > around validity of xfrm_offload() too.
> >
> > For example:
> > +               sp->olen++;
> > +               sp->xvec[sp->len++] = x;
> > +               xfrm_state_hold(x);
> > +
> > +               xo = xfrm_offload(skb);
> > +               if (!xo) { <--- previous code handled this case perfectly in validate_xmit_xfrm
> > +                       secpath_reset(skb);
> > +                       XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> > +                       kfree_skb(skb);
> > +                       return -EINVAL; <--- xfrm state leak
> > +               }
> >
> >
> > Can you please revert/drop this patch for now?
> >
> > Thanks

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

[Feng Wang]

Hi Leon,

Thank you again for your thoughtful questions and comments. I'd like
to provide further clarification and address your points:

SA Information Usage:

There are several instances in the kernel code where it's used, such
as in esp4(6)_offload.c and xfrm.c. This clearly demonstrates how SA
information is used. Moreover, passing this information to the driver
shouldn't negatively impact those drivers that don't require it.
Regarding a driver example, the function mlx5e_ipsec_feature_check
caught my attention.
https://elixir.bootlin.com/linux/v6.10/source/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_rxtx.h#L89)
As you're more familiar with this codebase, I defer to your expertise
on whether it's an appropriate sample. However, the crucial point is
that including this information empowers certain drivers to leverage
it without affecting those that don't need it.

validate_xmit_xfrm Function:
My primary goal in discussing the validate_xmit_xfrm function is to
assure you that my patch maintains the existing packet offload code
flow, avoiding any unintended disruption.

State Release:
I've noticed that secpath_reset() is called before xfrm_output(). The
sequence seems to be: xfrmi_xmit2 -> xfrmi_scrub_packet ->
secpath_reset(), followed by xfrmi_xmit2 calling dst_output, which is
essentially xfrm_output().
I'm also open to moving the xfrm_state_hold(x) after the if (!xo)
check block. This would ensure the state is held only when everything
is ok. I'll gladly make this adjustment if you believe it's the better
option.

Thank you once again for your valuable insights and collaboration.
Your feedback is greatly appreciated!

Feng

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

[Leon Romanovsky]

On Thu, Aug 29, 2024 at 02:19:25PM -0700, Feng Wang wrote:
> Hi Leon,
> 
> Thank you again for your thoughtful questions and comments. I'd like
> to provide further clarification and address your points:
> 
> SA Information Usage:
> 
> There are several instances in the kernel code where it's used, such
> as in esp4(6)_offload.c and xfrm.c. This clearly demonstrates how SA
> information is used. Moreover, passing this information to the driver
> shouldn't negatively impact those drivers that don't require it.
> Regarding a driver example, the function mlx5e_ipsec_feature_check
> caught my attention.
> https://elixir.bootlin.com/linux/v6.10/source/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_rxtx.h#L89)
> As you're more familiar with this codebase, I defer to your expertise
> on whether it's an appropriate sample. 

This function is not involved when packet is going to be offloaded for "IPsec packet offload".

> However, the crucial point is that including this information empowers certain drivers to leverage
> it without affecting those that don't need it.

Can you please provide a list of drivers that will benefit from this change?
Can you give a complete flow (including driver) which didn't work before
and will work after this change?

> 
> validate_xmit_xfrm Function:
> My primary goal in discussing the validate_xmit_xfrm function is to
> assure you that my patch maintains the existing packet offload code
> flow, avoiding any unintended disruption.

The whole idea of packet offload is to skip everything in XFRM stack and
present packet as plain text.

> 
> State Release:
> I've noticed that secpath_reset() is called before xfrm_output(). The
> sequence seems to be: xfrmi_xmit2 -> xfrmi_scrub_packet ->
> secpath_reset(), followed by xfrmi_xmit2 calling dst_output, which is
> essentially xfrm_output().
> I'm also open to moving the xfrm_state_hold(x) after the if (!xo)
> check block. This would ensure the state is held only when everything
> is ok. I'll gladly make this adjustment if you believe it's the better
> option.
> 
> Thank you once again for your valuable insights and collaboration.
> Your feedback is greatly appreciated!
> 
> Feng
> 

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

[Feng Wang]

Hi Leon,

I believe you are right about the mlx5e_ipsec_feature_check function.
And it shows that the driver can indeed make use of the SA
information. Similarly, in packet offload mode, drivers can
potentially leverage this information for their own purposes. The
patch is designed to be non-intrusive, so drivers that don't utilize
this information won't be affected in any way.

I'm also curious about why the mlx driver doesn't seem to use the XFRM
interface ID in the same way that xfrm_policy_match() does.
https://elixir.bootlin.com/linux/v6.10.7/source/net/xfrm/xfrm_policy.c#L1993
This ID is critical in scenarios with multiple IPsec tunnels, where
source and destination addresses alone might not be sufficient to
identify the correct security policy. Perhaps there's a specific
reason or design choice behind this in the mlx driver?

Thank you once again for your valuable insights and collaboration.

Feng

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

[Leon Romanovsky]

On Fri, Aug 30, 2024 at 05:27:29PM -0700, Feng Wang wrote:
> Hi Leon,
> 
> I believe you are right about the mlx5e_ipsec_feature_check function.
> And it shows that the driver can indeed make use of the SA
> information. Similarly, in packet offload mode, drivers can
> potentially leverage this information for their own purposes. The
> patch is designed to be non-intrusive, so drivers that don't utilize
> this information won't be affected in any way.

I asked about examples of such drivers. Can you please provide them?

> 
> I'm also curious about why the mlx driver doesn't seem to use the XFRM
> interface ID in the same way that xfrm_policy_match() does.
> https://elixir.bootlin.com/linux/v6.10.7/source/net/xfrm/xfrm_policy.c#L1993a

HW offload is always last in packet TX traversal and it means that if HW
catches that packet and it meets the HW offload requirements, it will be
encrypted. The main idea is that routing (sending to right if_id) is handled
by the upper layers and HW offload is just a final step.

> This ID is critical in scenarios with multiple IPsec tunnels, where
> source and destination addresses alone might not be sufficient to
> identify the correct security policy. Perhaps there's a specific
> reason or design choice behind this in the mlx driver?

It is not specific to mlx5, but to all HW offload drivers. They should
implement both policy and SA offloading. It is violation of current mailing
list deign to do not offload policy. If you offload both policy and SA, you
won't need if_id at all.

> 
> Thank you once again for your valuable insights and collaboration.
> 
> Feng

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

[Leon Romanovsky]

On Wed, Aug 28, 2024 at 07:32:47AM +0200, Steffen Klassert wrote:
> On Thu, Aug 22, 2024 at 01:02:52PM -0700, 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 crucial in setups
> > with multiple XFRM interfaces where source/destination addresses alone
> > can't pinpoint the right policy.
> > 
> > Signed-off-by: wangfe <wangfe@google.com>
> 
> Applied to ipsec-next, thanks Feng!

Steffen,

What is your position on this patch?
It is the same patch (logically) as the one that was rejected before?
https://lore.kernel.org/all/ZfpnCIv+8eYd7CpO@gauss3.secunet.de/

Thanks

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

[Steffen Klassert]

Sorry for the delay. I'm on vacation, so responses will take a bit
longer during the next two weeks.

On Sat, Aug 31, 2024 at 08:39:34PM +0300, Leon Romanovsky wrote:
> On Wed, Aug 28, 2024 at 07:32:47AM +0200, Steffen Klassert wrote:
> > On Thu, Aug 22, 2024 at 01:02:52PM -0700, 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 crucial in setups
> > > with multiple XFRM interfaces where source/destination addresses alone
> > > can't pinpoint the right policy.
> > > 
> > > Signed-off-by: wangfe <wangfe@google.com>
> > 
> > Applied to ipsec-next, thanks Feng!
> 
> Steffen,
> 
> What is your position on this patch?
> It is the same patch (logically) as the one that was rejected before?
> https://lore.kernel.org/all/ZfpnCIv+8eYd7CpO@gauss3.secunet.de/

This is an infrastructure patch to support routing based IPsec
with xfrm interfaces. I just did not notice it because it was not
mentioned in the commit message of the first patchset. This should have
been included into the packet offload API patchset, but I overlooked
that xfrm interfaces can't work with packet offload mode. The stack
infrastructure should be complete, so that drivers can implement
that without the need to fix the stack before.

In case the patch has issues, we should fix it.

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

[Leon Romanovsky]

On Mon, Sep 02, 2024 at 09:44:24AM +0200, Steffen Klassert wrote:
> Sorry for the delay. I'm on vacation, so responses will take a bit
> longer during the next two weeks.
> 
> On Sat, Aug 31, 2024 at 08:39:34PM +0300, Leon Romanovsky wrote:
> > On Wed, Aug 28, 2024 at 07:32:47AM +0200, Steffen Klassert wrote:
> > > On Thu, Aug 22, 2024 at 01:02:52PM -0700, 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 crucial in setups
> > > > with multiple XFRM interfaces where source/destination addresses alone
> > > > can't pinpoint the right policy.
> > > > 
> > > > Signed-off-by: wangfe <wangfe@google.com>
> > > 
> > > Applied to ipsec-next, thanks Feng!
> > 
> > Steffen,
> > 
> > What is your position on this patch?
> > It is the same patch (logically) as the one that was rejected before?
> > https://lore.kernel.org/all/ZfpnCIv+8eYd7CpO@gauss3.secunet.de/
> 
> This is an infrastructure patch to support routing based IPsec
> with xfrm interfaces. I just did not notice it because it was not
> mentioned in the commit message of the first patchset. This should have
> been included into the packet offload API patchset, but I overlooked
> that xfrm interfaces can't work with packet offload mode. The stack
> infrastructure should be complete, so that drivers can implement
> that without the need to fix the stack before.

Core implementation that is not used by any upstream code is rarely
right thing to do. It is not tested, complicates the code and mostly
overlooked when patches are reviewed. The better way will be to extend
the stack when this feature will be actually used and needed.

IMHO, attempt to enrich core code without showing users of this new flow
is comparable to premature optimization.

And Feng more than once said that this code is for some out-of-tree
driver.

> 
> In case the patch has issues, we should fix it.

Yes, this patch doesn't have in-kernel users.

Thanks

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

[Feng Wang]

This patch simply assigns a value to a field, replicating existing
crypto offload behavior - it's working/tested in that mode. Many
instances within the kernel code utilize this information in different
cases, making the implementation pretty simple and safe.

Hi Leon,

"It is not specific to mlx5, but to all HW offload drivers. They should
implement both policy and SA offloading. It is violation of current mailing
list deign to do not offload policy. If you offload both policy and SA, you
won't need if_id at all."

Could you please clarify why the if_id is unnecessary in scenarios
with hardware offload?

For instance, imagine I have two tunnel sessions sharing the same
source and destination addresses. One tunnel utilizes xfrm ID 1, while
the other uses xfrm ID 2. If a packet is sent out via xfrm ID 1 and
lacks any specific markings, how does the hardware offload determine
that this packet belongs to xfrm ID 1 and not xfrm ID 2? This
distinction is crucial for the hardware to locate the correct
encryption information and encrypt the packet accordingly.

Thanks for your help.

Feng

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

[Leon Romanovsky]

On Tue, Sep 03, 2024 at 11:19:41AM -0700, Feng Wang wrote:
> This patch simply assigns a value to a field, replicating existing
> crypto offload behavior - it's working/tested in that mode. Many
> instances within the kernel code utilize this information in different
> cases, making the implementation pretty simple and safe.

Not really, the thing that you are adding secpath in the place which
wasn't designed to have it, is very problematic. Crypto and packet
offloads are two different things as they treat packets differently.
First one sees them as ESP packets, while the second one sees them as
plain text packets.

> 
> Hi Leon,
> 
> "It is not specific to mlx5, but to all HW offload drivers. They should
> implement both policy and SA offloading. It is violation of current mailing
> list deign to do not offload policy. If you offload both policy and SA, you
> won't need if_id at all."
> 
> Could you please clarify why the if_id is unnecessary in scenarios
> with hardware offload?

I have no objections to support xfrm IDs in the packet offload, my
request is to have upstream driver which uses this feature.

> 
> For instance, imagine I have two tunnel sessions sharing the same
> source and destination addresses. One tunnel utilizes xfrm ID 1, while
> the other uses xfrm ID 2. If a packet is sent out via xfrm ID 1 and
> lacks any specific markings, how does the hardware offload determine
> that this packet belongs to xfrm ID 1 and not xfrm ID 2? This
> distinction is crucial for the hardware to locate the correct
> encryption information and encrypt the packet accordingly.

HW doesn't know about xfrm ID, as this information is kernel specific.
In order HW will use this information someone needs to pass it to HW,
while configuring SA/policy and nothing in the kernel does that.

Thanks

> 
> Thanks for your help.
> 
> Feng

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

[Feng Wang]

Hi Leon,

I'm looking at the MLX5 driver to understand how the SA information is
used. In mlx5e_ipsec_handle_tx_skb(), it appears we might leverage the
current MLX5 implementation to verify the xfrm id.
https://elixir.bootlin.com/linux/v6.10/source/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_rxtx.c#L271

During the mlx5e_xfrm_add_state() function, the xfrm ID (x->if_id) is
passed to the driver along with the associated xfrm_state pointer.
Therefore, by checking the if_id within the skb tx function like
mlx5e_ipsec_handle_tx_skb(), we should be able to demonstrate the use
case effectively.

What’s your opinion?

Thanks for your help.

Feng

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

[Leon Romanovsky]

On Wed, Sep 04, 2024 at 10:41:38AM -0700, Feng Wang wrote:
> Hi Leon,
> 
> I'm looking at the MLX5 driver to understand how the SA information is
> used. In mlx5e_ipsec_handle_tx_skb(), it appears we might leverage the
> current MLX5 implementation to verify the xfrm id.
> https://elixir.bootlin.com/linux/v6.10/source/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_rxtx.c#L271
> 
> During the mlx5e_xfrm_add_state() function, the xfrm ID (x->if_id) is
> passed to the driver along with the associated xfrm_state pointer.
> Therefore, by checking the if_id within the skb tx function like
> mlx5e_ipsec_handle_tx_skb(), we should be able to demonstrate the use
> case effectively.
> 
> What’s your opinion?

Packet offloaded packets don't pass mlx5e_ipsec_handle_tx_skb() because SKB is
treated as plain text and not encrypted.

In order to support this feature in mlx5, you will need to do two things:
1. Create rule which matches x->if_id in mlx5 flow steering, while
creating SAs (see tx_add_rule()->setup_fte_reg_a()).

This register is used in the transmit steering tables, and is loaded with
the value of flow_table_metadata field in the Ethernet Segment of the WQE.

2. Set x->if_id from SKB in flow_table_metadata to allow HW to catch
these packets. It means change mlx5e datapath to set this value from
SKB.

The first item is easy, just move setup_fte_reg_a() to the right place,
but the second one is more complex as whole packet offload assumption
that we are working with plain text packets.

I'm not even talking about eswitch mode, which will bring more
complexity.

Thanks

> 
> Thanks for your help.
> 
> Feng

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

[Feng Wang]

Hi Leon,

I'm currently reading the mlx5e_xmit() function to understand the MLX5
datapath. I'm particularly interested in understanding how packet
offload mode functions.

Could you please clarify if CONFIG_MLX5_EN_IPSEC needs to be defined
when MLX5 employs packet offload mode? Additionally, are there any
specific flags associated with packet offload that I should be mindful
of?

Any guidance would be greatly appreciated.

Thanks, Feng

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

[Steffen Klassert]

On Mon, Sep 02, 2024 at 12:44:52PM +0300, Leon Romanovsky wrote:
> On Mon, Sep 02, 2024 at 09:44:24AM +0200, Steffen Klassert wrote:
> > > 
> > > Steffen,
> > > 
> > > What is your position on this patch?
> > > It is the same patch (logically) as the one that was rejected before?
> > > https://lore.kernel.org/all/ZfpnCIv+8eYd7CpO@gauss3.secunet.de/
> > 
> > This is an infrastructure patch to support routing based IPsec
> > with xfrm interfaces. I just did not notice it because it was not
> > mentioned in the commit message of the first patchset. This should have
> > been included into the packet offload API patchset, but I overlooked
> > that xfrm interfaces can't work with packet offload mode. The stack
> > infrastructure should be complete, so that drivers can implement
> > that without the need to fix the stack before.
> 
> Core implementation that is not used by any upstream code is rarely
> right thing to do. It is not tested, complicates the code and mostly
> overlooked when patches are reviewed. The better way will be to extend
> the stack when this feature will be actually used and needed.

This is our tradeoff, an API should be fully designed from the
beginning, everything else is bad design and will likely result
in band aids (as it happens here). The API can be connected to
netdevsim to test it.

Currently the combination of xfrm interfaces and packet offload
is just broken. Unfortunalely this patch does not fix it.

I think we need to do three things:

- Fix xfrm interfaces + packet offload combination

- Extend netdevsim to support packet offload

- Extend the API for xfrm interfaces (and everything
  else we forgot).

> IMHO, attempt to enrich core code without showing users of this new flow
> is comparable to premature optimization.
> 
> And Feng more than once said that this code is for some out-of-tree
> driver.

It is an API, so everyone can use it.

Anyway, I'm thinking about reverting it for now and do it right
in the next development cycle.

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

[Steffen Klassert]

On Mon, Sep 09, 2024 at 11:09:06AM +0200, Steffen Klassert wrote:
> On Mon, Sep 02, 2024 at 12:44:52PM +0300, Leon Romanovsky wrote:
> > On Mon, Sep 02, 2024 at 09:44:24AM +0200, Steffen Klassert wrote:
> 
> Anyway, I'm thinking about reverting it for now and do it right
> in the next development cycle.

I finally decided to revert it. Let's work on it after the next
merge window.

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

[Leon Romanovsky]

On Mon, Sep 09, 2024 at 11:09:05AM +0200, Steffen Klassert wrote:
> On Mon, Sep 02, 2024 at 12:44:52PM +0300, Leon Romanovsky wrote:
> > On Mon, Sep 02, 2024 at 09:44:24AM +0200, Steffen Klassert wrote:
> > > > 
> > > > Steffen,
> > > > 
> > > > What is your position on this patch?
> > > > It is the same patch (logically) as the one that was rejected before?
> > > > https://lore.kernel.org/all/ZfpnCIv+8eYd7CpO@gauss3.secunet.de/
> > > 
> > > This is an infrastructure patch to support routing based IPsec
> > > with xfrm interfaces. I just did not notice it because it was not
> > > mentioned in the commit message of the first patchset. This should have
> > > been included into the packet offload API patchset, but I overlooked
> > > that xfrm interfaces can't work with packet offload mode. The stack
> > > infrastructure should be complete, so that drivers can implement
> > > that without the need to fix the stack before.
> > 
> > Core implementation that is not used by any upstream code is rarely
> > right thing to do. It is not tested, complicates the code and mostly
> > overlooked when patches are reviewed. The better way will be to extend
> > the stack when this feature will be actually used and needed.
> 
> This is our tradeoff, an API should be fully designed from the
> beginning, everything else is bad design and will likely result
> in band aids (as it happens here). The API can be connected to
> netdevsim to test it.
> 
> Currently the combination of xfrm interfaces and packet offload
> is just broken. 

I don't think that it is broken. It is just not implemented. XFRM
interfaces are optional field, which is not really popular in the
field.

> Unfortunalely this patch does not fix it.
> 
> I think we need to do three things:
> 
> - Fix xfrm interfaces + packet offload combination
> 
> - Extend netdevsim to support packet offload
> 
> - Extend the API for xfrm interfaces (and everything
>   else we forgot).

This is the most challenging part. It is not clear what should
we extend if customers are not asking for it and they are extremely
happy with the current IPsec packet offload state.

BTW, I'm aware of one gap, which is not clear how to handle, and
it is combination of policy sockets and offload.

> 
> > IMHO, attempt to enrich core code without showing users of this new flow
> > is comparable to premature optimization.
> > 
> > And Feng more than once said that this code is for some out-of-tree
> > driver.
> 
> It is an API, so everyone can use it.

Of course, as long as in-kernel user exists.

Thanks

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

[Feng Wang]

Hi Steffen,

Can you reconsider the revert of the CL? Based on our discussion, I
believe we've reached a consensus that the xfrm id is necessary. If
some customers prefer not to utilize it, we can extend the offload
flag (something like XFRM_DEV_OFFLOAD_FLAG_ACQ) to manage this
behavior. The default setting would remain consistent with the current
implementation.

Thank you!

Feng

On Wed, Sep 11, 2024 at 3:40 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Mon, Sep 09, 2024 at 11:09:05AM +0200, Steffen Klassert wrote:
> > On Mon, Sep 02, 2024 at 12:44:52PM +0300, Leon Romanovsky wrote:
> > > On Mon, Sep 02, 2024 at 09:44:24AM +0200, Steffen Klassert wrote:
> > > > >
> > > > > Steffen,
> > > > >
> > > > > What is your position on this patch?
> > > > > It is the same patch (logically) as the one that was rejected before?
> > > > > https://lore.kernel.org/all/ZfpnCIv+8eYd7CpO@gauss3.secunet.de/
> > > >
> > > > This is an infrastructure patch to support routing based IPsec
> > > > with xfrm interfaces. I just did not notice it because it was not
> > > > mentioned in the commit message of the first patchset. This should have
> > > > been included into the packet offload API patchset, but I overlooked
> > > > that xfrm interfaces can't work with packet offload mode. The stack
> > > > infrastructure should be complete, so that drivers can implement
> > > > that without the need to fix the stack before.
> > >
> > > Core implementation that is not used by any upstream code is rarely
> > > right thing to do. It is not tested, complicates the code and mostly
> > > overlooked when patches are reviewed. The better way will be to extend
> > > the stack when this feature will be actually used and needed.
> >
> > This is our tradeoff, an API should be fully designed from the
> > beginning, everything else is bad design and will likely result
> > in band aids (as it happens here). The API can be connected to
> > netdevsim to test it.
> >
> > Currently the combination of xfrm interfaces and packet offload
> > is just broken.
>
> I don't think that it is broken. It is just not implemented. XFRM
> interfaces are optional field, which is not really popular in the
> field.
>
> > Unfortunalely this patch does not fix it.
> >
> > I think we need to do three things:
> >
> > - Fix xfrm interfaces + packet offload combination
> >
> > - Extend netdevsim to support packet offload
> >
> > - Extend the API for xfrm interfaces (and everything
> >   else we forgot).
>
> This is the most challenging part. It is not clear what should
> we extend if customers are not asking for it and they are extremely
> happy with the current IPsec packet offload state.
>
> BTW, I'm aware of one gap, which is not clear how to handle, and
> it is combination of policy sockets and offload.
>
> >
> > > IMHO, attempt to enrich core code without showing users of this new flow
> > > is comparable to premature optimization.
> > >
> > > And Feng more than once said that this code is for some out-of-tree
> > > driver.
> >
> > It is an API, so everyone can use it.
>
> Of course, as long as in-kernel user exists.
>
> Thanks

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

[Leon Romanovsky]

On Wed, Sep 11, 2024 at 04:43:55PM -0700, Feng Wang wrote:
> Hi Steffen,
> 
> Can you reconsider the revert of the CL? Based on our discussion, I
> believe we've reached a consensus that the xfrm id is necessary. 

It is only half of the consensus, the other half is that upstream
customer should exist.

Thanks

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

[Steffen Klassert]

On Wed, Sep 11, 2024 at 04:43:55PM -0700, Feng Wang wrote:
> Hi Steffen,
> 
> Can you reconsider the revert of the CL? Based on our discussion, I
> believe we've reached a consensus that the xfrm id is necessary.

I did not say it is necessary, all I want is a complete API that
exposes all xfrm stack features correctly to HW.

> If
> some customers prefer not to utilize it, we can extend the offload
> flag (something like XFRM_DEV_OFFLOAD_FLAG_ACQ) to manage this
> behavior. The default setting would remain consistent with the current
> implementation.

No new flags please.

The easiest thing would be to upstream your driver, that is the
prefered way and would just end this discussion.

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

[Steffen Klassert]

On Wed, Sep 11, 2024 at 01:40:40PM +0300, Leon Romanovsky wrote:
> On Mon, Sep 09, 2024 at 11:09:05AM +0200, Steffen Klassert wrote:
> > On Mon, Sep 02, 2024 at 12:44:52PM +0300, Leon Romanovsky wrote:
> > > On Mon, Sep 02, 2024 at 09:44:24AM +0200, Steffen Klassert wrote:
> > > > > 
> > > > > Steffen,
> > > > > 
> > > > > What is your position on this patch?
> > > > > It is the same patch (logically) as the one that was rejected before?
> > > > > https://lore.kernel.org/all/ZfpnCIv+8eYd7CpO@gauss3.secunet.de/
> > > > 
> > > > This is an infrastructure patch to support routing based IPsec
> > > > with xfrm interfaces. I just did not notice it because it was not
> > > > mentioned in the commit message of the first patchset. This should have
> > > > been included into the packet offload API patchset, but I overlooked
> > > > that xfrm interfaces can't work with packet offload mode. The stack
> > > > infrastructure should be complete, so that drivers can implement
> > > > that without the need to fix the stack before.
> > > 
> > > Core implementation that is not used by any upstream code is rarely
> > > right thing to do. It is not tested, complicates the code and mostly
> > > overlooked when patches are reviewed. The better way will be to extend
> > > the stack when this feature will be actually used and needed.
> > 
> > This is our tradeoff, an API should be fully designed from the
> > beginning, everything else is bad design and will likely result
> > in band aids (as it happens here). The API can be connected to
> > netdevsim to test it.
> > 
> > Currently the combination of xfrm interfaces and packet offload
> > is just broken. 
> 
> I don't think that it is broken.

I don't see anything that prevents you from offloading a SA
with an xfrm interface ID. The binding to the interface is
just ignored in that case.

> It is just not implemented. XFRM
> interfaces are optional field, which is not really popular in the
> field.

It is very popular, I know of more than a billion devices that
are using xfrm interfaces.

> 
> > Unfortunalely this patch does not fix it.
> > 
> > I think we need to do three things:
> > 
> > - Fix xfrm interfaces + packet offload combination
> > 
> > - Extend netdevsim to support packet offload
> > 
> > - Extend the API for xfrm interfaces (and everything
> >   else we forgot).
> 
> This is the most challenging part. It is not clear what should
> we extend if customers are not asking for it and they are extremely
> happy with the current IPsec packet offload state.

We just need to push the information down to the driver,
and reject the offload if not supported.

> 
> BTW, I'm aware of one gap, which is not clear how to handle, and
> it is combination of policy sockets and offload.

Socket policies are a bit special as they are configured by
the application that uses the socket. I don't think that
we can even configure offload for a socket policy.

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

[Feng Wang]

Hi Steffen,

The easiest thing would be to upstream your driver, that is the
prefered way and would just end this discussion.

I will try to upstream the xfrm interface id handling code to
netdevsim, thus it will have an in-driver implementation.

Thanks,

Feng

On Tue, Sep 24, 2024 at 3:34 AM Steffen Klassert
<steffen.klassert@secunet.com> wrote:
>
> On Wed, Sep 11, 2024 at 01:40:40PM +0300, Leon Romanovsky wrote:
> > On Mon, Sep 09, 2024 at 11:09:05AM +0200, Steffen Klassert wrote:
> > > On Mon, Sep 02, 2024 at 12:44:52PM +0300, Leon Romanovsky wrote:
> > > > On Mon, Sep 02, 2024 at 09:44:24AM +0200, Steffen Klassert wrote:
> > > > > >
> > > > > > Steffen,
> > > > > >
> > > > > > What is your position on this patch?
> > > > > > It is the same patch (logically) as the one that was rejected before?
> > > > > > https://lore.kernel.org/all/ZfpnCIv+8eYd7CpO@gauss3.secunet.de/
> > > > >
> > > > > This is an infrastructure patch to support routing based IPsec
> > > > > with xfrm interfaces. I just did not notice it because it was not
> > > > > mentioned in the commit message of the first patchset. This should have
> > > > > been included into the packet offload API patchset, but I overlooked
> > > > > that xfrm interfaces can't work with packet offload mode. The stack
> > > > > infrastructure should be complete, so that drivers can implement
> > > > > that without the need to fix the stack before.
> > > >
> > > > Core implementation that is not used by any upstream code is rarely
> > > > right thing to do. It is not tested, complicates the code and mostly
> > > > overlooked when patches are reviewed. The better way will be to extend
> > > > the stack when this feature will be actually used and needed.
> > >
> > > This is our tradeoff, an API should be fully designed from the
> > > beginning, everything else is bad design and will likely result
> > > in band aids (as it happens here). The API can be connected to
> > > netdevsim to test it.
> > >
> > > Currently the combination of xfrm interfaces and packet offload
> > > is just broken.
> >
> > I don't think that it is broken.
>
> I don't see anything that prevents you from offloading a SA
> with an xfrm interface ID. The binding to the interface is
> just ignored in that case.
>
> > It is just not implemented. XFRM
> > interfaces are optional field, which is not really popular in the
> > field.
>
> It is very popular, I know of more than a billion devices that
> are using xfrm interfaces.
>
> >
> > > Unfortunalely this patch does not fix it.
> > >
> > > I think we need to do three things:
> > >
> > > - Fix xfrm interfaces + packet offload combination
> > >
> > > - Extend netdevsim to support packet offload
> > >
> > > - Extend the API for xfrm interfaces (and everything
> > >   else we forgot).
> >
> > This is the most challenging part. It is not clear what should
> > we extend if customers are not asking for it and they are extremely
> > happy with the current IPsec packet offload state.
>
> We just need to push the information down to the driver,
> and reject the offload if not supported.
>
> >
> > BTW, I'm aware of one gap, which is not clear how to handle, and
> > it is combination of policy sockets and offload.
>
> Socket policies are a bit special as they are configured by
> the application that uses the socket. I don't think that
> we can even configure offload for a socket policy.
>

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

[Steffen Klassert]

Please don't top post!

On Tue, Sep 24, 2024 at 10:57:12AM -0700, Feng Wang wrote:
> Hi Steffen,
> 
> The easiest thing would be to upstream your driver, that is the
> prefered way and would just end this discussion.
> 
> I will try to upstream the xfrm interface id handling code to
> netdevsim, thus it will have an in-driver implementation.

Netdevsim is just the second best option and still might
lead to discussion.

Upstream the google driver that actually uses it, this _is_
the prefered way.

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

[Leon Romanovsky]

On Tue, Sep 24, 2024 at 08:10:41PM +0200, Steffen Klassert wrote:
> Please don't top post!
> 
> On Tue, Sep 24, 2024 at 10:57:12AM -0700, Feng Wang wrote:
> > Hi Steffen,
> > 
> > The easiest thing would be to upstream your driver, that is the
> > prefered way and would just end this discussion.
> > 
> > I will try to upstream the xfrm interface id handling code to
> > netdevsim, thus it will have an in-driver implementation.
> 
> Netdevsim is just the second best option and still might
> lead to discussion.

netdevsim should come with relevant selftests, otherwise this
new feature won't be tested properly. We can't rely on someone
out-of-tree to test it.

> 
> Upstream the google driver that actually uses it, this _is_
> the prefered way.

+1

> 

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

[Leon Romanovsky]

On Tue, Sep 24, 2024 at 12:34:32PM +0200, Steffen Klassert wrote:
> On Wed, Sep 11, 2024 at 01:40:40PM +0300, Leon Romanovsky wrote:
> > On Mon, Sep 09, 2024 at 11:09:05AM +0200, Steffen Klassert wrote:
> > > On Mon, Sep 02, 2024 at 12:44:52PM +0300, Leon Romanovsky wrote:
> > > > On Mon, Sep 02, 2024 at 09:44:24AM +0200, Steffen Klassert wrote:
> > > > > > 
> > > > > > Steffen,
> > > > > > 
> > > > > > What is your position on this patch?
> > > > > > It is the same patch (logically) as the one that was rejected before?
> > > > > > https://lore.kernel.org/all/ZfpnCIv+8eYd7CpO@gauss3.secunet.de/
> > > > > 
> > > > > This is an infrastructure patch to support routing based IPsec
> > > > > with xfrm interfaces. I just did not notice it because it was not
> > > > > mentioned in the commit message of the first patchset. This should have
> > > > > been included into the packet offload API patchset, but I overlooked
> > > > > that xfrm interfaces can't work with packet offload mode. The stack
> > > > > infrastructure should be complete, so that drivers can implement
> > > > > that without the need to fix the stack before.
> > > > 
> > > > Core implementation that is not used by any upstream code is rarely
> > > > right thing to do. It is not tested, complicates the code and mostly
> > > > overlooked when patches are reviewed. The better way will be to extend
> > > > the stack when this feature will be actually used and needed.
> > > 
> > > This is our tradeoff, an API should be fully designed from the
> > > beginning, everything else is bad design and will likely result
> > > in band aids (as it happens here). The API can be connected to
> > > netdevsim to test it.
> > > 
> > > Currently the combination of xfrm interfaces and packet offload
> > > is just broken. 
> > 
> > I don't think that it is broken.
> 
> I don't see anything that prevents you from offloading a SA
> with an xfrm interface ID. The binding to the interface is
> just ignored in that case.
> 
> > It is just not implemented. XFRM
> > interfaces are optional field, which is not really popular in the
> > field.
> 
> It is very popular, I know of more than a billion devices that
> are using xfrm interfaces.

We see different parts of "the field". In my case it it enterprise/cloud
world, and I can say same sentence as you with "are NOT using ..."
words instead. This is why so important to see google's driver (which is Android)
to understand the real need from this feature.

> 
> > 
> > > Unfortunalely this patch does not fix it.
> > > 
> > > I think we need to do three things:
> > > 
> > > - Fix xfrm interfaces + packet offload combination
> > > 
> > > - Extend netdevsim to support packet offload
> > > 
> > > - Extend the API for xfrm interfaces (and everything
> > >   else we forgot).
> > 
> > This is the most challenging part. It is not clear what should
> > we extend if customers are not asking for it and they are extremely
> > happy with the current IPsec packet offload state.
> 
> We just need to push the information down to the driver,
> and reject the offload if not supported.

Yes and in addition to that it will be beneficial do not add this information
to SKB if it won't be used.

> 
> > 
> > BTW, I'm aware of one gap, which is not clear how to handle, and
> > it is combination of policy sockets and offload.
> 
> Socket policies are a bit special as they are configured by
> the application that uses the socket. I don't think that
> we can even configure offload for a socket policy.

One of the idea is to iterate over all devices and check if they
support offload, if yes, then offload, if not, then fallback
to software for that device. This is just rough idea with many
caveats.

Thanks

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

[Steffen Klassert]

On Thu, Aug 22, 2024 at 01:02:52PM -0700, 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 crucial in setups
> with multiple XFRM interfaces where source/destination addresses alone
> can't pinpoint the right policy.
> 
> Signed-off-by: wangfe <wangfe@google.com>
> ---
> 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/
> ---
>  net/xfrm/xfrm_output.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> 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);

Maybe we should do that only if if we really use xfrm interfaces,
i.e. if xfrm if_id is not zero.

[PATCH] 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>
---
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            | 21 ++++++++++++++++++++
 3 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/net/netdevsim/ipsec.c b/drivers/net/netdevsim/ipsec.c
index f0d58092e7e9..afd2005bf7a8 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)
@@ -237,6 +253,7 @@ bool nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
 	struct xfrm_state *xs;
 	struct nsim_sa *tsa;
 	u32 sa_idx;
+	struct xfrm_offload *xo;
 
 	/* do we even need to check this packet? */
 	if (!sp)
@@ -272,6 +289,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..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.277.g8800431eea-goog

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

[Leon Romanovsky]

On Tue, Nov 12, 2024 at 11:22:49AM -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>
> ---
> 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            | 21 ++++++++++++++++++++
>  3 files changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/netdevsim/ipsec.c b/drivers/net/netdevsim/ipsec.c
> index f0d58092e7e9..afd2005bf7a8 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)
> @@ -237,6 +253,7 @@ bool nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
>  	struct xfrm_state *xs;
>  	struct nsim_sa *tsa;
>  	u32 sa_idx;
> +	struct xfrm_offload *xo;
>  
>  	/* do we even need to check this packet? */
>  	if (!sp)
> @@ -272,6 +289,19 @@ bool nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
>  		return false;
>  	}
>  
> +	if (xs->if_id) {

If you are checking this, you will need to make sure that XFRM_XMIT is
set when if_id != 0. It is not how it is implemented now.

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

Not an expert and just looked how XFRM_XMIT is assigned in other places,
and simple skb_ext_add(skb, SKB_EXT_SEC_PATH) is used, why is it
different here?

The additional issue is that you are adding extension and flag to all
offloaded packets, which makes me wonder what to do with this check in
validate_xmit_xfrm():

  136         /* The packet was sent to HW IPsec packet offload engine,
  137          * but to wrong device. Drop the packet, so it won't skip
  138          * XFRM stack.
  139          */
  140         if (x->xso.type == XFRM_DEV_OFFLOAD_PACKET && x->xso.dev != dev) {
  141                 kfree_skb(skb);
  142                 dev_core_stats_tx_dropped_inc(dev);
  143                 return NULL;
  144         }

Also because you are adding it to all packets, you are adding extra
overhead for all users, even these who didn't ask this if_id thing.

And again, you should block creation of SAs with if_id != 0 for already
existing in-kernel IPsec implementations.

Thanks

> +
> +		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.277.g8800431eea-goog
> 
> 

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

[Feng Wang]

Hi Leon,

If you are concerned about the case when if_id is 0,   I can make the
change to add the SA only when if_id is non-zero in the .  When the
XFRM_XMIT flag is set,  validate_xmit_xfrm will return immediately,
there is no need to check extension and flag. Are you ok with that?

Thanks,

Feng

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

[Leon Romanovsky]

On Mon, Nov 18, 2024 at 11:28:01AM -0800, Feng Wang wrote:
> Hi Leon,
> 
> If you are concerned about the case when if_id is 0,   I can make the
> change to add the SA only when if_id is non-zero in the .  When the
> XFRM_XMIT flag is set,  validate_xmit_xfrm will return immediately,
> there is no need to check extension and flag. Are you ok with that?
> 
> Thanks,
> 
> Feng

Hi, 

You was already asked to respect mailing list email etiquette and don't
respond in top-post format. I would like to add to that request, please
reply in-line and don't trim whole message as it is impossible to keep
context in your emails.

For now, I will just ignore your emails.

Thanks

> 

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

[Feng Wang]

Hi Leon,

Sorry about the format issue,  I thought I used the correct format.
Now I try to do it again, if it still has an issue, please let me
know.
I will create a new patch based on your comments.

Thanks,

Feng

On Sat, Nov 16, 2024 at 10:22 PM Leon Romanovsky <leonro@nvidia.com> wrote:
>
> On Tue, Nov 12, 2024 at 11:22:49AM -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>
> > ---
> > 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            | 21 ++++++++++++++++++++
> >  3 files changed, 53 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/netdevsim/ipsec.c b/drivers/net/netdevsim/ipsec.c
> > index f0d58092e7e9..afd2005bf7a8 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)
> > @@ -237,6 +253,7 @@ bool nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
> >       struct xfrm_state *xs;
> >       struct nsim_sa *tsa;
> >       u32 sa_idx;
> > +     struct xfrm_offload *xo;
> >
> >       /* do we even need to check this packet? */
> >       if (!sp)
> > @@ -272,6 +289,19 @@ bool nsim_ipsec_tx(struct netdevsim *ns, struct sk_buff *skb)
> >               return false;
> >       }
> >
> > +     if (xs->if_id) {
>
> If you are checking this, you will need to make sure that XFRM_XMIT is
> set when if_id != 0. It is not how it is implemented now.
>
I will change the code to add the SA only when if_id is non-zero,
then it will make sense.

> > +             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..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);
>
> Not an expert and just looked how XFRM_XMIT is assigned in other places,
> and simple skb_ext_add(skb, SKB_EXT_SEC_PATH) is used, why is it
> different here?
>
> The additional issue is that you are adding extension and flag to all
> offloaded packets, which makes me wonder what to do with this check in
> validate_xmit_xfrm():
>
Set the XFRM_XMIT flag makes validate_xmit_xfrm returns immediately
without need to do any further check.

>   136         /* The packet was sent to HW IPsec packet offload engine,
>   137          * but to wrong device. Drop the packet, so it won't skip
>   138          * XFRM stack.
>   139          */
>   140         if (x->xso.type == XFRM_DEV_OFFLOAD_PACKET && x->xso.dev != dev) {
>   141                 kfree_skb(skb);
>   142                 dev_core_stats_tx_dropped_inc(dev);
>   143                 return NULL;
>   144         }
>
> Also because you are adding it to all packets, you are adding extra
> overhead for all users, even these who didn't ask this if_id thing.
>
> And again, you should block creation of SAs with if_id != 0 for already
> existing in-kernel IPsec implementations.
>
> Thanks
>
I will add SA information only when id is non-zero,  thus no overhead
for other uses, and no need to block creating of SAs with if_id is 0,

> > +
> > +             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.277.g8800431eea-goog
> >
> >

[PATCH] 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.

Signed-off-by: wangfe <wangfe@google.com>
---
 net/xfrm/xfrm_output.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

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.46.0.rc2.264.g509ed76dc8-goog

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

[Steffen Klassert]

On Mon, Aug 12, 2024 at 11:23:17AM -0700, 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.

Please explain in the commit message _why_ we need that change.

Thanks!

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

[Feng Wang]

Hi Steffen,

I have added the reason why SA info is needed in the commit message.
The new patch is
https://patchwork.kernel.org/project/netdevbpf/patch/20240822200252.472298-1-wangfe@google.com/

Thanks for your review.

Feng


On Sun, Aug 18, 2024 at 11:06 PM Steffen Klassert
<steffen.klassert@secunet.com> wrote:
>
> On Mon, Aug 12, 2024 at 11:23:17AM -0700, 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.
>
> Please explain in the commit message _why_ we need that change.
>
> Thanks!