在 2021/4/23 下午12:19, Xuan Zhuo 写道:

On Fri, 23 Apr 2021 12:08:34 +0800, Jason Wang [off-list ref] wrote:

quoted

在 2021/4/22 下午11:16, Xuan Zhuo 写道:

quoted

When "headroom" > 0, the actual allocated memory space is the entire
page, so the address of the page should be used when passing it to
build_skb().

BUG: KASAN: use-after-free in skb_gro_receive (net/core/skbuff.c:4260)
Write of size 16 at addr ffff88811619fffc by task kworker/u9:0/534
CPU: 2 PID: 534 Comm: kworker/u9:0 Not tainted 5.12.0-rc7-custom-16372-gb150be05b806 #3382
Hardware name: QEMU MSN2700, BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
Workqueue: xprtiod xs_stream_data_receive_workfn [sunrpc]
Call Trace:
   <IRQ>
dump_stack (lib/dump_stack.c:122)
print_address_description.constprop.0 (mm/kasan/report.c:233)
kasan_report.cold (mm/kasan/report.c:400 mm/kasan/report.c:416)
skb_gro_receive (net/core/skbuff.c:4260)
tcp_gro_receive (net/ipv4/tcp_offload.c:266 (discriminator 1))
tcp4_gro_receive (net/ipv4/tcp_offload.c:316)
inet_gro_receive (net/ipv4/af_inet.c:1545 (discriminator 2))
dev_gro_receive (net/core/dev.c:6075)
napi_gro_receive (net/core/dev.c:6168 net/core/dev.c:6198)
receive_buf (drivers/net/virtio_net.c:1151) virtio_net
virtnet_poll (drivers/net/virtio_net.c:1415 drivers/net/virtio_net.c:1519) virtio_net
__napi_poll (net/core/dev.c:6964)
net_rx_action (net/core/dev.c:7033 net/core/dev.c:7118)
__do_softirq (./arch/x86/include/asm/jump_label.h:25 ./include/linux/jump_label.h:200 ./include/trace/events/irq.h:142 kernel/softirq.c:346)
irq_exit_rcu (kernel/softirq.c:221 kernel/softirq.c:422 kernel/softirq.c:434)
common_interrupt (arch/x86/kernel/irq.c:240 (discriminator 14))
</IRQ>

Fixes: fb32856b16ad ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Reported-by: Ido Schimmel <idosch@nvidia.com>
Tested-by: Ido Schimmel <idosch@nvidia.com>
---

Acked-by: Jason Wang <jasowang@redhat.com>

The codes became hard to read, I think we can try to do some cleanups on
top to make it easier to read.

Thanks

Yes, this piece of code needs to be sorted out. Especially the big and mergeable
scenarios should be handled separately. Remove the mergeable code from this
function, and mergeable uses a new function alone.

Right, another thing is that we may consider to relax the checking of 
len < GOOD_COPY_LEN.

Our QE still see low PPS compared with the code before 3226b158e67c 
("net: avoid 32 x truesize under-estimation for tiny skbs").

Thanks

Thanks.

quoted

quoted

   drivers/net/virtio_net.c | 12 +++++++++---
   1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 74d2d49264f3..7fda2ae4c40f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -387,7 +387,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
   	unsigned int copy, hdr_len, hdr_padded_len;
   	struct page *page_to_free = NULL;
   	int tailroom, shinfo_size;
-	char *p, *hdr_p;
+	char *p, *hdr_p, *buf;

   	p = page_address(page) + offset;
   	hdr_p = p;

@@ -403,11 +403,15 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
   	 * space are aligned.
   	 */
   	if (headroom) {
-		/* The actual allocated space size is PAGE_SIZE. */
+		/* Buffers with headroom use PAGE_SIZE as alloc size,
+		 * see add_recvbuf_mergeable() + get_mergeable_buf_len()
+		 */
   		truesize = PAGE_SIZE;
   		tailroom = truesize - len - offset;
+		buf = page_address(page);
   	} else {
   		tailroom = truesize - len;
+		buf = p;
   	}

   	len -= hdr_len;

@@ -416,11 +420,13 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,

   	shinfo_size = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));

+	/* copy small packet so we can reuse these pages */
   	if (!NET_IP_ALIGN && len > GOOD_COPY_LEN && tailroom >= shinfo_size) {
-		skb = build_skb(p, truesize);
+		skb = build_skb(buf, truesize);
   		if (unlikely(!skb))
   			return NULL;

+		skb_reserve(skb, p - buf);
   		skb_put(skb, len);
   		goto ok;
   	}