xfrm: some fixes for GSO with SW crypto

Commit 49431af6c4ef incorrectly assumes that the GSO path is only used by HW offload, but it's also useful for SW crypto.

This patch re-enables GSO for SW crypto. It's not an exact revert to preserve the other changes made to xfrm_dev_offload_ok afterwards, but it reverts all of its effects.

Fixes 49431af6c4ef ("xfrm: rely on XFRM offload")

Signed-off-by Sabrina Dubroca <[email protected]>

---
 net/xfrm/xfrm_device.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Commit 49431af6c4ef incorrectly assumes that the GSO path is only used by HW offload, but it's also useful for SW crypto.

This patch re-enables GSO for SW crypto. It's not an exact revert to preserve the other changes made to xfrm_dev_offload_ok afterwards, but it reverts all of its effects.

Fixes 49431af6c4ef ("xfrm: rely on XFRM offload")

Signed-off-by Sabrina Dubroca <[email protected]>

---
  net/xfrm/xfrm_device.c | 9 +++++++--
  1 file changed, 7 insertions(+), 2 deletions(-)

net/xfrm/xfrm_device.c +7 -2

index d2819baea414..1f88472aaac0 100644
@@ -415,10 +415,12 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x)
  	struct net_device *dev = x->xso.dev;
  	bool check_tunnel_size;
 
-	if (x->xso.type == XFRM_DEV_OFFLOAD_UNSPECIFIED)
+	if (!x->type_offload ||
+	    (x->xso.type == XFRM_DEV_OFFLOAD_UNSPECIFIED && x->encap))
  		return false;
 
-	if ((dev == xfrm_dst_path(dst)->dev) && !xdst->child->xfrm) {
+	if ((!dev || dev == xfrm_dst_path(dst)->dev) &&
+	    !xdst->child->xfrm) {
  		mtu = xfrm_state_mtu(x, xdst->child_mtu_cached);
  		if (skb->len <= mtu)
  			goto ok;

@@ -430,6 +432,9 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x)
  	return false;
 
  ok:
+	if (!dev)
+		return true;
+
  	check_tunnel_size = x->xso.type == XFRM_DEV_OFFLOAD_PACKET &&
  			    x->props.mode == XFRM_MODE_TUNNEL;
  	switch (x->props.family) {

This reverts commit d53dda291bbd993a29b84d358d282076e3d01506.

This change causes traffic using GSO with SW crypto running through a NIC capable of HW offload to no longer get segmented during validate_xmit_xfrm.

Fixes d53dda291bbd ("xfrm: Remove unneeded device check from validate_xmit_xfrm")

Signed-off-by Sabrina Dubroca <[email protected]>

---
 net/xfrm/xfrm_device.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

This reverts commit d53dda291bbd993a29b84d358d282076e3d01506.

This change causes traffic using GSO with SW crypto running through a NIC capable of HW offload to no longer get segmented during validate_xmit_xfrm.

Fixes d53dda291bbd ("xfrm: Remove unneeded device check from

validate_xmit_xfrm")

Quoted 9 lines · Sabrina Dubroca

This reverts commit d53dda291bbd993a29b84d358d282076e3d01506.

This change causes traffic using GSO with SW crypto running through a NIC capable of HW offload to no longer get segmented during validate_xmit_xfrm.

Fixes d53dda291bbd ("xfrm: Remove unneeded device check from

validate_xmit_xfrm")

Thanks for the fix, but I'm curious about details.

In that commit, I tried to map all of the possible code paths. Can you please explain what code paths I missed that need real_dev given that only bonding should use it now?

Quoted 12 lines · Cosmin Ratiu

Quoted 10 lines · Sabrina Dubroca

This reverts commit d53dda291bbd993a29b84d358d282076e3d01506.

This change causes traffic using GSO with SW crypto running through a
NIC capable of HW offload to no longer get segmented during validate_xmit_xfrm.

Fixes d53dda291bbd ("xfrm: Remove unneeded device check from

validate_xmit_xfrm")

Quoted 7 lines

Thanks for the fix, but I'm curious about details.

In that commit, I tried to map all of the possible code paths. Can you
please explain what code paths I missed that need real_dev given that
only bonding should use it now?

After running some more tests, it's not about real_dev, it's the other
check ("unlikely(x->xso.dev != dev)" below) that you also removed in that patch that causes the issue in my setup. I don't know how you decided that it should be dropped, since it predates bonding's ipsec offload.

The codepath is the usual:
__dev_queue_xmit -> validate_xmit_skb -> validate_xmit_xfrm

Since the commit message made the incorrect claim "ESP offload off:
validate_xmit_xfrm returns early on !xo." I didn't check if a partial revert was enough to fix the issue. My bad.

Quoted 21 lines · Sabrina Dubroca

Quoted 12 lines · Cosmin Ratiu

Quoted 10 lines · Sabrina Dubroca

This reverts commit d53dda291bbd993a29b84d358d282076e3d01506.

This change causes traffic using GSO with SW crypto running through a
NIC capable of HW offload to no longer get segmented during validate_xmit_xfrm.

Fixes d53dda291bbd ("xfrm: Remove unneeded device check from

validate_xmit_xfrm")

Quoted 7 lines

Thanks for the fix, but I'm curious about details.

In that commit, I tried to map all of the possible code paths. Can you
please explain what code paths I missed that need real_dev given that
only bonding should use it now?

Quoted 6 lines

After running some more tests, it's not about real_dev, it's the other
check ("unlikely(x->xso.dev != dev)" below) that you also removed in that patch that causes the issue in my setup. I don't know how you decided that it should be dropped, since it predates bonding's ipsec offload.

Apologies for that, I think I assumed that if offload is off, then xfrm_offload(skb) is NULL and the code bails out early on "if (!xo)". Seems I was wrong. On the TX side, the only place that adds a secpath and increments sp->olen (and thus add an xfrm_offload) is in xfrm_output, after the xfrm_dev_offload_ok check.

Quoted 7 lines

The codepath is the usual:
__dev_queue_xmit -> validate_xmit_skb -> validate_xmit_xfrm

Since the commit message made the incorrect claim "ESP offload off:
validate_xmit_xfrm returns early on !xo." I didn't check if a partial revert was enough to fix the issue. My bad.

No problem, good that we caught the actual issue. Will you prepare a follow-up patch then?