* [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).