Re: [PATCH] virtio-net: make copy len check in xdp_linearize_page

On Tue, Dec 14, 2021 at 11:48 AM 孙蒙恩 [off-list ref] wrote:

Jason Wang [off-list ref] 于2021年12月14日周二 11:13写道：

quoted

On Mon, Dec 13, 2021 at 5:14 PM 孙蒙恩 [off-list ref] wrote:

quoted

Jason Wang [off-list ref] 于2021年12月13日周一 15:49写道：

quoted

On Mon, Dec 13, 2021 at 12:50 PM [off-list ref] wrote:

quoted

From: mengensun <redacted>

xdp_linearize_page asume ring elem size is smaller then page size
when copy the first ring elem, but, there may be a elem size bigger
then page size.

add_recvbuf_mergeable may add a hole to ring elem, the hole size is
not sure, according EWMA.

The logic is to try to avoid dropping packets in this case, so I
wonder if it's better to "fix" the add_recvbuf_mergeable().

Adding lists back.

quoted

turn to XDP generic is so difficulty for me, here can "fix" the
add_recvbuf_mergeable link follow:

--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 36a4b7c195d5..06ce8bb10b47 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -1315,6 +1315,7 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
                alloc_frag->offset += hole;
        }
+       len = min(len, PAGE_SIZE - room);
        sg_init_one(rq->sg, buf, len);
        ctx = mergeable_len_to_ctx(len, headroom);

Then the truesize here is wrong.

many thanks!! i have  known i'm wrong immediately after click the
"send" botton , now, this problem seem not only about the *hole* but
the  EWMA, EWMA will give buff len bewteen min_buf_len and PAGE_SIZE,
while swith from no-attach-xdp to attach xdp, there may be some buff
already in ring and filled before xdp attach. xdp_linearize_page
assume buf size is PAGE_SIZE - VIRTIO_XDP_HEADROOM, and coped "len"
from the buff, while the buff may be **PAGE_SIZE**

So the issue I see is that though add_recvbuf_mergeable() tries to
make the buffer size is PAGE_SIZE, XDP might requires more on the
header which makes a single page is not sufficient.

because we have no idear when the user attach xdp prog, so, i have no
idear except make all the buff have a "header hole" len of
VIRTIO_XDP_HEADROOM(128), but it seem so expensive for no-xdp-attach
virtio eth to aways leave 128 byte not used at all.

That's an requirement for XDP header adjustment so far.

this problem is found by review code, in really test, it seemed not so
may big frame. so here we can just "fix" the  xdp_linearize_page, make
it trying best to not drop frames for now?

I think you can reproduce the issue by keeping sending large frames to
guest and try to attach XDP in the middle.

btw,  it seem no way to fix this thoroughly, except we drained all the
buff in the ring, and flush it all to "xdp buff" when attaching xdp
prog.

is that some mistake i have makeed again? #^_^

I see two ways to solve this:

1) reverse more room (but not headerroom) to make sure PAGE_SIZE can
work in the case of linearizing
2) switch to use XDP genreic

2) looks much more easier, you may refer tun_xdp_one() to see how it
works, it's as simple as call do_xdp_generic()

Thanks

quoted

quoted

        err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);

it seems a rule that, length of elem giving to vring is away smaller
or equall then PAGE_SIZE

It aims to be consistent to what EWMA tries to do:

        len = hdr_len + clamp_t(unsigned int, ewma_pkt_len_read(avg_pkt_len),
                        rq->min_buf_len, PAGE_SIZE - hdr_len);

Thanks

quoted

quoted

Or another idea is to switch to use XDP generic here where we can use
skb_linearize() which should be more robust and we can drop the
xdp_linearize_page() logic completely.

Thanks

quoted

so, fix it by check copy len,if checked failed, just dropped the
whole frame, not make the memory dirty after the page.

Signed-off-by: mengensun <redacted>
Reviewed-by: MengLong Dong <redacted>
Reviewed-by: ZhengXiong Jiang <redacted>
---
 drivers/net/virtio_net.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 36a4b7c195d5..844bdbd67ff7 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -662,8 +662,12 @@ static struct page *xdp_linearize_page(struct receive_queue *rq,
                                       int page_off,
                                       unsigned int *len)
 {
-       struct page *page = alloc_page(GFP_ATOMIC);
+       struct page *page;

+       if (*len > PAGE_SIZE - page_off)
+               return NULL;
+
+       page = alloc_page(GFP_ATOMIC);
        if (!page)
                return NULL;

--

2.27.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization