Jason Wang
2021-Dec-14 06:14 UTC
[PATCH] virtio-net: make copy len check in xdp_linearize_page
On Tue, Dec 14, 2021 at 11:48 AM ??? <mengensun8801 at gmail.com> wrote:> > Jason Wang <jasowang at redhat.com> ?2021?12?14??? 11:13??? > > > > On Mon, Dec 13, 2021 at 5:14 PM ??? <mengensun8801 at gmail.com> wrote: > > > > > > Jason Wang <jasowang at redhat.com> ?2021?12?13??? 15:49??? > > > > > > > > On Mon, Dec 13, 2021 at 12:50 PM <mengensun8801 at gmail.com> wrote: > > > > > > > > > > From: mengensun <mengensun at tencent.com> > > > > > > > > > > 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. > > > > > 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> > > > > > > > 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 > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > 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 <mengensun at tencent.com> > > > > > Reviewed-by: MengLong Dong <imagedong at tencent.com> > > > > > Reviewed-by: ZhengXiong Jiang <mungerjiang at tencent.com> > > > > > --- > > > > > 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 > > > > > > > > > > > > > > >