[PATCHv2 NEXT 0/5]qlcnic: vlan rx accleration support

[PATCHv2 NEXT 0/5]qlcnic: vlan rx accleration support

[Amit Kumar Salecha]

Hi
  Series v2 of 5 patches to support vlan rx accleration.
  This takes care of Eric Dumazet comment of using dev_kfree_skb(skb).
 
  I haven't combined accleration and GRO patch. Though its look strange
  with first patch. But I want to have separate patches for two different 
  entity.

-Amit

[PATCHv2 NEXT 1/5] qlcnic: support vlan rx accleration

[Amit Kumar Salecha]

Implemented vlan rx accleration in driver.
This helps in increasing significant performance and
reduces cpu utilization with GRO and LRO.

Eric Dumazet:
	"Its a bit strange you use dev_kfree_skb_any(skb) here."
	"We run in NAPI mode, so you can use dev_kfree_skb()."
Amit:
	Done. Using dev_kfree_skb();

Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
---
 drivers/net/qlcnic/qlcnic.h      |    1 +
 drivers/net/qlcnic/qlcnic_init.c |   59 ++++++++++++++++++++++----------------
 drivers/net/qlcnic/qlcnic_main.c |   10 ++++++-
 3 files changed, 44 insertions(+), 26 deletions(-)

diff --git a/drivers/net/qlcnic/qlcnic.h b/drivers/net/qlcnic/qlcnic.h
index cc8385a..c8caec9 100644
--- a/drivers/net/qlcnic/qlcnic.h
+++ b/drivers/net/qlcnic/qlcnic.h
@@ -1013,6 +1013,7 @@ struct qlcnic_adapter {
 
 	u64 dev_rst_time;
 
+	struct vlan_group *vlgrp;
 	struct qlcnic_npar_info *npars;
 	struct qlcnic_eswitch *eswitch;
 	struct qlcnic_nic_template *nic_ops;
diff --git a/drivers/net/qlcnic/qlcnic_init.c b/drivers/net/qlcnic/qlcnic_init.c
index 26a7d6b..10cebb1 100644
--- a/drivers/net/qlcnic/qlcnic_init.c
+++ b/drivers/net/qlcnic/qlcnic_init.c
@@ -1380,24 +1380,28 @@ static struct sk_buff *qlcnic_process_rxbuf(struct qlcnic_adapter *adapter,
 }
 
 static int
-qlcnic_check_rx_tagging(struct qlcnic_adapter *adapter, struct sk_buff *skb)
+qlcnic_check_rx_tagging(struct qlcnic_adapter *adapter, struct sk_buff *skb,
+			u16 *vlan_tag)
 {
-	u16 vlan_tag;
 	struct ethhdr *eth_hdr;
 
-	if (!__vlan_get_tag(skb, &vlan_tag)) {
-		if (vlan_tag == adapter->pvid) {
-			/* strip the tag from the packet and send it up */
-			eth_hdr = (struct ethhdr *) skb->data;
-			memmove(skb->data + VLAN_HLEN, eth_hdr, ETH_ALEN * 2);
-			skb_pull(skb, VLAN_HLEN);
-			return 0;
-		}
+	if (!__vlan_get_tag(skb, vlan_tag)) {
+		eth_hdr = (struct ethhdr *) skb->data;
+		memmove(skb->data + VLAN_HLEN, eth_hdr, ETH_ALEN * 2);
+		skb_pull(skb, VLAN_HLEN);
+	}
+	if (!adapter->pvid)
+		return 0;
+
+	if (*vlan_tag == adapter->pvid) {
+		/* Outer vlan tag. Packet should follow non-vlan path */
+		*vlan_tag = 0xffff;
+		return 0;
 	}
 	if (adapter->flags & QLCNIC_TAGGING_ENABLED)
 		return 0;
 
-	return -EIO;
+	return -EINVAL;
 }
 
 static struct qlcnic_rx_buffer *
@@ -1411,6 +1415,7 @@ qlcnic_process_rcv(struct qlcnic_adapter *adapter,
 	struct sk_buff *skb;
 	struct qlcnic_host_rds_ring *rds_ring;
 	int index, length, cksum, pkt_offset;
+	u16 vid = 0xffff;
 
 	if (unlikely(ring >= adapter->max_rds_rings))
 		return NULL;
@@ -1441,17 +1446,18 @@ qlcnic_process_rcv(struct qlcnic_adapter *adapter,
 
 	skb->truesize = skb->len + sizeof(struct sk_buff);
 
-	if (unlikely(adapter->pvid)) {
-		if (qlcnic_check_rx_tagging(adapter, skb)) {
-			adapter->stats.rxdropped++;
-			dev_kfree_skb_any(skb);
-			return buffer;
-		}
+	if (unlikely(qlcnic_check_rx_tagging(adapter, skb, &vid))) {
+		adapter->stats.rxdropped++;
+		dev_kfree_skb(skb);
+		return buffer;
 	}
 
 	skb->protocol = eth_type_trans(skb, netdev);
 
-	napi_gro_receive(&sds_ring->napi, skb);
+	if ((vid != 0xffff) && adapter->vlgrp)
+		vlan_hwaccel_receive_skb(skb, adapter->vlgrp, vid);
+	else
+		napi_gro_receive(&sds_ring->napi, skb);
 
 	adapter->stats.rx_pkts++;
 	adapter->stats.rxbytes += length;
@@ -1480,6 +1486,7 @@ qlcnic_process_lro(struct qlcnic_adapter *adapter,
 	int index;
 	u16 lro_length, length, data_offset;
 	u32 seq_number;
+	u16 vid = 0xffff;
 
 	if (unlikely(ring > adapter->max_rds_rings))
 		return NULL;
@@ -1514,13 +1521,12 @@ qlcnic_process_lro(struct qlcnic_adapter *adapter,
 
 	skb_pull(skb, l2_hdr_offset);
 
-	if (unlikely(adapter->pvid)) {
-		if (qlcnic_check_rx_tagging(adapter, skb)) {
-			adapter->stats.rxdropped++;
-			dev_kfree_skb_any(skb);
-			return buffer;
-		}
+	if (unlikely(qlcnic_check_rx_tagging(adapter, skb, &vid))) {
+		adapter->stats.rxdropped++;
+		dev_kfree_skb(skb);
+		return buffer;
 	}
+
 	skb->protocol = eth_type_trans(skb, netdev);
 
 	iph = (struct iphdr *)skb->data;
@@ -1535,7 +1541,10 @@ qlcnic_process_lro(struct qlcnic_adapter *adapter,
 
 	length = skb->len;
 
-	netif_receive_skb(skb);
+	if ((vid != 0xffff) && adapter->vlgrp)
+		vlan_hwaccel_receive_skb(skb, adapter->vlgrp, vid);
+	else
+		netif_receive_skb(skb);
 
 	adapter->stats.lro_pkts++;
 	adapter->stats.lrobytes += length;
diff --git a/drivers/net/qlcnic/qlcnic_main.c b/drivers/net/qlcnic/qlcnic_main.c
index 5fd2abd..9eb0ced 100644
--- a/drivers/net/qlcnic/qlcnic_main.c
+++ b/drivers/net/qlcnic/qlcnic_main.c
@@ -371,6 +371,13 @@ static int qlcnic_set_mac(struct net_device *netdev, void *p)
 	return 0;
 }
 
+static void qlcnic_vlan_rx_register(struct net_device *netdev,
+		struct vlan_group *grp)
+{
+	struct qlcnic_adapter *adapter = netdev_priv(netdev);
+	adapter->vlgrp = grp;
+}
+
 static const struct net_device_ops qlcnic_netdev_ops = {
 	.ndo_open	   = qlcnic_open,
 	.ndo_stop	   = qlcnic_close,
@@ -381,6 +388,7 @@ static const struct net_device_ops qlcnic_netdev_ops = {
 	.ndo_set_mac_address    = qlcnic_set_mac,
 	.ndo_change_mtu	   = qlcnic_change_mtu,
 	.ndo_tx_timeout	   = qlcnic_tx_timeout,
+	.ndo_vlan_rx_register = qlcnic_vlan_rx_register,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller = qlcnic_poll_controller,
 #endif
@@ -1446,7 +1454,7 @@ qlcnic_setup_netdev(struct qlcnic_adapter *adapter,
 	SET_ETHTOOL_OPS(netdev, &qlcnic_ethtool_ops);
 
 	netdev->features |= (NETIF_F_SG | NETIF_F_IP_CSUM |
-		NETIF_F_IPV6_CSUM | NETIF_F_GRO);
+		NETIF_F_IPV6_CSUM | NETIF_F_GRO | NETIF_F_HW_VLAN_RX);
 	netdev->vlan_features |= (NETIF_F_SG | NETIF_F_IP_CSUM |
 		NETIF_F_IPV6_CSUM);
 
-- 
1.6.0.2

[PATCHv2 NEXT 2/5] qlcnic: vlan gro support

[Amit Kumar Salecha]

GRO support + vlan rx accleration, boost around 9%
performance and reduces 25% of cpu utilization on vlan
interface.

Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
---
 drivers/net/qlcnic/qlcnic_init.c |    2 +-
 drivers/net/qlcnic/qlcnic_main.c |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/qlcnic/qlcnic_init.c b/drivers/net/qlcnic/qlcnic_init.c
index 10cebb1..9f994b9 100644
--- a/drivers/net/qlcnic/qlcnic_init.c
+++ b/drivers/net/qlcnic/qlcnic_init.c
@@ -1455,7 +1455,7 @@ qlcnic_process_rcv(struct qlcnic_adapter *adapter,
 	skb->protocol = eth_type_trans(skb, netdev);
 
 	if ((vid != 0xffff) && adapter->vlgrp)
-		vlan_hwaccel_receive_skb(skb, adapter->vlgrp, vid);
+		vlan_gro_receive(&sds_ring->napi, adapter->vlgrp, vid, skb);
 	else
 		napi_gro_receive(&sds_ring->napi, skb);
 
diff --git a/drivers/net/qlcnic/qlcnic_main.c b/drivers/net/qlcnic/qlcnic_main.c
index 9eb0ced..f7eb30b 100644
--- a/drivers/net/qlcnic/qlcnic_main.c
+++ b/drivers/net/qlcnic/qlcnic_main.c
@@ -796,7 +796,7 @@ qlcnic_set_netdev_features(struct qlcnic_adapter *adapter,
 	features = (NETIF_F_SG | NETIF_F_IP_CSUM |
 			NETIF_F_IPV6_CSUM | NETIF_F_GRO);
 	vlan_features = (NETIF_F_SG | NETIF_F_IP_CSUM |
-			NETIF_F_IPV6_CSUM);
+			NETIF_F_IPV6_CSUM | NETIF_F_GRO);
 
 	if (adapter->capabilities & QLCNIC_FW_CAPABILITY_TSO) {
 		features |= (NETIF_F_TSO | NETIF_F_TSO6);
@@ -1456,7 +1456,7 @@ qlcnic_setup_netdev(struct qlcnic_adapter *adapter,
 	netdev->features |= (NETIF_F_SG | NETIF_F_IP_CSUM |
 		NETIF_F_IPV6_CSUM | NETIF_F_GRO | NETIF_F_HW_VLAN_RX);
 	netdev->vlan_features |= (NETIF_F_SG | NETIF_F_IP_CSUM |
-		NETIF_F_IPV6_CSUM);
+		NETIF_F_IPV6_CSUM | NETIF_F_GRO);
 
 	if (adapter->capabilities & QLCNIC_FW_CAPABILITY_TSO) {
 		netdev->features |= (NETIF_F_TSO | NETIF_F_TSO6);
-- 
1.6.0.2

[PATCHv2 NEXT 3/5] qlcnic: vlan lro support

[Amit Kumar Salecha]

LRO + GRO + vlan rx accleration support, performance increases
around 20% and cpu utilization reduces around 70% on vlan interface.

Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
---
 drivers/net/qlcnic/qlcnic_main.c |   39 +++++++++++++++++++++++++++++--------
 1 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/net/qlcnic/qlcnic_main.c b/drivers/net/qlcnic/qlcnic_main.c
index f7eb30b..ea729a5 100644
--- a/drivers/net/qlcnic/qlcnic_main.c
+++ b/drivers/net/qlcnic/qlcnic_main.c
@@ -107,7 +107,7 @@ static irqreturn_t qlcnic_msi_intr(int irq, void *data);
 static irqreturn_t qlcnic_msix_intr(int irq, void *data);
 
 static struct net_device_stats *qlcnic_get_stats(struct net_device *netdev);
-static void qlcnic_config_indev_addr(struct net_device *dev, unsigned long);
+static void qlcnic_restore_indev_addr(struct net_device *dev, unsigned long);
 static int qlcnic_start_firmware(struct qlcnic_adapter *);
 
 static void qlcnic_alloc_lb_filters_mem(struct qlcnic_adapter *adapter);
@@ -1759,7 +1759,7 @@ qlcnic_resume(struct pci_dev *pdev)
 		if (err)
 			goto done;
 
-		qlcnic_config_indev_addr(netdev, NETDEV_UP);
+		qlcnic_restore_indev_addr(netdev, NETDEV_UP);
 	}
 done:
 	netif_device_attach(netdev);
@@ -2958,7 +2958,7 @@ attach:
 		if (qlcnic_up(adapter, netdev))
 			goto done;
 
-		qlcnic_config_indev_addr(netdev, NETDEV_UP);
+		qlcnic_restore_indev_addr(netdev, NETDEV_UP);
 	}
 
 done:
@@ -3120,7 +3120,7 @@ static int qlcnic_attach_func(struct pci_dev *pdev)
 		if (err)
 			goto done;
 
-		qlcnic_config_indev_addr(netdev, NETDEV_UP);
+		qlcnic_restore_indev_addr(netdev, NETDEV_UP);
 	}
  done:
 	netif_device_attach(netdev);
@@ -4035,10 +4035,10 @@ qlcnic_remove_diag_entries(struct qlcnic_adapter *adapter)
 #define is_qlcnic_netdev(dev) (dev->netdev_ops == &qlcnic_netdev_ops)
 
 static void
-qlcnic_config_indev_addr(struct net_device *dev, unsigned long event)
+qlcnic_config_indev_addr(struct qlcnic_adapter *adapter,
+			struct net_device *dev, unsigned long event)
 {
 	struct in_device *indev;
-	struct qlcnic_adapter *adapter = netdev_priv(dev);
 
 	indev = in_dev_get(dev);
 	if (!indev)
@@ -4062,6 +4062,27 @@ qlcnic_config_indev_addr(struct net_device *dev, unsigned long event)
 	in_dev_put(indev);
 }
 
+static void
+qlcnic_restore_indev_addr(struct net_device *netdev, unsigned long event)
+{
+	struct qlcnic_adapter *adapter = netdev_priv(netdev);
+	struct net_device *dev;
+	u16 vid;
+
+	qlcnic_config_indev_addr(adapter, netdev, event);
+
+	if (!adapter->vlgrp)
+		return;
+
+	for (vid = 0; vid < VLAN_GROUP_ARRAY_LEN; vid++) {
+		dev = vlan_group_get_device(adapter->vlgrp, vid);
+		if (!dev)
+			continue;
+
+		qlcnic_config_indev_addr(adapter, dev, event);
+	}
+}
+
 static int qlcnic_netdev_event(struct notifier_block *this,
 				 unsigned long event, void *ptr)
 {
@@ -4088,7 +4109,7 @@ recheck:
 	if (!test_bit(__QLCNIC_DEV_UP, &adapter->state))
 		goto done;
 
-	qlcnic_config_indev_addr(dev, event);
+	qlcnic_config_indev_addr(adapter, dev, event);
 done:
 	return NOTIFY_DONE;
 }
@@ -4105,7 +4126,7 @@ qlcnic_inetaddr_event(struct notifier_block *this,
 	dev = ifa->ifa_dev ? ifa->ifa_dev->dev : NULL;
 
 recheck:
-	if (dev == NULL || !netif_running(dev))
+	if (dev == NULL)
 		goto done;
 
 	if (dev->priv_flags & IFF_802_1Q_VLAN) {
@@ -4148,7 +4169,7 @@ static struct notifier_block qlcnic_inetaddr_cb = {
 };
 #else
 static void
-qlcnic_config_indev_addr(struct net_device *dev, unsigned long event)
+qlcnic_restore_indev_addr(struct net_device *dev, unsigned long event)
 { }
 #endif
 static struct pci_error_handlers qlcnic_err_handler = {
-- 
1.6.0.2

[PATCHv2 NEXT 4/5] qlcnic: remove fw version check

[Amit Kumar Salecha]

Don't compare flash and file fw version. Allow to load
old fw from file than flashed fw.
If file fw is present, don't skip fw re-intialization.

Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
---
 drivers/net/qlcnic/qlcnic_init.c |   30 ++----------------------------
 1 files changed, 2 insertions(+), 28 deletions(-)

diff --git a/drivers/net/qlcnic/qlcnic_init.c b/drivers/net/qlcnic/qlcnic_init.c
index 9f994b9..e26fa95 100644
--- a/drivers/net/qlcnic/qlcnic_init.c
+++ b/drivers/net/qlcnic/qlcnic_init.c
@@ -1015,8 +1015,6 @@ qlcnic_check_fw_hearbeat(struct qlcnic_adapter *adapter)
 int
 qlcnic_need_fw_reset(struct qlcnic_adapter *adapter)
 {
-	u32 val, version, major, minor, build;
-
 	if (qlcnic_check_fw_hearbeat(adapter)) {
 		qlcnic_rom_lock_recovery(adapter);
 		return 1;
@@ -1025,20 +1023,8 @@ qlcnic_need_fw_reset(struct qlcnic_adapter *adapter)
 	if (adapter->need_fw_reset)
 		return 1;
 
-	/* check if we have got newer or different file firmware */
-	if (adapter->fw) {
-
-		val = qlcnic_get_fw_version(adapter);
-
-		version = QLCNIC_DECODE_VERSION(val);
-
-		major = QLCRD32(adapter, QLCNIC_FW_VERSION_MAJOR);
-		minor = QLCRD32(adapter, QLCNIC_FW_VERSION_MINOR);
-		build = QLCRD32(adapter, QLCNIC_FW_VERSION_SUB);
-
-		if (version > QLCNIC_VERSION_CODE(major, minor, build))
-			return 1;
-	}
+	if (adapter->fw)
+		return 1;
 
 	return 0;
 }
@@ -1174,18 +1160,6 @@ qlcnic_validate_firmware(struct qlcnic_adapter *adapter)
 		return -EINVAL;
 	}
 
-	/* check if flashed firmware is newer */
-	if (qlcnic_rom_fast_read(adapter,
-			QLCNIC_FW_VERSION_OFFSET, (int *)&val))
-		return -EIO;
-
-	val = QLCNIC_DECODE_VERSION(val);
-	if (val > ver) {
-		dev_info(&pdev->dev, "%s: firmware is older than flash\n",
-				fw_name[fw_type]);
-		return -EINVAL;
-	}
-
 	QLCWR32(adapter, QLCNIC_CAM_RAM(0x1fc), QLCNIC_BDINFO_MAGIC);
 	return 0;
 }
-- 
1.6.0.2

[PATCHv2 NEXT 5/5] qlcnic: update version 5.0.10

[Amit Kumar Salecha]

Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
---
 drivers/net/qlcnic/qlcnic.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/qlcnic/qlcnic.h b/drivers/net/qlcnic/qlcnic.h
index c8caec9..714ddf4 100644
--- a/drivers/net/qlcnic/qlcnic.h
+++ b/drivers/net/qlcnic/qlcnic.h
@@ -51,8 +51,8 @@
 
 #define _QLCNIC_LINUX_MAJOR 5
 #define _QLCNIC_LINUX_MINOR 0
-#define _QLCNIC_LINUX_SUBVERSION 9
-#define QLCNIC_LINUX_VERSIONID  "5.0.9"
+#define _QLCNIC_LINUX_SUBVERSION 10
+#define QLCNIC_LINUX_VERSIONID  "5.0.10"
 #define QLCNIC_DRV_IDC_VER  0x01
 #define QLCNIC_DRIVER_VERSION  ((_QLCNIC_LINUX_MAJOR << 16) |\
 		 (_QLCNIC_LINUX_MINOR << 8) | (_QLCNIC_LINUX_SUBVERSION))
-- 
1.6.0.2

Re: [PATCHv2 NEXT 1/5] qlcnic: support vlan rx accleration

[Eric Dumazet]

Le jeudi 16 septembre 2010 à 22:14 -0700, Amit Kumar Salecha a écrit :
> Implemented vlan rx accleration in driver.
> This helps in increasing significant performance and
> reduces cpu utilization with GRO and LRO.
> 
> Eric Dumazet:
> 	"Its a bit strange you use dev_kfree_skb_any(skb) here."
> 	"We run in NAPI mode, so you can use dev_kfree_skb()."
> Amit:
> 	Done. Using dev_kfree_skb();
> 
> Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

[PATCH] qlcnic: dont assume NET_IP_ALIGN is 2

[Eric Dumazet]

Amit, I noticed following bug in qlnic driver.

Also, skb->truesize should not be changed by drivers, unless dealing
with fragments.

When you have :
	skb->truesize = skb->len + sizeof(struct sk_buff);

you are lying to stack that can queue many "small" UDP packets,
accouting for small packets in socket rcvbuf, while the truesize was
really 1532 + sizeof(struct sk_buff)

Could you take a look ?

Thanks


[PATCH] qlcnic: dont assume NET_IP_ALIGN is 2

qlcnic driver allocates rx skbs and gives to hardware too bytes of extra
storage, allowing for corruption of kernel data.

NET_IP_ALIGN being 0 on some platforms (including x86), drivers should
not assume it's 2.

rds_ring->skb_size = rds_ring->dma_size + NET_IP_ALIGN;
...
skb = dev_alloc_skb(rds_ring->skb_size);
skb_reserve(skb, 2);
pci_map_single(pdev, skb->data, rds_ring->dma_size, PCI_DMA_FROMDEVICE);

(and rds_ring->skb_size == rds_ring->dma_size) -> bug


Because of extra alignment (1500 + 32) -> four extra bytes are available
before the struct skb_shared_info, so corruption is not noticed.

Note: this driver could use netdev_alloc_skb_ip_align()

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/drivers/net/qlcnic/qlcnic_init.c b/drivers/net/qlcnic/qlcnic_init.c
index 75ba744..60ab753 100644
--- a/drivers/net/qlcnic/qlcnic_init.c
+++ b/drivers/net/qlcnic/qlcnic_init.c
@@ -1316,7 +1316,7 @@ qlcnic_alloc_rx_skb(struct qlcnic_adapter *adapter,
 		return -ENOMEM;
 	}
 
-	skb_reserve(skb, 2);
+	skb_reserve(skb, NET_IP_ALIGN);
 
 	dma = pci_map_single(pdev, skb->data,
 			rds_ring->dma_size, PCI_DMA_FROMDEVICE);

RE: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2

[Amit Salecha]

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Friday, September 17, 2010 3:27 PM
> To: Amit Salecha
> Cc: netdev@vger.kernel.org; Ameen Rahman; Anirban Chakraborty; David
> Miller
> Subject: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2
> 
> Amit, I noticed following bug in qlnic driver.
> 
> Also, skb->truesize should not be changed by drivers, unless dealing
> with fragments.
> 
> When you have :
> 	skb->truesize = skb->len + sizeof(struct sk_buff);
> 
> you are lying to stack that can queue many "small" UDP packets,
> accouting for small packets in socket rcvbuf, while the truesize was
> really 1532 + sizeof(struct sk_buff)
> 
> Could you take a look ?

I will take a look and get back to you.

- Thanks.

> 
> Thanks
> 
> 
> [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2
> 
> qlcnic driver allocates rx skbs and gives to hardware too bytes of
> extra
> storage, allowing for corruption of kernel data.
> 
> NET_IP_ALIGN being 0 on some platforms (including x86), drivers should
> not assume it's 2.
> 
> rds_ring->skb_size = rds_ring->dma_size + NET_IP_ALIGN;
> ...
> skb = dev_alloc_skb(rds_ring->skb_size);
> skb_reserve(skb, 2);
> pci_map_single(pdev, skb->data, rds_ring->dma_size,
> PCI_DMA_FROMDEVICE);
> 
> (and rds_ring->skb_size == rds_ring->dma_size) -> bug
> 
> 
> Because of extra alignment (1500 + 32) -> four extra bytes are
> available
> before the struct skb_shared_info, so corruption is not noticed.
> 
> Note: this driver could use netdev_alloc_skb_ip_align()
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> diff --git a/drivers/net/qlcnic/qlcnic_init.c
> b/drivers/net/qlcnic/qlcnic_init.c
> index 75ba744..60ab753 100644
> --- a/drivers/net/qlcnic/qlcnic_init.c
> +++ b/drivers/net/qlcnic/qlcnic_init.c
> @@ -1316,7 +1316,7 @@ qlcnic_alloc_rx_skb(struct qlcnic_adapter
> *adapter,
>  		return -ENOMEM;
>  	}
> 
> -	skb_reserve(skb, 2);
> +	skb_reserve(skb, NET_IP_ALIGN);
> 
>  	dma = pci_map_single(pdev, skb->data,
>  			rds_ring->dma_size, PCI_DMA_FROMDEVICE);
> 
> 

Re: [PATCHv2 NEXT 2/5] qlcnic: vlan gro support

[David Miller]

From: Amit Kumar Salecha <amit.salecha@qlogic.com>
Date: Thu, 16 Sep 2010 22:14:40 -0700

> @@ -796,7 +796,7 @@ qlcnic_set_netdev_features(struct qlcnic_adapter *adapter,
>  	features = (NETIF_F_SG | NETIF_F_IP_CSUM |
>  			NETIF_F_IPV6_CSUM | NETIF_F_GRO);
>  	vlan_features = (NETIF_F_SG | NETIF_F_IP_CSUM |
> -			NETIF_F_IPV6_CSUM);
> +			NETIF_F_IPV6_CSUM | NETIF_F_GRO);
>  
>  	if (adapter->capabilities & QLCNIC_FW_CAPABILITY_TSO) {
>  		features |= (NETIF_F_TSO | NETIF_F_TSO6);
> @@ -1456,7 +1456,7 @@ qlcnic_setup_netdev(struct qlcnic_adapter *adapter,
>  	netdev->features |= (NETIF_F_SG | NETIF_F_IP_CSUM |
>  		NETIF_F_IPV6_CSUM | NETIF_F_GRO | NETIF_F_HW_VLAN_RX);
>  	netdev->vlan_features |= (NETIF_F_SG | NETIF_F_IP_CSUM |
> -		NETIF_F_IPV6_CSUM);
> +		NETIF_F_IPV6_CSUM | NETIF_F_GRO);
>  
>  	if (adapter->capabilities & QLCNIC_FW_CAPABILITY_TSO) {

Drivers no longer should set NETIF_F_GRO in their vlan_features
flags, the generic networking core does this for you now in
register_netdevice() in net-next-2.6

Please respin this patch with these settings removed.

Thanks.

Re: [PATCHv2 NEXT 2/5] qlcnic: vlan gro support

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Fri, 17 Sep 2010 11:24:55 -0700 (PDT)

> From: Amit Kumar Salecha <amit.salecha@qlogic.com>
> Date: Thu, 16 Sep 2010 22:14:40 -0700
> 
>> @@ -796,7 +796,7 @@ qlcnic_set_netdev_features(struct qlcnic_adapter *adapter,
>>  	features = (NETIF_F_SG | NETIF_F_IP_CSUM |
>>  			NETIF_F_IPV6_CSUM | NETIF_F_GRO);
>>  	vlan_features = (NETIF_F_SG | NETIF_F_IP_CSUM |
>> -			NETIF_F_IPV6_CSUM);
>> +			NETIF_F_IPV6_CSUM | NETIF_F_GRO);
>>  
>>  	if (adapter->capabilities & QLCNIC_FW_CAPABILITY_TSO) {
>>  		features |= (NETIF_F_TSO | NETIF_F_TSO6);
>> @@ -1456,7 +1456,7 @@ qlcnic_setup_netdev(struct qlcnic_adapter *adapter,
>>  	netdev->features |= (NETIF_F_SG | NETIF_F_IP_CSUM |
>>  		NETIF_F_IPV6_CSUM | NETIF_F_GRO | NETIF_F_HW_VLAN_RX);
>>  	netdev->vlan_features |= (NETIF_F_SG | NETIF_F_IP_CSUM |
>> -		NETIF_F_IPV6_CSUM);
>> +		NETIF_F_IPV6_CSUM | NETIF_F_GRO);
>>  
>>  	if (adapter->capabilities & QLCNIC_FW_CAPABILITY_TSO) {
> 
> Drivers no longer should set NETIF_F_GRO in their vlan_features
> flags, the generic networking core does this for you now in
> register_netdevice() in net-next-2.6
> 
> Please respin this patch with these settings removed.

Actually, nevermind, I'll take care of doing this for you, just be
aware of this new convention.

Thank you.

Re: [PATCHv2 NEXT 0/5]qlcnic: vlan rx accleration support

[David Miller]

From: Amit Kumar Salecha <amit.salecha@qlogic.com>
Date: Thu, 16 Sep 2010 22:14:38 -0700

> Hi
>   Series v2 of 5 patches to support vlan rx accleration.
>   This takes care of Eric Dumazet comment of using dev_kfree_skb(skb).
>  
>   I haven't combined accleration and GRO patch. Though its look strange
>   with first patch. But I want to have separate patches for two different 
>   entity.

All applied, thanks.

Re: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 17 Sep 2010 11:57:28 +0200

> [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2
> 
> qlcnic driver allocates rx skbs and gives to hardware too bytes of extra
> storage, allowing for corruption of kernel data.
> 
> NET_IP_ALIGN being 0 on some platforms (including x86), drivers should
> not assume it's 2.
> 
> rds_ring->skb_size = rds_ring->dma_size + NET_IP_ALIGN;
> ...
> skb = dev_alloc_skb(rds_ring->skb_size);
> skb_reserve(skb, 2);
> pci_map_single(pdev, skb->data, rds_ring->dma_size, PCI_DMA_FROMDEVICE);
> 
> (and rds_ring->skb_size == rds_ring->dma_size) -> bug
> 
> 
> Because of extra alignment (1500 + 32) -> four extra bytes are available
> before the struct skb_shared_info, so corruption is not noticed.
> 
> Note: this driver could use netdev_alloc_skb_ip_align()
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied to net-2.6, thanks Eric.

RE: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2

[Amit Salecha]

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Friday, September 17, 2010 3:27 PM
> To: Amit Salecha
> Cc: netdev@vger.kernel.org; Ameen Rahman; Anirban Chakraborty; David
> Miller
> Subject: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2
> 
> Amit, I noticed following bug in qlnic driver.
> 
> Also, skb->truesize should not be changed by drivers, unless dealing
> with fragments.
> 
> When you have :
> 	skb->truesize = skb->len + sizeof(struct sk_buff);
> 
> you are lying to stack that can queue many "small" UDP packets,
> accouting for small packets in socket rcvbuf, while the truesize was
> really 1532 + sizeof(struct sk_buff)
> 
> Could you take a look ?
> 
> 
This can be cleaned up. 
Though I have one doubt. We are allocating larger packets than the actual data used.
Doesn't it will break accounting ?

-Amit

RE: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2

[Eric Dumazet]

Le lundi 20 septembre 2010 à 06:16 -0500, Amit Salecha a écrit :

> This can be cleaned up. 
> Though I have one doubt. We are allocating larger packets than the actual data used.
> Doesn't it will break accounting ?

truesize accounts for the real size of buffers, not the used part in
them.

IMHO, a driver not dealing with fragments should not touch skb->truesize

# grep truesize drivers/net/tg3.c
<nothing>

If a driver deals with fragments, it probably should use "+=" operator
only, not hardcoding sizeof(struct sk_buff) thing that only core network
has to deal with.

$ find drivers/net/bnx2x|xargs grep truesize
drivers/net/bnx2x/bnx2x_cmn.c:		skb->truesize += frag_len;

Almost all drivers are fine, they are some of them that should be
changed.

[PATCH net-next-2.6] qlnic: dont set skb->truesize

[Eric Dumazet]

Le lundi 20 septembre 2010 à 14:18 +0200, Eric Dumazet a écrit :
> Le lundi 20 septembre 2010 à 06:16 -0500, Amit Salecha a écrit :
> 
> > This can be cleaned up. 
> > Though I have one doubt. We are allocating larger packets than the actual data used.
> > Doesn't it will break accounting ?
> 
> truesize accounts for the real size of buffers, not the used part in
> them.
> 
> IMHO, a driver not dealing with fragments should not touch skb->truesize
> 
> # grep truesize drivers/net/tg3.c
> <nothing>
> 
> If a driver deals with fragments, it probably should use "+=" operator
> only, not hardcoding sizeof(struct sk_buff) thing that only core network
> has to deal with.
> 
> $ find drivers/net/bnx2x|xargs grep truesize
> drivers/net/bnx2x/bnx2x_cmn.c:		skb->truesize += frag_len;
> 
> Almost all drivers are fine, they are some of them that should be
> changed.
> 
> 

I suspect following patch should be fine :

[PATCH net-next-2.6] qlnic: dont set skb->truesize

skb->truesize is set in core network.

Dont change it unless dealing with fragments.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 drivers/net/qlcnic/qlcnic_init.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/qlcnic/qlcnic_init.c b/drivers/net/qlcnic/qlcnic_init.c
index e26fa95..16dd9eb 100644
--- a/drivers/net/qlcnic/qlcnic_init.c
+++ b/drivers/net/qlcnic/qlcnic_init.c
@@ -1418,8 +1418,6 @@ qlcnic_process_rcv(struct qlcnic_adapter *adapter,
 	if (pkt_offset)
 		skb_pull(skb, pkt_offset);
 
-	skb->truesize = skb->len + sizeof(struct sk_buff);
-
 	if (unlikely(qlcnic_check_rx_tagging(adapter, skb, &vid))) {
 		adapter->stats.rxdropped++;
 		dev_kfree_skb(skb);
@@ -1491,8 +1489,6 @@ qlcnic_process_lro(struct qlcnic_adapter *adapter,
 
 	skb_put(skb, lro_length + data_offset);
 
-	skb->truesize = skb->len + sizeof(struct sk_buff) + skb_headroom(skb);
-
 	skb_pull(skb, l2_hdr_offset);
 
 	if (unlikely(qlcnic_check_rx_tagging(adapter, skb, &vid))) {
@@ -1732,8 +1728,6 @@ qlcnic_process_rcv_diag(struct qlcnic_adapter *adapter,
 	if (pkt_offset)
 		skb_pull(skb, pkt_offset);
 
-	skb->truesize = skb->len + sizeof(struct sk_buff);
-
 	if (!qlcnic_check_loopback_buff(skb->data))
 		adapter->diag_cnt++;
 

Re: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2

[David Miller]

From: Amit Salecha <amit.salecha@qlogic.com>
Date: Mon, 20 Sep 2010 06:16:31 -0500

> Though I have one doubt. We are allocating larger packets than the actual data used.
> Doesn't it will break accounting ?

No, it will "fix" accounting.

You must charge to the SKB all of the non-shared memory that was
allocated to the SKB.

This means even if the packet only uses 128 bytes of the SKB
data area, you must still account for the full blob of linear
memory that was allocated for the SKB data area in skb->truesize.

Otherwise remote attackers could consume enormous amounts of memory by
tricking our socket accounting via carefully sized packets.

Re: [PATCH net-next-2.6] qlnic: dont set skb->truesize

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 20 Sep 2010 14:28:59 +0200

> [PATCH net-next-2.6] qlnic: dont set skb->truesize
> 
> skb->truesize is set in core network.
> 
> Dont change it unless dealing with fragments.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Looks good to me too, applied thanks!

RE: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2

[Amit Salecha]

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Monday, September 20, 2010 9:29 PM
> To: Amit Salecha
> Cc: eric.dumazet@gmail.com; netdev@vger.kernel.org; Ameen Rahman;
> Anirban Chakraborty
> Subject: Re: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2
> 
> From: Amit Salecha <amit.salecha@qlogic.com>
> Date: Mon, 20 Sep 2010 06:16:31 -0500
> 
> > Though I have one doubt. We are allocating larger packets than the
> actual data used.
> > Doesn't it will break accounting ?
> 
> No, it will "fix" accounting.
> 
> You must charge to the SKB all of the non-shared memory that was
> allocated to the SKB.
> 
> This means even if the packet only uses 128 bytes of the SKB
> data area, you must still account for the full blob of linear
> memory that was allocated for the SKB data area in skb->truesize.
> 
> Otherwise remote attackers could consume enormous amounts of memory by
> tricking our socket accounting via carefully sized packets.

Wont this affect throughput ?
As problem discuss in this thread http://www.mail-archive.com/netdev@vger.kernel.org/msg06848.html, it can affect tcp window scaling.

-Amit

RE: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2

[Eric Dumazet]

Le mardi 21 septembre 2010 à 03:19 -0500, Amit Salecha a écrit :
> 
> > -----Original Message-----
> > From: David Miller [mailto:davem@davemloft.net]
> > Sent: Monday, September 20, 2010 9:29 PM
> > To: Amit Salecha
> > Cc: eric.dumazet@gmail.com; netdev@vger.kernel.org; Ameen Rahman;
> > Anirban Chakraborty
> > Subject: Re: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2
> > 
> > From: Amit Salecha <amit.salecha@qlogic.com>
> > Date: Mon, 20 Sep 2010 06:16:31 -0500
> > 
> > > Though I have one doubt. We are allocating larger packets than the
> > actual data used.
> > > Doesn't it will break accounting ?
> > 
> > No, it will "fix" accounting.
> > 
> > You must charge to the SKB all of the non-shared memory that was
> > allocated to the SKB.
> > 
> > This means even if the packet only uses 128 bytes of the SKB
> > data area, you must still account for the full blob of linear
> > memory that was allocated for the SKB data area in skb->truesize.
> > 
> > Otherwise remote attackers could consume enormous amounts of memory by
> > tricking our socket accounting via carefully sized packets.
> 
> Wont this affect throughput ?
> As problem discuss in this thread http://www.mail-archive.com/netdev@vger.kernel.org/msg06848.html, it can affect tcp window scaling.
> 

Amit, if you believe this is a problem, you should address it for all
NICS, not only qlcnic.

Qlcnic was lying to stack, because it consumed 2Kbytes blocs and
pretended they were consuming skb->len bytes.
(assuming MTU=1500, problem is worse if MTU is bigger)

So in order to improve "throughput", you were allowing for memory
exhaust and freeze of the _machine_ ?

RE: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2

[Amit Salecha]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Eric Dumazet
> Sent: Tuesday, September 21, 2010 2:05 PM
> To: Amit Salecha
> Cc: David Miller; netdev@vger.kernel.org; Ameen Rahman; Anirban
> Chakraborty
> Subject: RE: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2
> 
> Le mardi 21 septembre 2010 à 03:19 -0500, Amit Salecha a écrit :
> >
> > > -----Original Message-----
> > > From: David Miller [mailto:davem@davemloft.net]
> > > Sent: Monday, September 20, 2010 9:29 PM
> > > To: Amit Salecha
> > > Cc: eric.dumazet@gmail.com; netdev@vger.kernel.org; Ameen Rahman;
> > > Anirban Chakraborty
> > > Subject: Re: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2
> > >
> > > From: Amit Salecha <amit.salecha@qlogic.com>
> > > Date: Mon, 20 Sep 2010 06:16:31 -0500
> > >
> > > > Though I have one doubt. We are allocating larger packets than
> the
> > > actual data used.
> > > > Doesn't it will break accounting ?
> > >
> > > No, it will "fix" accounting.
> > >
> > > You must charge to the SKB all of the non-shared memory that was
> > > allocated to the SKB.
> > >
> > > This means even if the packet only uses 128 bytes of the SKB
> > > data area, you must still account for the full blob of linear
> > > memory that was allocated for the SKB data area in skb->truesize.
> > >
> > > Otherwise remote attackers could consume enormous amounts of memory
> by
> > > tricking our socket accounting via carefully sized packets.
> >
> > Wont this affect throughput ?
> > As problem discuss in this thread http://www.mail-
> archive.com/netdev@vger.kernel.org/msg06848.html, it can affect tcp
> window scaling.
> >
> 
> Amit, if you believe this is a problem, you should address it for all
> NICS, not only qlcnic.
> 
> Qlcnic was lying to stack, because it consumed 2Kbytes blocs and
> pretended they were consuming skb->len bytes.
> (assuming MTU=1500, problem is worse if MTU is bigger)
> 
> So in order to improve "throughput", you were allowing for memory
> exhaust and freeze of the _machine_ ?
>
This won't lead to such problem. truesize is used for accounting only.
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2

[Eric Dumazet]

Le mardi 21 septembre 2010 à 03:41 -0500, Amit Salecha a écrit :

> > Amit, if you believe this is a problem, you should address it for all
> > NICS, not only qlcnic.
> > 
> > Qlcnic was lying to stack, because it consumed 2Kbytes blocs and
> > pretended they were consuming skb->len bytes.
> > (assuming MTU=1500, problem is worse if MTU is bigger)
> > 
> > So in order to improve "throughput", you were allowing for memory
> > exhaust and freeze of the _machine_ ?
> >
> This won't lead to such problem. truesize is used for accounting only.

You must have big machines in your lab and never hit OOM ?

You really should take a look on various files in net/core, net/ipv4
trees. And files like "/proc/sys/net/tcp_mem", "/proc/sys/net/udp_mem"

In fact, truesize is _underestimated_ : (we dont account for struct
skb_shared_info) and kmalloc() rounding

We probably should use this patch (without having to check all possible
net drivers !)

Problem is this would slow down alloc_skb(), so this patch is not for
inclusion.

cheap alternative would be to use 

size + sizeof(struct sk_buff) + SKB_DATA_ALIGN(sizeof(struct skb_shared_info))

If you think about it, when 128bit arches come, truesize will grow anyway.
If some tuning is needed in our stack, we'll do it.

(socket api SO_RCVBUF/ SO_SNDBUF is the problem, because
 applications are not aware of packetization or kernel internals)

SOCK_MIN_RCVBUF is way too small, since sizeof(struct sk_buff) 
is already close to 256. I guess we cannot even receive a single frame.

 include/net/sock.h |    2 +-
 net/core/skbuff.c  |    2 +-
 net/core/sock.c    |    8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)


diff --git a/include/net/sock.h b/include/net/sock.h
index 8ae97c4..348fc9e 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1558,7 +1558,7 @@ static inline void sk_wake_async(struct sock *sk, int how, int band)
 }
 
 #define SOCK_MIN_SNDBUF 2048
-#define SOCK_MIN_RCVBUF 256
+#define SOCK_MIN_RCVBUF 1024
 
 static inline void sk_stream_moderate_sndbuf(struct sock *sk)
 {
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 752c197..5ab2e8e 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -196,7 +196,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
 	 * the tail pointer in struct sk_buff!
 	 */
 	memset(skb, 0, offsetof(struct sk_buff, tail));
-	skb->truesize = size + sizeof(struct sk_buff);
+	skb->truesize = ksize(data) + sizeof(struct sk_buff);
 	atomic_set(&skb->users, 1);
 	skb->head = data;
 	skb->data = data;
diff --git a/net/core/sock.c b/net/core/sock.c
index f3a06c4..803e041 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -535,10 +535,10 @@ int sock_setsockopt(struct socket *sock, int level, int optname,
 			val = sysctl_wmem_max;
 set_sndbuf:
 		sk->sk_userlocks |= SOCK_SNDBUF_LOCK;
-		if ((val * 2) < SOCK_MIN_SNDBUF)
+		if ((val * 4) < SOCK_MIN_SNDBUF)
 			sk->sk_sndbuf = SOCK_MIN_SNDBUF;
 		else
-			sk->sk_sndbuf = val * 2;
+			sk->sk_sndbuf = val * 4;
 
 		/*
 		 *	Wake up sending tasks if we
@@ -579,10 +579,10 @@ set_rcvbuf:
 		 * returning the value we actually used in getsockopt
 		 * is the most desirable behavior.
 		 */
-		if ((val * 2) < SOCK_MIN_RCVBUF)
+		if ((val * 4) < SOCK_MIN_RCVBUF)
 			sk->sk_rcvbuf = SOCK_MIN_RCVBUF;
 		else
-			sk->sk_rcvbuf = val * 2;
+			sk->sk_rcvbuf = val * 4;
 		break;
 
 	case SO_RCVBUFFORCE:

Re: [PATCH] qlcnic: dont assume NET_IP_ALIGN is 2

[David Miller]

From: Amit Salecha <amit.salecha@qlogic.com>
Date: Tue, 21 Sep 2010 03:41:42 -0500

>> So in order to improve "throughput", you were allowing for memory
>> exhaust and freeze of the _machine_ ?
>>
> This won't lead to such problem. truesize is used for accounting only.

Yes, it will.

Do you understand that we enforce both socket-level and system-wide
networking buffer usage in the stack?  And this limiting is based
upon skb->truesize and therefore only works if skb->truesize is
accurate?

It's meant to keep people from attacking a server and consuming large
percentages of system memory with networking buffer memory such that
other tasks cannot complete successfully.

And by mis-reporting the truesize you are subverting that entirely.

This qlcnic truesize bug is a huge security hole, can't you see this
now?

[PATCH] netxen: dont set skb->truesize

[Eric Dumazet]

Le mardi 21 septembre 2010 à 12:33 -0700, David Miller a écrit :
> From: Amit Salecha <amit.salecha@qlogic.com>
> Date: Tue, 21 Sep 2010 03:41:42 -0500
> 
> >> So in order to improve "throughput", you were allowing for memory
> >> exhaust and freeze of the _machine_ ?
> >>
> > This won't lead to such problem. truesize is used for accounting only.
> 
> Yes, it will.
> 
> Do you understand that we enforce both socket-level and system-wide
> networking buffer usage in the stack?  And this limiting is based
> upon skb->truesize and therefore only works if skb->truesize is
> accurate?
> 
> It's meant to keep people from attacking a server and consuming large
> percentages of system memory with networking buffer memory such that
> other tasks cannot complete successfully.
> 
> And by mis-reporting the truesize you are subverting that entirely.
> 
> This qlcnic truesize bug is a huge security hole, can't you see this
> now?
> 

BTW, drivers/net/netxen/netxen_nic_init.c has same problem.

[PATCH] netxen: dont set skb->truesize

skb->truesize is set in core network.

Dont change it unless dealing with fragments.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 drivers/net/netxen/netxen_nic_init.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/netxen/netxen_nic_init.c b/drivers/net/netxen/netxen_nic_init.c
index cabae7b..b075a35 100644
--- a/drivers/net/netxen/netxen_nic_init.c
+++ b/drivers/net/netxen/netxen_nic_init.c
@@ -1540,7 +1540,6 @@ netxen_process_rcv(struct netxen_adapter *adapter,
 	if (pkt_offset)
 		skb_pull(skb, pkt_offset);
 
-	skb->truesize = skb->len + sizeof(struct sk_buff);
 	skb->protocol = eth_type_trans(skb, netdev);
 
 	napi_gro_receive(&sds_ring->napi, skb);
@@ -1602,8 +1601,6 @@ netxen_process_lro(struct netxen_adapter *adapter,
 
 	skb_put(skb, lro_length + data_offset);
 
-	skb->truesize = skb->len + sizeof(struct sk_buff) + skb_headroom(skb);
-
 	skb_pull(skb, l2_hdr_offset);
 	skb->protocol = eth_type_trans(skb, netdev);
 

Re: [PATCH] netxen: dont set skb->truesize

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 21 Sep 2010 21:55:10 +0200

> BTW, drivers/net/netxen/netxen_nic_init.c has same problem.
> 
> [PATCH] netxen: dont set skb->truesize

Applied to net-2.6, and I backported the qlcnic patch in net-next-2.6
into net-2.6 too.

Thanks.