Message-ID: <CACGkMEtMcU6f+AD6+_cKuDpLTFgaBJura39j16PnKqGWB2fULA@mail.gmail.com>
Date:   Tue, 14 Dec 2021 14:14:59 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     孙蒙恩 <mengensun8801@...il.com>
Cc:     davem <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        virtualization <virtualization@...ts.linux-foundation.org>,
        netdev <netdev@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>, bpf@...r.kernel.org,
        mengensun <mengensun@...cent.com>,
        MengLong Dong <imagedong@...cent.com>,
        ZhengXiong Jiang <mungerjiang@...cent.com>
Subject: Re: [PATCH] virtio-net: make copy len check in xdp_linearize_page

On Tue, Dec 14, 2021 at 11:48 AM 孙蒙恩 <mengensun8801@...il.com> wrote:
>
> Jason Wang <jasowang@...hat.com> 于2021年12月14日周二 11:13写道：
> >
> > On Mon, Dec 13, 2021 at 5:14 PM 孙蒙恩 <mengensun8801@...il.com> wrote:
> > >
> > > Jason Wang <jasowang@...hat.com> 于2021年12月13日周一 15:49写道：
> > > >
> > > > On Mon, Dec 13, 2021 at 12:50 PM <mengensun8801@...il.com> wrote:
> > > > >
> > > > > From: mengensun <mengensun@...cent.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@...cent.com>
> > > > > Reviewed-by: MengLong Dong <imagedong@...cent.com>
> > > > > Reviewed-by: ZhengXiong Jiang <mungerjiang@...cent.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
> > > > >
> > > >
> > >
> >
>

Powered by blists - more mailing lists