[PATCH 0/2] hv_netvsc: linearize SKBs bigger than MAX_PAGE_BUFFER_COUNT-2 pages

[PATCH 0/2] hv_netvsc: linearize SKBs bigger than MAX_PAGE_BUFFER_COUNT-2 pages

[Vitaly Kuznetsov]

This patch series fixes the same issue which was fixed in Xen with commit
97a6d1bb2b658ac85ed88205ccd1ab809899884d ("xen-netfront: Fix handling packets on
compound pages with skb_linearize").

It is relatively easy to create a packet which is small in size but occupies
more than 30 (MAX_PAGE_BUFFER_COUNT-2) pages. Here is a kernel-mode reproducer
which tries sending a packet with only 34 bytes of payload (but on 34 pages)
and fails:

#include <linux/module.h>
#include <linux/init.h>
#include <linux/net.h>
#include <linux/in.h>
#include <net/sock.h>

static int __init sendfb_init(void)
{
	struct socket *sock;
	int i, ret;
	struct sockaddr_in in4_addr = { 0 };
	struct page *pages[17];
	unsigned long flags;

	ret = sock_create_kern(AF_INET, SOCK_STREAM, IPPROTO_TCP, &sock);
	if (ret) {
		pr_err("failed to create socket: %d!\n", ret);
		return ret;
	}

	in4_addr.sin_family = AF_INET;
	/* www.google.com, 74.125.133.99 */
	in4_addr.sin_addr.s_addr = cpu_to_be32(0x4a7d8563);
	in4_addr.sin_port = cpu_to_be16(80);

	ret = sock->ops->connect(sock, (struct sockaddr *)&in4_addr, sizeof(in4_addr), 0);
	if (ret) {
		pr_err("failed to connect: %d!\n", ret);
		return ret;
	}

	/* We can send up to 17 frags */
	flags = MSG_MORE;
	for (i = 0; i < 17; i++) {
		if (i == 16)
			flags = MSG_EOR;
		pages[i] = alloc_pages(GFP_KERNEL | __GFP_COMP, 1);
		if (!pages[i]) {
			pr_err("out of memory!");
			goto free_pages;
		}
		sock->ops->sendpage(sock, pages[i], PAGE_SIZE -1, 2, flags);
	}

free_pages:
	for (; i > 0; i--)
		__free_pages(pages[i - 1], 1);

	printk("sendfb_init: test done\n");
        return -1;
}

module_init(sendfb_init);

MODULE_LICENSE("GPL");

A try to load such module results in multiple
'kernel: hv_netvsc vmbus_15 eth0: Packet too big: 100' messages as all retries
fail as well. It should also be possible to trigger the issue from userspace, I
expect e.g. NFS under heavy load to get stuck sometimes.

Vitaly Kuznetsov (2):
  hv_netvsc: use single existing drop path in netvsc_start_xmit
  hv_netvsc: try linearizing big SKBs before dropping them

 drivers/net/hyperv/netvsc_drv.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

-- 
1.9.3

[PATCH 1/2] hv_netvsc: use single existing drop path in netvsc_start_xmit

[Vitaly Kuznetsov]

... which validly uses dev_kfree_skb_any() instead of dev_kfree_skb().

Setting ret to -EFAULT and -ENOMEM have no real meaning here (we need to set
it to anything but -EAGAIN) as we drop the packet and return NETDEV_TX_OK
anyway.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/net/hyperv/netvsc_drv.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index e5fa094..9e4230d 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -370,7 +370,7 @@ not_ip:
 static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
 {
 	struct net_device_context *net_device_ctx = netdev_priv(net);
-	struct hv_netvsc_packet *packet;
+	struct hv_netvsc_packet *packet = NULL;
 	int ret;
 	unsigned int num_data_pgs;
 	struct rndis_message *rndis_msg;
@@ -396,9 +396,8 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
 	num_data_pgs = netvsc_get_slots(skb) + 2;
 	if (num_data_pgs > MAX_PAGE_BUFFER_COUNT) {
 		netdev_err(net, "Packet too big: %u\n", skb->len);
-		dev_kfree_skb(skb);
-		net->stats.tx_dropped++;
-		return NETDEV_TX_OK;
+		ret = -EFAULT;
+		goto drop;
 	}
 
 	pkt_sz = sizeof(struct hv_netvsc_packet) + RNDIS_AND_PPI_SIZE;
@@ -408,9 +407,8 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
 		if (!packet) {
 			/* out of memory, drop packet */
 			netdev_err(net, "unable to alloc hv_netvsc_packet\n");
-			dev_kfree_skb(skb);
-			net->stats.tx_dropped++;
-			return NETDEV_TX_OK;
+			ret = -ENOMEM;
+			goto drop;
 		}
 		packet->part_of_skb = false;
 	} else {
@@ -574,7 +572,7 @@ drop:
 		net->stats.tx_bytes += skb_length;
 		net->stats.tx_packets++;
 	} else {
-		if (!packet->part_of_skb)
+		if (packet && !packet->part_of_skb)
 			kfree(packet);
 		if (ret != -EAGAIN) {
 			dev_kfree_skb_any(skb);
-- 
1.9.3

[PATCH 2/2] hv_netvsc: try linearizing big SKBs before dropping them

[Vitaly Kuznetsov]

In netvsc_start_xmit() we can handle packets which are scattered around not
more than MAX_PAGE_BUFFER_COUNT-2 pages. It is, however, easy to create a
packet which is not big in size but occupies more pages (e.g. if it uses frags
on compound pages boundaries). When we drop such packet it cases sender to try
resending it but in most cases it will try resending the same packet which will
also get dropped, this will cause the particular connection to stick. To solve
the issue we can try linearizing skb.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/net/hyperv/netvsc_drv.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 9e4230d..4487167 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -377,27 +377,42 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
 	struct rndis_packet *rndis_pkt;
 	u32 rndis_msg_size;
 	bool isvlan;
+	bool linear = false;
 	struct rndis_per_packet_info *ppi;
 	struct ndis_tcp_ip_checksum_info *csum_info;
 	struct ndis_tcp_lso_info *lso_info;
 	int  hdr_offset;
 	u32 net_trans_info;
 	u32 hash;
-	u32 skb_length = skb->len;
-	u32 head_room = skb_headroom(skb);
+	u32 skb_length;
+	u32 head_room;
 	u32 pkt_sz;
 	struct hv_page_buffer page_buf[MAX_PAGE_BUFFER_COUNT];
 
 
 	/* We will atmost need two pages to describe the rndis
 	 * header. We can only transmit MAX_PAGE_BUFFER_COUNT number
-	 * of pages in a single packet.
+	 * of pages in a single packet. If skb is scattered around
+	 * more pages we try linearizing it.
 	 */
+
+check_size:
+	skb_length = skb->len;
+	head_room = skb_headroom(skb);
 	num_data_pgs = netvsc_get_slots(skb) + 2;
-	if (num_data_pgs > MAX_PAGE_BUFFER_COUNT) {
-		netdev_err(net, "Packet too big: %u\n", skb->len);
+	if (num_data_pgs > MAX_PAGE_BUFFER_COUNT && linear) {
+		net_alert_ratelimited("packet too big: %u pages (%u bytes)\n",
+				      num_data_pgs, skb->len);
 		ret = -EFAULT;
 		goto drop;
+	} else if (num_data_pgs > MAX_PAGE_BUFFER_COUNT) {
+		if (skb_linearize(skb)) {
+			net_alert_ratelimited("failed to linearize skb\n");
+			ret = -ENOMEM;
+			goto drop;
+		}
+		linear = true;
+		goto check_size;
 	}
 
 	pkt_sz = sizeof(struct hv_netvsc_packet) + RNDIS_AND_PPI_SIZE;
-- 
1.9.3

Re: [PATCH 0/2] hv_netvsc: linearize SKBs bigger than MAX_PAGE_BUFFER_COUNT-2 pages

[David Miller]

From: Vitaly Kuznetsov <vkuznets@redhat.com>
Date: Wed,  8 Apr 2015 17:54:04 +0200

> This patch series fixes the same issue which was fixed in Xen with commit
> 97a6d1bb2b658ac85ed88205ccd1ab809899884d ("xen-netfront: Fix handling packets on
> compound pages with skb_linearize").

This patch series only applies on net-next, so that's where I put it.

Please be completely explicit about this in the future, rather than
forcing me to try and figure it out.

Thanks.

Re: [PATCH 0/2] hv_netvsc: linearize SKBs bigger than MAX_PAGE_BUFFER_COUNT-2 pages

[Vitaly Kuznetsov]

David Miller <davem@davemloft.net> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com>
> Date: Wed,  8 Apr 2015 17:54:04 +0200
>
>> This patch series fixes the same issue which was fixed in Xen with commit
>> 97a6d1bb2b658ac85ed88205ccd1ab809899884d ("xen-netfront: Fix handling packets on
>> compound pages with skb_linearize").
>
> This patch series only applies on net-next, so that's where I put it.

Yes, it is for net-next, thanks.

>
> Please be completely explicit about this in the future, rather than
> forcing me to try and figure it out.

Sorry, I will, for some reason I thought it was the default.

>
> Thanks.

-- 
  Vitaly