Re: [E1000-devel] [PATCH 0/2] fix kernel crash with macvtap on top of LRO
From: Ben Hutchings bhutchi...@solarflare.com Date: Thu, 7 Feb 2013 22:31:35 + On Thu, 2013-02-07 at 23:33 +0200, Michael S. Tsirkin wrote: 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. -- 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 E1000-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel#174; Ethernet, visit http://communities.intel.com/community/wired
Re: [E1000-devel] [PATCH 0/2] fix kernel crash with macvtap on top of LRO
On Thu, Feb 07, 2013 at 11:25:13AM +0800, Cong Wang wrote: On 02/07/2013 07:02 AM, Michael S. Tsirkin wrote: 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... 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. diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c index b181dfb..b2c6227 100644 --- a/drivers/net/macvtap.c +++ b/drivers/net/macvtap.c @@ -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. -- 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 E1000-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel#174; Ethernet, visit http://communities.intel.com/community/wired
Re: [E1000-devel] [PATCH 0/2] fix kernel crash with macvtap on top of LRO
On Wed, 2013-02-06 at 19:18 -0800, Eric Dumazet wrote: On Wed, 2013-02-06 at 23:34 +, Ben Hutchings wrote: 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. -- 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 E1000-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel#174; Ethernet, visit http://communities.intel.com/community/wired
Re: [E1000-devel] [PATCH 0/2] fix kernel crash with macvtap on top of LRO
From: Ben Hutchings bhutchi...@solarflare.com Date: Thu, 7 Feb 2013 16:20:46 + 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. -- 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 E1000-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel#174; Ethernet, visit http://communities.intel.com/community/wired
Re: [E1000-devel] [PATCH 0/2] fix kernel crash with macvtap on top of LRO
On Thu, 2013-02-07 at 23:33 +0200, Michael S. Tsirkin wrote: On Thu, Feb 07, 2013 at 01:14:20PM -0500, David Miller wrote: From: Ben Hutchings bhutchi...@solarflare.com Date: Thu, 7 Feb 2013 16:20:46 + 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? macvtap_skb_to_vnet_hdr() certainly seems to be trying to preserve all the packet information. 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. 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. -- 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 E1000-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel#174; Ethernet, visit http://communities.intel.com/community/wired
Re: [E1000-devel] [PATCH 0/2] fix kernel crash with macvtap on top of LRO
On Thu, 2013-02-07 at 01:02 +0200, Michael S. Tsirkin wrote: 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. If we want to allow forwarding from LRO then net/ipv4/inet_lro.c also needs to set gso_type. Ben. -- 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 E1000-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel#174; Ethernet, visit http://communities.intel.com/community/wired
Re: [E1000-devel] [PATCH 0/2] fix kernel crash with macvtap on top of LRO
On 02/07/2013 07:02 AM, Michael S. Tsirkin wrote: 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... diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c index b181dfb..b2c6227 100644 --- a/drivers/net/macvtap.c +++ b/drivers/net/macvtap.c @@ -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. -- 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 E1000-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel#174; Ethernet, visit http://communities.intel.com/community/wired
