linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] virtio-net: make copy len check in xdp_linearize_page
@ 2021-12-13  4:50 mengensun8801
  2021-12-13  7:48 ` Jason Wang
  0 siblings, 1 reply; 5+ messages in thread
From: mengensun8801 @ 2021-12-13  4:50 UTC (permalink / raw)
  To: jasowang
  Cc: davem, kuba, ast, daniel, hawk, john.fastabend, virtualization,
	netdev, linux-kernel, bpf, mengensun, MengLong Dong,
	ZhengXiong Jiang

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.

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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] virtio-net: make copy len check in xdp_linearize_page
  2021-12-13  4:50 [PATCH] virtio-net: make copy len check in xdp_linearize_page mengensun8801
@ 2021-12-13  7:48 ` Jason Wang
  2021-12-14  3:13   ` Jason Wang
  0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2021-12-13  7:48 UTC (permalink / raw)
  To: mengensun8801
  Cc: davem, Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
	Jesper Dangaard Brouer, John Fastabend, virtualization, netdev,
	linux-kernel, bpf, mengensun, MengLong Dong, ZhengXiong Jiang

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().

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
>


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] virtio-net: make copy len check in xdp_linearize_page
  2021-12-13  7:48 ` Jason Wang
@ 2021-12-14  3:13   ` Jason Wang
  2021-12-14  3:48     ` 孙蒙恩
  0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2021-12-14  3:13 UTC (permalink / raw)
  To: mengensun8801
  Cc: davem, Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
	Jesper Dangaard Brouer, John Fastabend, virtualization, netdev,
	linux-kernel, bpf, mengensun, MengLong Dong, ZhengXiong Jiang

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.

>         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
> > >
> >
>


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] virtio-net: make copy len check in xdp_linearize_page
  2021-12-14  3:13   ` Jason Wang
@ 2021-12-14  3:48     ` 孙蒙恩
  2021-12-14  6:14       ` Jason Wang
  0 siblings, 1 reply; 5+ messages in thread
From: 孙蒙恩 @ 2021-12-14  3:48 UTC (permalink / raw)
  To: Jason Wang
  Cc: davem, Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
	Jesper Dangaard Brouer, John Fastabend, virtualization, netdev,
	linux-kernel, bpf, mengensun, MengLong Dong, ZhengXiong Jiang

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
> > > >
> > >
> >
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] virtio-net: make copy len check in xdp_linearize_page
  2021-12-14  3:48     ` 孙蒙恩
@ 2021-12-14  6:14       ` Jason Wang
  0 siblings, 0 replies; 5+ messages in thread
From: Jason Wang @ 2021-12-14  6:14 UTC (permalink / raw)
  To: 孙蒙恩
  Cc: davem, Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
	Jesper Dangaard Brouer, John Fastabend, virtualization, netdev,
	linux-kernel, bpf, mengensun, MengLong Dong, ZhengXiong Jiang

On Tue, Dec 14, 2021 at 11:48 AM 孙蒙恩 <mengensun8801@gmail.com> wrote:
>
> 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**

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@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
> > > > >
> > > >
> > >
> >
>


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-12-14  6:15 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-13  4:50 [PATCH] virtio-net: make copy len check in xdp_linearize_page mengensun8801
2021-12-13  7:48 ` Jason Wang
2021-12-14  3:13   ` Jason Wang
2021-12-14  3:48     ` 孙蒙恩
2021-12-14  6:14       ` Jason Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).