[PATCH ipsec] xfrm: fix offloading of cross-family tunnels

[PATCH ipsec] xfrm: fix offloading of cross-family tunnels

[Sabrina Dubroca]

Xiumei reported a regression in IPsec offload tests over xfrmi, where
IPv6 over IPv4 tunnels are no longer offloaded after commit
cc18f482e8b6 ("xfrm: provide common xdo_dev_offload_ok callback
implementation").

Commit cc18f482e8b6 added a generic version of existing checks
attempting to prevent packets with IPv4 options or IPv6 extension
headers from being sent to HW that doesn't support offloading such
packets. The check mistakenly uses x->props.family (the outer family)
to determine the inner packet's family and verify if
options/extensions are present.

In the case of IPv6 over IPv4, the check compares some of the traffic
class bits to the expected no-options ihl value (5). The original
check was introduced in commit 2ac9cfe78223 ("net/mlx5e: IPSec, Add
Innova IPSec offload TX data path"), and then duplicated in the other
drivers. Before commit cc18f482e8b6, the loose check (ihl > 5) passed
because those traffic class bits were not set to a value that
triggered the no-offload codepath. Packets with options/extension
headers that should have been handled in SW went through the offload
path, and were likely dropped by the NIC or incorrectly
processed. Since commit cc18f482e8b6, the check is now strict (ihl !=
5), and in a basic setup (no traffic class configured), all packets go
through the no-offload codepath.

The commits that introduced the incorrect family checks in each driver
are:
2ac9cfe78223 ("net/mlx5e: IPSec, Add Innova IPSec offload TX data path")
8362ea16f69f ("crypto: chcr - ESN for Inline IPSec Tx")
859a497fe80c ("nfp: implement xfrm callbacks and expose ipsec offload feature to upper layer")
32188be805d0 ("cn10k-ipsec: Allow ipsec crypto offload for skb with SA")
[ixgbe/ixgbevf commits are ignored, as that HW does not support tunnel
mode, thus no cross-family setups are possible]

Fixes: cc18f482e8b6 ("xfrm: provide common xdo_dev_offload_ok callback implementation")
Reported-by: Xiumei Mu <xmu@redhat.com>
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/xfrm/xfrm_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index c7a1f080d2de..44b9de6e4e77 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -438,7 +438,7 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x)
 
 	check_tunnel_size = x->xso.type == XFRM_DEV_OFFLOAD_PACKET &&
 			    x->props.mode == XFRM_MODE_TUNNEL;
-	switch (x->props.family) {
+	switch (x->inner_mode.family) {
 	case AF_INET:
 		/* Check for IPv4 options */
 		if (ip_hdr(skb)->ihl != 5)
-- 
2.50.0

Re: [PATCH ipsec] xfrm: fix offloading of cross-family tunnels

[Leon Romanovsky]

On Mon, Aug 25, 2025 at 02:50:23PM +0200, Sabrina Dubroca wrote:
> Xiumei reported a regression in IPsec offload tests over xfrmi, where
> IPv6 over IPv4 tunnels are no longer offloaded after commit
> cc18f482e8b6 ("xfrm: provide common xdo_dev_offload_ok callback
> implementation").

What does it mean "tunnels not offloaded"? xdo_dev_offload_ok()
participates in data path and influences packet processing itself,
but not if tunnel offloaded or not.

Also what type of "offload" are you talking? Crypto or packet?

> 
> Commit cc18f482e8b6 added a generic version of existing checks
> attempting to prevent packets with IPv4 options or IPv6 extension
> headers from being sent to HW that doesn't support offloading such
> packets. The check mistakenly uses x->props.family (the outer family)
> to determine the inner packet's family and verify if
> options/extensions are present.

This is how ALL implementations did, so I'm not agree with claimed Fixes
tag (it it not important).

> 
> In the case of IPv6 over IPv4, the check compares some of the traffic
> class bits to the expected no-options ihl value (5). The original
> check was introduced in commit 2ac9cfe78223 ("net/mlx5e: IPSec, Add
> Innova IPSec offload TX data path"), and then duplicated in the other
> drivers. Before commit cc18f482e8b6, the loose check (ihl > 5) passed
> because those traffic class bits were not set to a value that
> triggered the no-offload codepath. Packets with options/extension
> headers that should have been handled in SW went through the offload
> path, and were likely dropped by the NIC or incorrectly
> processed.

The latter is more correct, so it raises question against which
in-kernel driver were these xfrmi tests performed?


> Since commit cc18f482e8b6, the check is now strict (ihl !=
> 5), and in a basic setup (no traffic class configured), all packets go
> through the no-offload codepath.
> 
> The commits that introduced the incorrect family checks in each driver
> are:
> 2ac9cfe78223 ("net/mlx5e: IPSec, Add Innova IPSec offload TX data path")
> 8362ea16f69f ("crypto: chcr - ESN for Inline IPSec Tx")
> 859a497fe80c ("nfp: implement xfrm callbacks and expose ipsec offload feature to upper layer")
> 32188be805d0 ("cn10k-ipsec: Allow ipsec crypto offload for skb with SA")
> [ixgbe/ixgbevf commits are ignored, as that HW does not support tunnel
> mode, thus no cross-family setups are possible]
> 
> Fixes: cc18f482e8b6 ("xfrm: provide common xdo_dev_offload_ok callback implementation")
> Reported-by: Xiumei Mu <xmu@redhat.com>
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> ---
>  net/xfrm/xfrm_device.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> index c7a1f080d2de..44b9de6e4e77 100644
> --- a/net/xfrm/xfrm_device.c
> +++ b/net/xfrm/xfrm_device.c
> @@ -438,7 +438,7 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x)
>  
>  	check_tunnel_size = x->xso.type == XFRM_DEV_OFFLOAD_PACKET &&
>  			    x->props.mode == XFRM_MODE_TUNNEL;
> -	switch (x->props.family) {
> +	switch (x->inner_mode.family) {

Will it work for transport mode too? We are taking this path both for
tunnel and transport modes.

Thanks

>  	case AF_INET:
>  		/* Check for IPv4 options */
>  		if (ip_hdr(skb)->ihl != 5)
> -- 
> 2.50.0
> 
> 

Re: [PATCH ipsec] xfrm: fix offloading of cross-family tunnels

[Yanjun.Zhu]

On 8/25/25 5:50 AM, Sabrina Dubroca wrote:
> Xiumei reported a regression in IPsec offload tests over xfrmi, where
> IPv6 over IPv4 tunnels are no longer offloaded after commit
> cc18f482e8b6 ("xfrm: provide common xdo_dev_offload_ok callback
> implementation").
>
> Commit cc18f482e8b6 added a generic version of existing checks
> attempting to prevent packets with IPv4 options or IPv6 extension
> headers from being sent to HW that doesn't support offloading such
> packets. The check mistakenly uses x->props.family (the outer family)
> to determine the inner packet's family and verify if
> options/extensions are present.
>
> In the case of IPv6 over IPv4, the check compares some of the traffic
> class bits to the expected no-options ihl value (5). The original
> check was introduced in commit 2ac9cfe78223 ("net/mlx5e: IPSec, Add
> Innova IPSec offload TX data path"), and then duplicated in the other
> drivers. Before commit cc18f482e8b6, the loose check (ihl > 5) passed
> because those traffic class bits were not set to a value that
> triggered the no-offload codepath. Packets with options/extension
> headers that should have been handled in SW went through the offload
> path, and were likely dropped by the NIC or incorrectly
> processed. Since commit cc18f482e8b6, the check is now strict (ihl !=
> 5), and in a basic setup (no traffic class configured), all packets go
> through the no-offload codepath.
>
> The commits that introduced the incorrect family checks in each driver
> are:
> 2ac9cfe78223 ("net/mlx5e: IPSec, Add Innova IPSec offload TX data path")
> 8362ea16f69f ("crypto: chcr - ESN for Inline IPSec Tx")
> 859a497fe80c ("nfp: implement xfrm callbacks and expose ipsec offload feature to upper layer")
> 32188be805d0 ("cn10k-ipsec: Allow ipsec crypto offload for skb with SA")
> [ixgbe/ixgbevf commits are ignored, as that HW does not support tunnel
> mode, thus no cross-family setups are possible]
>
> Fixes: cc18f482e8b6 ("xfrm: provide common xdo_dev_offload_ok callback implementation")
> Reported-by: Xiumei Mu <xmu@redhat.com>
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Thanks. I applied this commit and made tests in my local hosts. I can 
confirm that ipv6 over ipv4 packets can be offloaded into HW. About 
replacing props with inner_mode, I am fine with it.

Thus,

Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>

Zhu Yanjun

> ---
>   net/xfrm/xfrm_device.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> index c7a1f080d2de..44b9de6e4e77 100644
> --- a/net/xfrm/xfrm_device.c
> +++ b/net/xfrm/xfrm_device.c
> @@ -438,7 +438,7 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x)
>   
>   	check_tunnel_size = x->xso.type == XFRM_DEV_OFFLOAD_PACKET &&
>   			    x->props.mode == XFRM_MODE_TUNNEL;
> -	switch (x->props.family) {
> +	switch (x->inner_mode.family) {
>   	case AF_INET:
>   		/* Check for IPv4 options */
>   		if (ip_hdr(skb)->ihl != 5)

Re: [PATCH ipsec] xfrm: fix offloading of cross-family tunnels

[Sabrina Dubroca]

2025-09-09, 12:23:15 +0300, Leon Romanovsky wrote:
> On Mon, Aug 25, 2025 at 02:50:23PM +0200, Sabrina Dubroca wrote:
> > Xiumei reported a regression in IPsec offload tests over xfrmi, where
> > IPv6 over IPv4 tunnels are no longer offloaded after commit
> > cc18f482e8b6 ("xfrm: provide common xdo_dev_offload_ok callback
> > implementation").
> 
> What does it mean "tunnels not offloaded"?

Offload is no longer performed for those tunnels, or for packets going
through those tunnels if we want to be pedantic.

> xdo_dev_offload_ok()
> participates in data path and influences packet processing itself,
> but not if tunnel offloaded or not.

If for you "tunnel is offloaded" means "xdo_dev_state_add is called",
then yes.


> Also what type of "offload" are you talking? Crypto or packet?

Crypto offload, but I don't think packet offload would behave
differently here.

> > Commit cc18f482e8b6 added a generic version of existing checks
> > attempting to prevent packets with IPv4 options or IPv6 extension
> > headers from being sent to HW that doesn't support offloading such
> > packets. The check mistakenly uses x->props.family (the outer family)
> > to determine the inner packet's family and verify if
> > options/extensions are present.
> 
> This is how ALL implementations did, so I'm not agree with claimed Fixes
> tag (it it not important).

Well, prior to your commit, offload seemed to work on mlx5 as I
describe just after this.

But yes, I opted for a Fixes tag more aimed at stable backports with
additional references to the commits. I don't mind putting all the
Fixes tags for each driver as well (except ixgbe/ixgbevf since it's
transport-only so not affected by this, as I wrote in the commit).

> > In the case of IPv6 over IPv4, the check compares some of the traffic
> > class bits to the expected no-options ihl value (5). The original
> > check was introduced in commit 2ac9cfe78223 ("net/mlx5e: IPSec, Add
> > Innova IPSec offload TX data path"), and then duplicated in the other
> > drivers. Before commit cc18f482e8b6, the loose check (ihl > 5) passed
> > because those traffic class bits were not set to a value that
> > triggered the no-offload codepath. Packets with options/extension
> > headers that should have been handled in SW went through the offload
> > path, and were likely dropped by the NIC or incorrectly
> > processed.
> 
> The latter is more correct, so it raises question against which
> in-kernel driver were these xfrmi tests performed?

mlx5

> > Since commit cc18f482e8b6, the check is now strict (ihl !=
> > 5), and in a basic setup (no traffic class configured), all packets go
> > through the no-offload codepath.
> > 
> > The commits that introduced the incorrect family checks in each driver
> > are:
> > 2ac9cfe78223 ("net/mlx5e: IPSec, Add Innova IPSec offload TX data path")
> > 8362ea16f69f ("crypto: chcr - ESN for Inline IPSec Tx")
> > 859a497fe80c ("nfp: implement xfrm callbacks and expose ipsec offload feature to upper layer")
> > 32188be805d0 ("cn10k-ipsec: Allow ipsec crypto offload for skb with SA")
> > [ixgbe/ixgbevf commits are ignored, as that HW does not support tunnel
> > mode, thus no cross-family setups are possible]
> > 
> > Fixes: cc18f482e8b6 ("xfrm: provide common xdo_dev_offload_ok callback implementation")
> > Reported-by: Xiumei Mu <xmu@redhat.com>
> > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> > ---
> >  net/xfrm/xfrm_device.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> > index c7a1f080d2de..44b9de6e4e77 100644
> > --- a/net/xfrm/xfrm_device.c
> > +++ b/net/xfrm/xfrm_device.c
> > @@ -438,7 +438,7 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x)
> >  
> >  	check_tunnel_size = x->xso.type == XFRM_DEV_OFFLOAD_PACKET &&
> >  			    x->props.mode == XFRM_MODE_TUNNEL;
> > -	switch (x->props.family) {
> > +	switch (x->inner_mode.family) {
> 
> Will it work for transport mode too? We are taking this path both for
> tunnel and transport modes.

Yes, if you look at __xfrm_init_state, inner_mode will always be set
to whatever family is "inside".

> Thanks
> 
> >  	case AF_INET:
> >  		/* Check for IPv4 options */
> >  		if (ip_hdr(skb)->ihl != 5)
> > -- 
> > 2.50.0
> > 
> > 

-- 
Sabrina

Re: [PATCH ipsec] xfrm: fix offloading of cross-family tunnels

[Leon Romanovsky]

On Tue, Sep 09, 2025 at 08:29:20PM +0200, Sabrina Dubroca wrote:
> 2025-09-09, 12:23:15 +0300, Leon Romanovsky wrote:
> > On Mon, Aug 25, 2025 at 02:50:23PM +0200, Sabrina Dubroca wrote:
> > > Xiumei reported a regression in IPsec offload tests over xfrmi, where
> > > IPv6 over IPv4 tunnels are no longer offloaded after commit
> > > cc18f482e8b6 ("xfrm: provide common xdo_dev_offload_ok callback
> > > implementation").
> > 
> > What does it mean "tunnels not offloaded"?
> 
> Offload is no longer performed for those tunnels, or for packets going
> through those tunnels if we want to be pedantic.
> 
> > xdo_dev_offload_ok()
> > participates in data path and influences packet processing itself,
> > but not if tunnel offloaded or not.
> 
> If for you "tunnel is offloaded" means "xdo_dev_state_add is called",
> then yes.

Yes, "offloaded" means that we created HW objects.

> 
> 
> > Also what type of "offload" are you talking? Crypto or packet?
> 
> Crypto offload, but I don't think packet offload would behave
> differently here.

It will, at least in the latest code, we have an extra check before
passing packet to HW.

  765         if (x->xso.type == XFRM_DEV_OFFLOAD_PACKET) {
  766                 if (!xfrm_dev_offload_ok(skb, x)) {
  767                         XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
  768                         kfree_skb(skb);
  769                         return -EHOSTUNREACH;
  770                 }

> 
> > > Commit cc18f482e8b6 added a generic version of existing checks
> > > attempting to prevent packets with IPv4 options or IPv6 extension
> > > headers from being sent to HW that doesn't support offloading such
> > > packets. The check mistakenly uses x->props.family (the outer family)
> > > to determine the inner packet's family and verify if
> > > options/extensions are present.
> > 
> > This is how ALL implementations did, so I'm not agree with claimed Fixes
> > tag (it it not important).
> 
> Well, prior to your commit, offload seemed to work on mlx5 as I
> describe just after this.

It worked by chance, not by design :)

> 
> But yes, I opted for a Fixes tag more aimed at stable backports with
> additional references to the commits. I don't mind putting all the
> Fixes tags for each driver as well (except ixgbe/ixgbevf since it's
> transport-only so not affected by this, as I wrote in the commit).

No problem, like I wrote, it is not important.

> 
> > > In the case of IPv6 over IPv4, the check compares some of the traffic
> > > class bits to the expected no-options ihl value (5). The original
> > > check was introduced in commit 2ac9cfe78223 ("net/mlx5e: IPSec, Add
> > > Innova IPSec offload TX data path"), and then duplicated in the other
> > > drivers. Before commit cc18f482e8b6, the loose check (ihl > 5) passed
> > > because those traffic class bits were not set to a value that
> > > triggered the no-offload codepath. Packets with options/extension
> > > headers that should have been handled in SW went through the offload
> > > path, and were likely dropped by the NIC or incorrectly
> > > processed.
> > 
> > The latter is more correct, so it raises question against which
> > in-kernel driver were these xfrmi tests performed?
> 
> mlx5

It is artifact.

> 
> > > Since commit cc18f482e8b6, the check is now strict (ihl !=
> > > 5), and in a basic setup (no traffic class configured), all packets go
> > > through the no-offload codepath.
> > > 
> > > The commits that introduced the incorrect family checks in each driver
> > > are:
> > > 2ac9cfe78223 ("net/mlx5e: IPSec, Add Innova IPSec offload TX data path")
> > > 8362ea16f69f ("crypto: chcr - ESN for Inline IPSec Tx")
> > > 859a497fe80c ("nfp: implement xfrm callbacks and expose ipsec offload feature to upper layer")
> > > 32188be805d0 ("cn10k-ipsec: Allow ipsec crypto offload for skb with SA")
> > > [ixgbe/ixgbevf commits are ignored, as that HW does not support tunnel
> > > mode, thus no cross-family setups are possible]
> > > 
> > > Fixes: cc18f482e8b6 ("xfrm: provide common xdo_dev_offload_ok callback implementation")
> > > Reported-by: Xiumei Mu <xmu@redhat.com>
> > > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> > > ---
> > >  net/xfrm/xfrm_device.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> > > index c7a1f080d2de..44b9de6e4e77 100644
> > > --- a/net/xfrm/xfrm_device.c
> > > +++ b/net/xfrm/xfrm_device.c
> > > @@ -438,7 +438,7 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x)
> > >  
> > >  	check_tunnel_size = x->xso.type == XFRM_DEV_OFFLOAD_PACKET &&
> > >  			    x->props.mode == XFRM_MODE_TUNNEL;
> > > -	switch (x->props.family) {
> > > +	switch (x->inner_mode.family) {
> > 
> > Will it work for transport mode too? We are taking this path both for
> > tunnel and transport modes.
> 
> Yes, if you look at __xfrm_init_state, inner_mode will always be set
> to whatever family is "inside".

I believe that you need to rephrase commit message around meaning of "offloaded"
but the change looks ok to me.

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH ipsec] xfrm: fix offloading of cross-family tunnels

[Sabrina Dubroca]

2025-09-10, 08:45:50 +0300, Leon Romanovsky wrote:
> On Tue, Sep 09, 2025 at 08:29:20PM +0200, Sabrina Dubroca wrote:
> > 2025-09-09, 12:23:15 +0300, Leon Romanovsky wrote:
> > > On Mon, Aug 25, 2025 at 02:50:23PM +0200, Sabrina Dubroca wrote:
> > > > Xiumei reported a regression in IPsec offload tests over xfrmi, where
> > > > IPv6 over IPv4 tunnels are no longer offloaded after commit
> > > > cc18f482e8b6 ("xfrm: provide common xdo_dev_offload_ok callback
> > > > implementation").
> > > 
> > > What does it mean "tunnels not offloaded"?
> > 
> > Offload is no longer performed for those tunnels, or for packets going
> > through those tunnels if we want to be pedantic.
> > 
> > > xdo_dev_offload_ok()
> > > participates in data path and influences packet processing itself,
> > > but not if tunnel offloaded or not.
> > 
> > If for you "tunnel is offloaded" means "xdo_dev_state_add is called",
> > then yes.
> 
> Yes, "offloaded" means that we created HW objects.

For me "offloaded" can mean either the xfrm state or the packets
depending on context, and I don't think there's a strict definition,
but whatever.

Xiumei reported a regression in IPsec offload tests over xfrmi, where
the traffic for IPv6 over IPv4 tunnels is processed in SW instead of
going through crypto offload, after commit [...].

It's getting too verbose IMO, but does that work for you?


For the subject, are you ok with the current one? It's hard to fit
more details into such a short space.

> > > Also what type of "offload" are you talking? Crypto or packet?
> > 
> > Crypto offload, but I don't think packet offload would behave
> > differently here.
> 
> It will, at least in the latest code, we have an extra check before
> passing packet to HW.
> 
>   765         if (x->xso.type == XFRM_DEV_OFFLOAD_PACKET) {
>   766                 if (!xfrm_dev_offload_ok(skb, x)) {
>   767                         XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
>   768                         kfree_skb(skb);
>   769                         return -EHOSTUNREACH;
>   770                 }

So it looks like packet offload is also affected. We get to
xfrm_dev_offload_ok, it does the wrong check, and the packets will get
dropped instead of being sent through SW crypto. Am I misreading this?


> > > > Commit cc18f482e8b6 added a generic version of existing checks
> > > > attempting to prevent packets with IPv4 options or IPv6 extension
> > > > headers from being sent to HW that doesn't support offloading such
> > > > packets. The check mistakenly uses x->props.family (the outer family)
> > > > to determine the inner packet's family and verify if
> > > > options/extensions are present.
> > > 
> > > This is how ALL implementations did, so I'm not agree with claimed Fixes
> > > tag (it it not important).
> > 
> > Well, prior to your commit, offload seemed to work on mlx5 as I
> > describe just after this.
> 
> It worked by chance, not by design :)

Sure.

[...]
> > > The latter is more correct, so it raises question against which
> > > in-kernel driver were these xfrmi tests performed?
> > 
> > mlx5
> 
> It is artifact.

Sorry, I'm not sure what you mean here.

[...]
> > > Will it work for transport mode too? We are taking this path both for
> > > tunnel and transport modes.
> > 
> > Yes, if you look at __xfrm_init_state, inner_mode will always be set
> > to whatever family is "inside".
> 
> I believe that you need to rephrase commit message around meaning of "offloaded"
> but the change looks ok to me.
> 
> Thanks,
> Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Thanks. I'll send a v2 when we agree on the wording, to avoid
resending multiple times.

-- 
Sabrina

Re: [PATCH ipsec] xfrm: fix offloading of cross-family tunnels

[Leon Romanovsky]

On Wed, Sep 10, 2025 at 10:04:34AM +0200, Sabrina Dubroca wrote:
> 2025-09-10, 08:45:50 +0300, Leon Romanovsky wrote:
> > On Tue, Sep 09, 2025 at 08:29:20PM +0200, Sabrina Dubroca wrote:
> > > 2025-09-09, 12:23:15 +0300, Leon Romanovsky wrote:
> > > > On Mon, Aug 25, 2025 at 02:50:23PM +0200, Sabrina Dubroca wrote:
> > > > > Xiumei reported a regression in IPsec offload tests over xfrmi, where
> > > > > IPv6 over IPv4 tunnels are no longer offloaded after commit
> > > > > cc18f482e8b6 ("xfrm: provide common xdo_dev_offload_ok callback
> > > > > implementation").
> > > > 
> > > > What does it mean "tunnels not offloaded"?
> > > 
> > > Offload is no longer performed for those tunnels, or for packets going
> > > through those tunnels if we want to be pedantic.
> > > 
> > > > xdo_dev_offload_ok()
> > > > participates in data path and influences packet processing itself,
> > > > but not if tunnel offloaded or not.
> > > 
> > > If for you "tunnel is offloaded" means "xdo_dev_state_add is called",
> > > then yes.
> > 
> > Yes, "offloaded" means that we created HW objects.
> 
> For me "offloaded" can mean either the xfrm state or the packets
> depending on context, and I don't think there's a strict definition,
> but whatever.
> 
> Xiumei reported a regression in IPsec offload tests over xfrmi, where
> the traffic for IPv6 over IPv4 tunnels is processed in SW instead of
> going through crypto offload, after commit [...].
> 
> It's getting too verbose IMO, but does that work for you?

Yes, it is perfectly fine.

> 
> 
> For the subject, are you ok with the current one? It's hard to fit
> more details into such a short space.

Leave subject as is, you describe issue well enough in the commit
message.

> 
> > > > Also what type of "offload" are you talking? Crypto or packet?
> > > 
> > > Crypto offload, but I don't think packet offload would behave
> > > differently here.
> > 
> > It will, at least in the latest code, we have an extra check before
> > passing packet to HW.
> > 
> >   765         if (x->xso.type == XFRM_DEV_OFFLOAD_PACKET) {
> >   766                 if (!xfrm_dev_offload_ok(skb, x)) {
> >   767                         XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTERROR);
> >   768                         kfree_skb(skb);
> >   769                         return -EHOSTUNREACH;
> >   770                 }
> 
> So it looks like packet offload is also affected. We get to
> xfrm_dev_offload_ok, it does the wrong check, and the packets will get
> dropped instead of being sent through SW crypto. Am I misreading this?

There is no fallback in packet offload, so dropping packet which can't
be handled by HW is right thing to do. I agree that we shouldn't fail
here.

> 
> 
> > > > > Commit cc18f482e8b6 added a generic version of existing checks
> > > > > attempting to prevent packets with IPv4 options or IPv6 extension
> > > > > headers from being sent to HW that doesn't support offloading such
> > > > > packets. The check mistakenly uses x->props.family (the outer family)
> > > > > to determine the inner packet's family and verify if
> > > > > options/extensions are present.
> > > > 
> > > > This is how ALL implementations did, so I'm not agree with claimed Fixes
> > > > tag (it it not important).
> > > 
> > > Well, prior to your commit, offload seemed to work on mlx5 as I
> > > describe just after this.
> > 
> > It worked by chance, not by design :)
> 
> Sure.
> 
> [...]
> > > > The latter is more correct, so it raises question against which
> > > > in-kernel driver were these xfrmi tests performed?
> > > 
> > > mlx5
> > 
> > It is artifact.
> 
> Sorry, I'm not sure what you mean here.

I'm saying that "works" depends on FW and HW revision.

> 
> [...]
> > > > Will it work for transport mode too? We are taking this path both for
> > > > tunnel and transport modes.
> > > 
> > > Yes, if you look at __xfrm_init_state, inner_mode will always be set
> > > to whatever family is "inside".
> > 
> > I believe that you need to rephrase commit message around meaning of "offloaded"
> > but the change looks ok to me.
> > 
> > Thanks,
> > Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
> 
> Thanks. I'll send a v2 when we agree on the wording, to avoid
> resending multiple times.
> 
> -- 
> Sabrina