Re: [PATCH net] virtio-net: fix for skb_over_panic inside big mode

[Xuan Zhuo <xuanzhuo@linux.alibaba.com>]

On Fri, 4 Jun 2021 11:00:25 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2021/6/4 上午10:30, Xuan Zhuo 写道:
> > On Fri, 4 Jun 2021 10:28:41 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >> 在 2021/6/4 上午1:09, Xuan Zhuo 写道:
> >>> In virtio-net's large packet mode, there is a hole in the space behind
> >>> buf.
> >>
> >> before the buf actually or behind the vnet header?
> >>
> >>
> >>>       hdr_padded_len - hdr_len
> >>>
> >>> We must take this into account when calculating tailroom.
> >>>
> >>> [   44.544385] skb_put.cold (net/core/skbuff.c:5254 (discriminator 1) net/core/skbuff.c:5252 (discriminator 1))
> >>> [   44.544864] page_to_skb (drivers/net/virtio_net.c:485) [   44.545361] receive_buf (drivers/net/virtio_net.c:849 drivers/net/virtio_net.c:1131)
> >>> [   44.545870] ? netif_receive_skb_list_internal (net/core/dev.c:5714)
> >>> [   44.546628] ? dev_gro_receive (net/core/dev.c:6103)
> >>> [   44.547135] ? napi_complete_done (./include/linux/list.h:35 net/core/dev.c:5867 net/core/dev.c:5862 net/core/dev.c:6565)
> >>> [   44.547672] virtnet_poll (drivers/net/virtio_net.c:1427 drivers/net/virtio_net.c:1525)
> >>> [   44.548251] __napi_poll (net/core/dev.c:6985)
> >>> [   44.548744] net_rx_action (net/core/dev.c:7054 net/core/dev.c:7139)
> >>> [   44.549264] __do_softirq (./arch/x86/include/asm/jump_label.h:19 ./include/linux/jump_label.h:200 ./include/trace/events/irq.h:142 kernel/softirq.c:560)
> >>> [   44.549762] irq_exit_rcu (kernel/softirq.c:433 kernel/softirq.c:637 kernel/softirq.c:649)
> >>> [   44.551384] common_interrupt (arch/x86/kernel/irq.c:240 (discriminator 13))
> >>> [   44.551991] ? asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
> >>> [   44.552654] asm_common_interrupt (./arch/x86/include/asm/idtentry.h:638)
> >>>
> >>> 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: Corentin Noël <corentin.noel@collabora.com>
> >>> Tested-by: Corentin Noël <corentin.noel@collabora.com>
> >>> ---
> >>>    drivers/net/virtio_net.c | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >>> index fa407eb8b457..78a01c71a17c 100644
> >>> --- a/drivers/net/virtio_net.c
> >>> +++ b/drivers/net/virtio_net.c
> >>> @@ -406,7 +406,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >>>    	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
> >>>    	 */
> >>>    	truesize = headroom ? PAGE_SIZE : truesize;
> >>> -	tailroom = truesize - len - headroom;
> >>> +	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
> >>
> >> The patch looks correct and I saw it has been merged.
> >>
> >> But I prefer to do that in receive_big() instead of here.
> >>
> >> Thanks
> > How?
> >
> > change truesize or headroom?
> >
> > I didn't find a good way. Do you have a good way?
>
>
> Something like the following? The API is designed to let the caller to
> pass a correct headroom instead of figure it out by itself.
>
>          struct sk_buff *skb =
>                  page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0,
> hdr_padded_len - hdr_len);
>
> Thanks


This line may be affected.

	buf = p - headroom;

In my opinion, this changes the semantics of the original headroom. The meaning
of headroom in big mode and merge mode has become different. The more confusing
problem is that the parameters of page_to_skb() are getting more and more
chaotic.  So I wrote the previous patch. Of course, I understand your concern.
This patch may bring Here are more questions, although I did a lot of tests.

	"virtio-net: Refactor the code related to page_to_skb"

But I hope that our code development direction is as close to what this patch
realizes. I hope that the meaning of the parameters can be more clear.

Do you think this is ok?

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 78a01c71a17c..6d62bb45a188 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -380,34 +380,20 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 				   struct page *page, unsigned int offset,
 				   unsigned int len, unsigned int truesize,
 				   bool hdr_valid, unsigned int metasize,
-				   unsigned int headroom)
+				   int tailroom, char *buf,
+				   unsigned int hdr_padded_len)
 {
 	struct sk_buff *skb;
 	struct virtio_net_hdr_mrg_rxbuf *hdr;
-	unsigned int copy, hdr_len, hdr_padded_len;
+	unsigned int copy, hdr_len;
 	struct page *page_to_free = NULL;
-	int tailroom, shinfo_size;
-	char *p, *hdr_p, *buf;
+	int shinfo_size;
+	char *p, *hdr_p;

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

 	hdr_len = vi->hdr_len;
-	if (vi->mergeable_rx_bufs)
-		hdr_padded_len = sizeof(*hdr);
-	else
-		hdr_padded_len = sizeof(struct padded_vnet_hdr);
-
-	/* If headroom is not 0, there is an offset between the beginning of the
-	 * data and the allocated space, otherwise the data and the allocated
-	 * space are aligned.
-	 *
-	 * Buffers with headroom use PAGE_SIZE as alloc size, see
-	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
-	 */
-	truesize = headroom ? PAGE_SIZE : truesize;
-	tailroom = truesize - len - headroom - (hdr_padded_len - hdr_len);
-	buf = p - headroom;

 	len -= hdr_len;
 	offset += hdr_padded_len;
@@ -492,6 +478,51 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 	return skb;
 }

+static struct sk_buff *merge_page_to_skb(struct virtnet_info *vi,
+					 struct receive_queue *rq,
+					 struct page *page, unsigned int offset,
+					 unsigned int len, unsigned int truesize,
+					 bool hdr_valid, unsigned int metasize,
+					 unsigned int headroom)
+{
+	int tailroom;
+	char *buf;
+
+	/* If headroom is not 0, there is an offset between the beginning of the
+	 * data and the allocated space, otherwise the data and the allocated
+	 * space are aligned.
+	 *
+	 * Buffers with headroom use PAGE_SIZE as alloc size, see
+	 * add_recvbuf_mergeable() + get_mergeable_buf_len()
+	 */
+	truesize = headroom ? PAGE_SIZE : truesize;
+	tailroom = truesize - len - headroom;
+	buf = page_address(page) + offset - headroom;
+
+	page_to_skb(vi, rq, page, offset, len, truesize, hdr_valid, metasize,
+		    tailroom, buf, sizeof(struct virtio_net_hdr_mrg_rxbuf))
+
+}
+
+static struct sk_buff *big_page_to_skb(struct virtnet_info *vi,
+				       struct receive_queue *rq,
+				       struct page *page, unsigned int offset,
+				       unsigned int len, unsigned int truesize,
+				       bool hdr_valid, unsigned int metasize,
+				       unsigned int headroom)
+{
+	char *p = page_address(page);
+	int hold;
+	int tailroom;
+
+	hold = sizeof(struct padded_vnet_hdr) - vi->hdr_len;
+
+	tailroom = truesize - len - headroom - hold;
+
+	page_to_skb(vi, rq, page, offset, len, truesize, hdr_valid, metasize,
+		    tailroom, p, sizeof(struct padded_vnet_hdr));
+}
+
 static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
 				   struct send_queue *sq,
 				   struct xdp_frame *xdpf)


>
>
> >
> > Thanks.
> >
> >>
> >>
> >>>    	buf = p - headroom;
> >>>
> >>>    	len -= hdr_len;
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization