fix kernel crash with macvtap on top of LRO

PATCH In review Michael S. Tsirkin 6 participants 21 messages Feb 6, 2013 – Jun 18, 2013

Subsystems (5)

Networking Network drivers virtio core Intel Ethernet +1 more subsystems

lore

Share & tools

lore

Michael S. Tsirkin

author maintainer

2013-02-06 22:58 UTC #0 lore raw

cover fix kernel crash with macvtap on top of LRO

At the moment, macvtap crashes are observed if macvtap is attached to an interface with LRO enabled.
The crash in question is BUG() in macvtap_skb_to_vnet_hdr. This happens because several drivers set gso_size but not gso_type in incoming skbs.
The following patches fix this for
Additionally, cbf1de72324a8105ddcc3d9ce9acbc613faea17e is required to fix this for broadcom - would it make sense to cherry-pick this patch into 3.8?

I tested that the patch fixes the crash for ixgbe but don't have qlogic hardware to fix. I also only tested TCP.

Feedback wellcome.

Michael S. Tsirkin (2):
ixgbe: fix gso type
qlcnic: set gso_type

 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  | 8 ++++++--
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c | 7 ++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

Signature · 1 line

MST

Michael S. Tsirkin

author maintainer

2013-02-06 22:58 UTC #1 lore raw

PATCH 1/2 ixgbe: fix gso type

ixgbe set gso_size but not gso_type. This leads to crashes in macvtap.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

I tested that this fixes the crash for me. I am told on ixgbe LRO only triggers with TCP so checking protocol should be enough?

 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +6 -2

index 396e280..9d01673 100644
@@ -1399,6 +1399,10 @@ static void ixgbe_set_rsc_gso_size(struct ixgbe_ring *ring,
 	/* set gso_size to avoid messing up TCP MSS */
 	skb_shinfo(skb)->gso_size = DIV_ROUND_UP((skb->len - hdr_len),
 						 IXGBE_CB(skb)->append_cnt);
+	if (skb->protocol == ETH_P_IPV6)
+		skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
+	else
+		skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
 }
 
 static void ixgbe_update_rsc_stats(struct ixgbe_ring *rx_ring,

@@ -1433,6 +1437,8 @@ static void ixgbe_process_skb_fields(struct ixgbe_ring *rx_ring,
 {
 	struct net_device *dev = rx_ring->netdev;
 
+	skb->protocol = eth_type_trans(skb, dev);
+
 	ixgbe_update_rsc_stats(rx_ring, skb);
 
 	ixgbe_rx_hash(rx_ring, rx_desc, skb);

@@ -1448,8 +1454,6 @@ static void ixgbe_process_skb_fields(struct ixgbe_ring *rx_ring,
 	}
 
 	skb_record_rx_queue(skb, rx_ring->queue_index);
-
-	skb->protocol = eth_type_trans(skb, dev);
 }
 
 static void ixgbe_rx_skb(struct ixgbe_q_vector *q_vector,

Signature · 1 line

MST

Eric Dumazet

2013-02-07 03:21 UTC #2 lore raw

Re: [PATCH 1/2] ixgbe: fix gso type

Quoted 17 lines · Michael S. Tsirkin

ixgbe set gso_size but not gso_type. This leads to crashes in macvtap.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
I tested that this fixes the crash for me. I am told on ixgbe LRO only triggers with TCP so checking protocol should be enough?
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0 -0
index 396e280..9d01673 100644

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static void ixgbe_set_rsc_gso_size(struct ixgbe_ring *ring, · line 1402

Quoted 4 lines

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +1 -0

@@ -1399,6 +1399,10 @@ static void ixgbe_set_rsc_gso_size(struct ixgbe_ring *ring,
 	/* set gso_size to avoid messing up TCP MSS */
 	skb_shinfo(skb)->gso_size = DIV_ROUND_UP((skb->len - hdr_len),
 						 IXGBE_CB(skb)->append_cnt);
+	if (skb->protocol == ETH_P_IPV6)

Same problem here (skb->protocol == htons(ETH_P_IPV6))

Quoted 5 lines

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +3 -0

+		skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
+	else
+		skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
 }

Michael S. Tsirkin

author maintainer

2013-02-06 22:59 UTC #3 lore raw

PATCH 2/2 qlcnic: set gso_type

In reply to Michael S. Tsirkin

qlcnic set gso_size but not gso type. This leads to crashes in macvtap.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
This one I only compiled - don't have qlogic hardware.

 drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c +6 -1

index bb4311e..370049c 100644
@@ -1043,8 +1043,13 @@ qlcnic_process_lro(struct qlcnic_adapter *adapter,
 	th->seq = htonl(seq_number);
 	length = skb->len;
 
-	if (adapter->flags & QLCNIC_FW_LRO_MSS_CAP)
+	if (adapter->flags & QLCNIC_FW_LRO_MSS_CAP) {
 		skb_shinfo(skb)->gso_size = qlcnic_get_lro_sts_mss(sts_data1);
+		if (skb->protocol == ETH_P_IPV6)
+			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
+		else
+			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
+	}
 
 	if (vid != 0xffff)
 		__vlan_hwaccel_put_tag(skb, vid);

Signature · 1 line

MST

Eric Dumazet

2013-02-07 03:20 UTC #4 lore raw

Re: [PATCH 2/2] qlcnic: set gso_type

Quoted 15 lines · Michael S. Tsirkin

qlcnic set gso_size but not gso type. This leads to crashes in macvtap.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
This one I only compiled - don't have qlogic hardware.
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c +0 -0
index bb4311e..370049c 100644

drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c qlcnic_process_lro(struct qlcnic_adapter *adapter, · line 1048

Quoted 7 lines

drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c +2 -1

@@ -1043,8 +1043,13 @@ qlcnic_process_lro(struct qlcnic_adapter *adapter,
 	th->seq = htonl(seq_number);
 	length = skb->len;
 
-	if (adapter->flags & QLCNIC_FW_LRO_MSS_CAP)
+	if (adapter->flags & QLCNIC_FW_LRO_MSS_CAP) {
 		skb_shinfo(skb)->gso_size = qlcnic_get_lro_sts_mss(sts_data1);
+		if (skb->protocol == ETH_P_IPV6)

Are you sure its not skb->protocol == htons(ETH_P_IPV6) ?

Quoted 7 lines

drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c +4 -0

+			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
+		else
+			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
+	}
 
 	if (vid != 0xffff)
 		__vlan_hwaccel_put_tag(skb, vid);

Michael S. Tsirkin

author maintainer

2013-02-07 08:12 UTC #5 lore raw

Re: [PATCH 2/2] qlcnic: set gso_type

Quoted 19 lines · Eric Dumazet

Quoted 18 lines · Michael S. Tsirkin
qlcnic set gso_size but not gso type. This leads to crashes in macvtap.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
This one I only compiled - don't have qlogic hardware.
 drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c +0 -0
index bb4311e..370049c 100644
@@ -1043,8 +1043,13 @@ qlcnic_process_lro(struct qlcnic_adapter *adapter,
 	th->seq = htonl(seq_number);
 	length = skb->len;
 

Quoted 6 lines

drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c qlcnic_process_lro(struct qlcnic_adapter *adapter, · line 1048
Quoted 4 lines
drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c +2 -1
-	if (adapter->flags & QLCNIC_FW_LRO_MSS_CAP)
+	if (adapter->flags & QLCNIC_FW_LRO_MSS_CAP) {
 		skb_shinfo(skb)->gso_size = qlcnic_get_lro_sts_mss(sts_data1);
+		if (skb->protocol == ETH_P_IPV6)
Are you sure its not skb->protocol == htons(ETH_P_IPV6) ?

Um, so it is. It just happens to work correctly for ipv4 :) I'll respin.

Quoted 8 lines

Quoted 7 lines

drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c +4 -0

+			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
+		else
+			skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
+	}
 
 	if (vid != 0xffff)
 		__vlan_hwaccel_put_tag(skb, vid);

Ben Hutchings

2013-02-06 23:34 UTC #6 lore raw

In reply to Michael S. Tsirkin

Quoted 10 lines · Michael S. Tsirkin

At the moment, macvtap crashes are observed if macvtap is attached to an interface with LRO enabled.
The crash in question is BUG() in macvtap_skb_to_vnet_hdr. This happens because several drivers set gso_size but not gso_type in incoming skbs.
The following patches fix this for
Additionally, cbf1de72324a8105ddcc3d9ce9acbc613faea17e is required to fix this for broadcom - would it make sense to cherry-pick this patch into 3.8?

Quoted 4 lines

I tested that the patch fixes the crash for ixgbe but don't have qlogic hardware to fix. I also only tested TCP.
Feedback wellcome.

If we want to allow forwarding from LRO then net/ipv4/inet_lro.c also needs to set gso_type.

Ben.

Signature · 15 lines

Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.


------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013 
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Eric Dumazet

2013-02-07 03:18 UTC #7 lore raw

Quoted 2 lines · Ben Hutchings

If we want to allow forwarding from LRO then net/ipv4/inet_lro.c also needs to set gso_type.

Then, what is dev_disable_lro() purpose ?

------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

Ben Hutchings

2013-02-07 16:20 UTC #8 lore raw

Quoted 6 lines · Eric Dumazet

Quoted 2 lines · Ben Hutchings
If we want to allow forwarding from LRO then net/ipv4/inet_lro.c also needs to set gso_type.
Then, what is dev_disable_lro() purpose ?

The purpose was to disable LRO when forwarding because they weren't compatible.

If the consensus now is that the modifications made by LRO+TSO are acceptable in a bridge/router, then we should get rid of dev_disable_lro() and set both gso_size & gso_type on all LRO receive paths.

If the consensus is still that we must preserve packets exactly (aside from the usual modifications by IP routers) then LRO should be disabled on all devices for which forwarding is enabled. (Also, we really ought to keep a count of the number of forwarders and use that in netdev_fix_features(), rather than doing a one-time disable.)

Ben.

Signature · 15 lines

Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.


------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013 
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

David Miller

maintainer

2013-02-07 18:14 UTC #9 lore raw

From: Ben Hutchings <[email protected]>
Date: Thu, 7 Feb 2013 16:20:46 +0000

Quoted 3 lines

If the consensus is still that we must preserve packets exactly (aside from the usual modifications by IP routers) then LRO should be disabled on all devices for which forwarding is enabled.

I believe this is still undoubtedly the consensus.

Michael S. Tsirkin

author maintainer

2013-02-07 21:29 UTC #10 lore raw

Quoted 8 lines · David Miller

From: Ben Hutchings <[email protected]>
Date: Thu, 7 Feb 2013 16:20:46 +0000
Quoted 3 lines
If the consensus is still that we must preserve packets exactly (aside from the usual modifications by IP routers) then LRO should be disabled on all devices for which forwarding is enabled.
I believe this is still undoubtedly the consensus.

But we don't need to preserve the packets when passing them to macvtap (which discards all this info smashing the packet into a single buffer anyway), correct?
If true LRO with macvtap might be useful and so the patchset is probably still the right thing to do to fix the macvtap crash. Makes sense?

We might want to add code to forward LRO status from macvlan (not macvtap) back to the lowerdev, so that setting up forwarding from macvlan disables LRO on the lowerdev, but that seems like another issue.

Signature · 1 line

MST

Ben Hutchings

2013-02-07 22:31 UTC #11 lore raw

Quoted 8 lines · Michael S. Tsirkin

Quoted 7 lines · David Miller
From: Ben Hutchings <[email protected]>
Date: Thu, 7 Feb 2013 16:20:46 +0000
Quoted 3 lines
If the consensus is still that we must preserve packets exactly (aside from the usual modifications by IP routers) then LRO should be disabled on all devices for which forwarding is enabled.

Quoted 5 lines

Quoted 1 line
I believe this is still undoubtedly the consensus.
But we don't need to preserve the packets when passing them to macvtap (which discards all this info smashing the packet into a single buffer anyway), correct?

macvtap_skb_to_vnet_hdr() certainly seems to be trying to preserve all the packet information.

Quoted 2 lines

If true LRO with macvtap might be useful and so the patchset is probably still the right thing to do to fix the macvtap crash. Makes sense?

If macvtap+virtio_net is expected to re-segment then this is fine. But I don't see why it should be different from other uses of macvlan.

Quoted 4 lines

We might want to add code to forward LRO status from macvlan (not macvtap) back to the lowerdev, so that setting up forwarding from macvlan disables LRO on the lowerdev, but that seems like another issue.

I think it's the same issue!

Ben.

Signature · 15 lines

Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.


------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013 
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

David Miller

maintainer

2013-02-11 01:15 UTC #12 lore raw

From: Ben Hutchings <[email protected]>
Date: Thu, 7 Feb 2013 22:31:35 +0000

Quoted 7 lines · Michael S. Tsirkin

Quoted 4 lines
We might want to add code to forward LRO status from macvlan (not macvtap) back to the lowerdev, so that setting up forwarding from macvlan disables LRO on the lowerdev, but that seems like another issue.
I think it's the same issue!

I do too, can someome please work to resolve this properly?

Thanks.

Michael S. Tsirkin

author maintainer

2013-06-10 07:06 UTC #13 lore raw

In reply to David Miller

Quoted 8 lines · David Miller

From: Ben Hutchings <[email protected]>
Date: Thu, 7 Feb 2013 16:20:46 +0000
Quoted 3 lines
If the consensus is still that we must preserve packets exactly (aside from the usual modifications by IP routers) then LRO should be disabled on all devices for which forwarding is enabled.
I believe this is still undoubtedly the consensus.

With virtio we are getting packets from a linux host, so we could thinkably preserve packets exactly even with LRO. I am guessing other hardware could be doing this as well.

I am not sure what information would need to be preserved - could someone help clarify please?

Signature · 1 line

MST

Ben Hutchings

2013-06-12 14:56 UTC #14 lore raw

Quoted 10 lines · Michael S. Tsirkin

Quoted 8 lines · David Miller
From: Ben Hutchings <[email protected]>
Date: Thu, 7 Feb 2013 16:20:46 +0000
Quoted 3 lines
If the consensus is still that we must preserve packets exactly (aside from the usual modifications by IP routers) then LRO should be disabled on all devices for which forwarding is enabled.
I believe this is still undoubtedly the consensus.

Quoted 7 lines

With virtio we are getting packets from a linux host, so we could thinkably preserve packets exactly even with LRO. I am guessing other hardware could be doing this as well.
I am not sure what information would need to be preserved - could someone help clarify please?

Some LRO implementations may not preserve:

- Packet boundaries
- TSO/GSO produces packets all the same size, except possibly for the last one. GRO therefore flushes a flow after merging a packet with a different segment size.
- IPv4 TTL, IPv6 hop-limit, TCP timestamp
- TSO/GSO will put the same values in all packets. GRO flushes a flow if they change.
- IPv4 fragment ID
- TSO/GSO produces consecutive fragment IDs. GRO flushes a flow if it sees a non-consecutive fragment ID.
- MAC header, IPv4 TOS, IPv6 traffic class
- Should be the same for all packets in a flow. GRO actually checks and flushes a flow if they change.

Ben.

Signature · 3 lines

Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Michael S. Tsirkin

author maintainer

2013-06-12 19:00 UTC #15 lore raw

Quoted 35 lines · Ben Hutchings

Quoted 10 lines · Michael S. Tsirkin
Quoted 8 lines · David Miller
From: Ben Hutchings <[email protected]>
Date: Thu, 7 Feb 2013 16:20:46 +0000
Quoted 3 lines
If the consensus is still that we must preserve packets exactly (aside from the usual modifications by IP routers) then LRO should be disabled on all devices for which forwarding is enabled.
I believe this is still undoubtedly the consensus.
Quoted 7 lines
With virtio we are getting packets from a linux host, so we could thinkably preserve packets exactly even with LRO. I am guessing other hardware could be doing this as well.
I am not sure what information would need to be preserved - could someone help clarify please?
Some LRO implementations may not preserve:
- Packet boundaries
- TSO/GSO produces packets all the same size, except possibly for the last one. GRO therefore flushes a flow after merging a packet with a different segment size.
- IPv4 TTL, IPv6 hop-limit, TCP timestamp
- TSO/GSO will put the same values in all packets. GRO flushes a flow if they change.
- IPv4 fragment ID
- TSO/GSO produces consecutive fragment IDs. GRO flushes a flow if it sees a non-consecutive fragment ID.
- MAC header, IPv4 TOS, IPv6 traffic class
- Should be the same for all packets in a flow. GRO actually checks and flushes a flow if they change.

Quoted 1 line

Ben.

Thanks!

Okay so since LRO in virtio is actually running on top of GRO/TSO in linux, looks like the only things worth preserving that we don't preserve at the moment are the packet boundaries, for which it's enough to report
the first segment size to guest - we have the total length, from that we can calculate the last segment size.

Quoted 4 lines

Signature · 3 lines

Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Rusty Russell

2013-06-17 01:35 UTC #16 lore raw

Quoted 38 lines · Michael S. Tsirkin

Quoted 35 lines · Ben Hutchings
Quoted 10 lines · Michael S. Tsirkin
Quoted 8 lines · David Miller
From: Ben Hutchings <[email protected]>
Date: Thu, 7 Feb 2013 16:20:46 +0000
Quoted 3 lines
If the consensus is still that we must preserve packets exactly (aside from the usual modifications by IP routers) then LRO should be disabled on all devices for which forwarding is enabled.
I believe this is still undoubtedly the consensus.
Quoted 7 lines
With virtio we are getting packets from a linux host, so we could thinkably preserve packets exactly even with LRO. I am guessing other hardware could be doing this as well.
I am not sure what information would need to be preserved - could someone help clarify please?
Some LRO implementations may not preserve:
- Packet boundaries
- TSO/GSO produces packets all the same size, except possibly for the last one. GRO therefore flushes a flow after merging a packet with a different segment size.
- IPv4 TTL, IPv6 hop-limit, TCP timestamp
- TSO/GSO will put the same values in all packets. GRO flushes a flow if they change.
- IPv4 fragment ID
- TSO/GSO produces consecutive fragment IDs. GRO flushes a flow if it sees a non-consecutive fragment ID.
- MAC header, IPv4 TOS, IPv6 traffic class
- Should be the same for all packets in a flow. GRO actually checks and flushes a flow if they change.
Quoted 1 line
Ben.

Quoted 8 lines

Thanks!
Okay so since LRO in virtio is actually running on top of GRO/TSO in linux, looks like the only things worth preserving that we don't preserve at the moment are the packet boundaries, for which it's enough to report
the first segment size to guest - we have the total length, from that we can calculate the last segment size.

Isn't this reflected in virtio_net_hdr->gso_size?

But the bigger point here is that we shouldn't be feeding guests LRO packets which don't meet the stricter GRO requirements, as we don't know what the guest is doing with them. It might be forwarding them itself.

I thought LRO was deprecated and GRO was the new hotness, but I haven't been following. Do we still care about LRO?

Cheers,
Rusty.

Ben Hutchings

2013-06-17 16:20 UTC #17 lore raw

Quoted 48 lines · Rusty Russell

Quoted 38 lines · Michael S. Tsirkin
Quoted 35 lines · Ben Hutchings
Quoted 10 lines · Michael S. Tsirkin
Quoted 8 lines · David Miller
From: Ben Hutchings <[email protected]>
Date: Thu, 7 Feb 2013 16:20:46 +0000
Quoted 3 lines
If the consensus is still that we must preserve packets exactly (aside from the usual modifications by IP routers) then LRO should be disabled on all devices for which forwarding is enabled.
I believe this is still undoubtedly the consensus.
Quoted 7 lines
With virtio we are getting packets from a linux host, so we could thinkably preserve packets exactly even with LRO. I am guessing other hardware could be doing this as well.
I am not sure what information would need to be preserved - could someone help clarify please?
Some LRO implementations may not preserve:
- Packet boundaries
- TSO/GSO produces packets all the same size, except possibly for the last one. GRO therefore flushes a flow after merging a packet with a different segment size.
- IPv4 TTL, IPv6 hop-limit, TCP timestamp
- TSO/GSO will put the same values in all packets. GRO flushes a flow if they change.
- IPv4 fragment ID
- TSO/GSO produces consecutive fragment IDs. GRO flushes a flow if it sees a non-consecutive fragment ID.
- MAC header, IPv4 TOS, IPv6 traffic class
- Should be the same for all packets in a flow. GRO actually checks and flushes a flow if they change.
Quoted 1 line
Ben.
Quoted 8 lines
Thanks!
Okay so since LRO in virtio is actually running on top of GRO/TSO in linux, looks like the only things worth preserving that we don't preserve at the moment are the packet boundaries, for which it's enough to report
the first segment size to guest - we have the total length, from that we can calculate the last segment size.

Quoted 8 lines

Isn't this reflected in virtio_net_hdr->gso_size?
But the bigger point here is that we shouldn't be feeding guests LRO packets which don't meet the stricter GRO requirements, as we don't know what the guest is doing with them. It might be forwarding them itself.
I thought LRO was deprecated and GRO was the new hotness, but I haven't been following. Do we still care about LRO?

The old software LRO implementation, inet_lro, is deprecated in favour of GRO and is now only used by one or two drivers. Hardware/firmware implementations of LRO are still in use and not deprecated, but we try to disable them on devices for which forwarding is enabled because of this information loss.

The problem I was talking about is this: you can put macvlan on top of a device that has LRO enabled, and then if the macvtap/macvlan device is used for forwarding the output packets might not look the same as those originally received. So LRO should be disabled on the underlying device whenever forwarding is enabled on the macvtap/macvlan device; however we can't necessarily tell when that happens as the forwarding might be done inside a VM. Maybe this is just too obscure a use case to worry much about getting it right automatically.

Ben.

Signature · 3 lines

Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Rusty Russell

2013-06-18 05:12 UTC #18 lore raw

Quoted 9 lines · Ben Hutchings

Quoted 2 lines · Rusty Russell
I thought LRO was deprecated and GRO was the new hotness, but I haven't been following. Do we still care about LRO?
The old software LRO implementation, inet_lro, is deprecated in favour of GRO and is now only used by one or two drivers. Hardware/firmware implementations of LRO are still in use and not deprecated, but we try to disable them on devices for which forwarding is enabled because of this information loss.

Right, thanks for the clarification.

Hardware implementations of LRO which can't meet GRO rules are only semi-useful, and that should be fed back to vendors. Hard.

Quoted 8 lines

The problem I was talking about is this: you can put macvlan on top of a device that has LRO enabled, and then if the macvtap/macvlan device is used for forwarding the output packets might not look the same as those originally received. So LRO should be disabled on the underlying device whenever forwarding is enabled on the macvtap/macvlan device; however we can't necessarily tell when that happens as the forwarding might be done inside a VM. Maybe this is just too obscure a use case to worry much about getting it right automatically.

The VM needs to tell us it's OK with such mangling, otherwise we shouldn't do it (at least by default). The same way we'd be annoyed if a card rev started doing LRO without the driver explicitly enabling it.

Cheers,
Rusty.

Cong Wang

2013-02-07 03:25 UTC #19 lore raw

In reply to Michael S. Tsirkin

Quoted 9 lines · Michael S. Tsirkin

At the moment, macvtap crashes are observed if macvtap is attached to an interface with LRO enabled.
The crash in question is BUG() in macvtap_skb_to_vnet_hdr. This happens because several drivers set gso_size but not gso_type in incoming skbs.
The following patches fix this for
Additionally, cbf1de72324a8105ddcc3d9ce9acbc613faea17e is required to fix this for broadcom - would it make sense to cherry-pick this patch into 3.8?

Doesn't macvtap need to call skb_warn_if_lro() too like bridge and openvswitch? Something like...

drivers/net/macvtap.c +0 -0

index b181dfb..b2c6227 100644
@@ -253,6 +253,9 @@ static int macvtap_forward(struct net_device *dev,

struct sk_buff *skb)

         if (!q)
                 goto drop;

+ if (unlikely(skb_warn_if_lro(skb)))
+ goto drop;
+

         if (skb_queue_len(&q->sk.sk_receive_queue) >= dev->tx_queue_len)
                 goto drop;

I am not saying these drivers don't need to fix, I am just saying if we need to fix LRO case.

Thanks.

Michael S. Tsirkin

author maintainer

2013-02-07 08:11 UTC #20 lore raw

Quoted 12 lines · Cong Wang

Quoted 9 lines · Michael S. Tsirkin
At the moment, macvtap crashes are observed if macvtap is attached to an interface with LRO enabled.
The crash in question is BUG() in macvtap_skb_to_vnet_hdr. This happens because several drivers set gso_size but not gso_type in incoming skbs.
The following patches fix this for
Additionally, cbf1de72324a8105ddcc3d9ce9acbc613faea17e is required to fix this for broadcom - would it make sense to cherry-pick this patch into 3.8?

Quoted 2 lines

Doesn't macvtap need to call skb_warn_if_lro() too like bridge and openvswitch? Something like...

This is what Ben proposed a year ago
http://thread.gmane.org/gmane.linux.network/221695
but apparently people really want LRO to work with macvtap.

Quoted 20 lines

drivers/net/macvtap.c +3 -0

index b181dfb..b2c6227 100644
@@ -253,6 +253,9 @@ static int macvtap_forward(struct net_device
 *dev, struct sk_buff *skb)
        if (!q)
                goto drop;
 
+       if (unlikely(skb_warn_if_lro(skb)))
+               goto drop;
+
        if (skb_queue_len(&q->sk.sk_receive_queue) >= dev->tx_queue_len)
                goto drop;

I am not saying these drivers don't need to fix, I am just saying if we need to fix LRO case.

Thanks.