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

[孙蒙恩 <mengensun8801@gmail.com>]

Jason Wang <jasowang@redhat.com> 于2021年12月14日周二 11:13写道：
>
> On Mon, Dec 13, 2021 at 5:14 PM 孙蒙恩 <mengensun8801@gmail.com> wrote:
> >
> > Jason Wang <jasowang@redhat.com> 于2021年12月13日周一 15:49写道：
> > >
> > > On Mon, Dec 13, 2021 at 12:50 PM <mengensun8801@gmail.com> wrote:
> > > >
> > > > From: mengensun <mengensun@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**

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.

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?

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? #^_^

>
>
> >         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@tencent.com>
> > > > Reviewed-by: MengLong Dong <imagedong@tencent.com>
> > > > Reviewed-by: ZhengXiong Jiang <mungerjiang@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
> > > >
> > >
> >
>