| Message ID | 20230131085004.98687-1-hengqi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 981f14d42a7f1610292a1ef0f7cd00138fff361d |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] virtio-net: fix possible unsigned integer overflow | expand |
Hello: This patch was applied to netdev/net-next.git (master) by Jakub Kicinski <kuba@kernel.org>: On Tue, 31 Jan 2023 16:50:04 +0800 you wrote: > When the single-buffer xdp is loaded and after xdp_linearize_page() > is called, *num_buf becomes 0 and (*num_buf - 1) may overflow into > a large integer in virtnet_build_xdp_buff_mrg(), resulting in > unexpected packet dropping. > > Fixes: ef75cb51f139 ("virtio-net: build xdp_buff with multi buffers") > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > [...] Here is the summary with links: - [net-next,v2] virtio-net: fix possible unsigned integer overflow https://git.kernel.org/netdev/net-next/c/981f14d42a7f You are awesome, thank you!
On Tue, Jan 31, 2023 at 04:50:04PM +0800, Heng Qi wrote: > When the single-buffer xdp is loaded and after xdp_linearize_page() > is called, *num_buf becomes 0 and (*num_buf - 1) may overflow into > a large integer in virtnet_build_xdp_buff_mrg(), resulting in > unexpected packet dropping. > > Fixes: ef75cb51f139 ("virtio-net: build xdp_buff with multi buffers") > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > v1->v2: > - Change the type of num_buf from unsigned int to int. @Michael S . Tsirkin > - Some cleaner codes. @Michael S . Tsirkin > > drivers/net/virtio_net.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index aaa6fe9b214a..8102861785a2 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -716,7 +716,7 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi) > * have enough headroom. > */ > static struct page *xdp_linearize_page(struct receive_queue *rq, > - u16 *num_buf, > + int *num_buf, > struct page *p, > int offset, > int page_off, > @@ -816,7 +816,7 @@ static struct sk_buff *receive_small(struct net_device *dev, > if (unlikely(xdp_headroom < virtnet_get_headroom(vi))) { > int offset = buf - page_address(page) + header_offset; > unsigned int tlen = len + vi->hdr_len; > - u16 num_buf = 1; > + int num_buf = 1; > > xdp_headroom = virtnet_get_headroom(vi); > header_offset = VIRTNET_RX_PAD + xdp_headroom; > @@ -989,7 +989,7 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, > void *buf, > unsigned int len, > unsigned int frame_sz, > - u16 *num_buf, > + int *num_buf, > unsigned int *xdp_frags_truesize, > struct virtnet_rq_stats *stats) > { > @@ -1007,6 +1007,9 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, > xdp_prepare_buff(xdp, buf - VIRTIO_XDP_HEADROOM, > VIRTIO_XDP_HEADROOM + vi->hdr_len, len - vi->hdr_len, true); > > + if (!*num_buf) > + return 0; > + > if (*num_buf > 1) { > /* If we want to build multi-buffer xdp, we need > * to specify that the flags of xdp_buff have the Ouch. Why is this here? Merged so pls remove by a follow up patch, the rest of the code handles 0 fine. I'm not sure this introduces a bug by we don't want spaghetti code. > @@ -1020,10 +1023,10 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, > shinfo->xdp_frags_size = 0; > } > > - if ((*num_buf - 1) > MAX_SKB_FRAGS) > + if (*num_buf > MAX_SKB_FRAGS + 1) > return -EINVAL; > > - while ((--*num_buf) >= 1) { > + while (--*num_buf > 0) { > buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx); > if (unlikely(!buf)) { > pr_debug("%s: rx error: %d buffers out of %d missing\n", > @@ -1076,7 +1079,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, > struct virtnet_rq_stats *stats) > { > struct virtio_net_hdr_mrg_rxbuf *hdr = buf; > - u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers); > + int num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers); > struct page *page = virt_to_head_page(buf); > int offset = buf - page_address(page); > struct sk_buff *head_skb, *curr_skb; > -- > 2.19.1.6.gb485710b
在 2023/2/2 下午4:07, Michael S. Tsirkin 写道: > On Tue, Jan 31, 2023 at 04:50:04PM +0800, Heng Qi wrote: >> When the single-buffer xdp is loaded and after xdp_linearize_page() >> is called, *num_buf becomes 0 and (*num_buf - 1) may overflow into >> a large integer in virtnet_build_xdp_buff_mrg(), resulting in >> unexpected packet dropping. >> >> Fixes: ef75cb51f139 ("virtio-net: build xdp_buff with multi buffers") >> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> >> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> --- >> v1->v2: >> - Change the type of num_buf from unsigned int to int. @Michael S . Tsirkin >> - Some cleaner codes. @Michael S . Tsirkin >> >> drivers/net/virtio_net.c | 15 +++++++++------ >> 1 file changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >> index aaa6fe9b214a..8102861785a2 100644 >> --- a/drivers/net/virtio_net.c >> +++ b/drivers/net/virtio_net.c >> @@ -716,7 +716,7 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi) >> * have enough headroom. >> */ >> static struct page *xdp_linearize_page(struct receive_queue *rq, >> - u16 *num_buf, >> + int *num_buf, >> struct page *p, >> int offset, >> int page_off, >> @@ -816,7 +816,7 @@ static struct sk_buff *receive_small(struct net_device *dev, >> if (unlikely(xdp_headroom < virtnet_get_headroom(vi))) { >> int offset = buf - page_address(page) + header_offset; >> unsigned int tlen = len + vi->hdr_len; >> - u16 num_buf = 1; >> + int num_buf = 1; >> >> xdp_headroom = virtnet_get_headroom(vi); >> header_offset = VIRTNET_RX_PAD + xdp_headroom; >> @@ -989,7 +989,7 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, >> void *buf, >> unsigned int len, >> unsigned int frame_sz, >> - u16 *num_buf, >> + int *num_buf, >> unsigned int *xdp_frags_truesize, >> struct virtnet_rq_stats *stats) >> { >> @@ -1007,6 +1007,9 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, >> xdp_prepare_buff(xdp, buf - VIRTIO_XDP_HEADROOM, >> VIRTIO_XDP_HEADROOM + vi->hdr_len, len - vi->hdr_len, true); >> >> + if (!*num_buf) >> + return 0; >> + >> if (*num_buf > 1) { >> /* If we want to build multi-buffer xdp, we need >> * to specify that the flags of xdp_buff have the > Ouch. Why is this here? Merged so pls remove by a follow up patch, the > rest of the code handles 0 fine. I'm not sure this introduces a bug by > we don't want spaghetti code. Yes it would work without this, but I was keeping this because I wanted it to handle 0 early and exit early. Do you want to remove this? Thanks. > >> @@ -1020,10 +1023,10 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, >> shinfo->xdp_frags_size = 0; >> } >> >> - if ((*num_buf - 1) > MAX_SKB_FRAGS) >> + if (*num_buf > MAX_SKB_FRAGS + 1) >> return -EINVAL; >> >> - while ((--*num_buf) >= 1) { >> + while (--*num_buf > 0) { >> buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx); >> if (unlikely(!buf)) { >> pr_debug("%s: rx error: %d buffers out of %d missing\n", >> @@ -1076,7 +1079,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, >> struct virtnet_rq_stats *stats) >> { >> struct virtio_net_hdr_mrg_rxbuf *hdr = buf; >> - u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers); >> + int num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers); >> struct page *page = virt_to_head_page(buf); >> int offset = buf - page_address(page); >> struct sk_buff *head_skb, *curr_skb; >> -- >> 2.19.1.6.gb485710b
On Thu, Feb 02, 2023 at 04:14:51PM +0800, Heng Qi wrote: > > > 在 2023/2/2 下午4:07, Michael S. Tsirkin 写道: > > On Tue, Jan 31, 2023 at 04:50:04PM +0800, Heng Qi wrote: > > > When the single-buffer xdp is loaded and after xdp_linearize_page() > > > is called, *num_buf becomes 0 and (*num_buf - 1) may overflow into > > > a large integer in virtnet_build_xdp_buff_mrg(), resulting in > > > unexpected packet dropping. > > > > > > Fixes: ef75cb51f139 ("virtio-net: build xdp_buff with multi buffers") > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > --- > > > v1->v2: > > > - Change the type of num_buf from unsigned int to int. @Michael S . Tsirkin > > > - Some cleaner codes. @Michael S . Tsirkin > > > > > > drivers/net/virtio_net.c | 15 +++++++++------ > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index aaa6fe9b214a..8102861785a2 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -716,7 +716,7 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi) > > > * have enough headroom. > > > */ > > > static struct page *xdp_linearize_page(struct receive_queue *rq, > > > - u16 *num_buf, > > > + int *num_buf, > > > struct page *p, > > > int offset, > > > int page_off, > > > @@ -816,7 +816,7 @@ static struct sk_buff *receive_small(struct net_device *dev, > > > if (unlikely(xdp_headroom < virtnet_get_headroom(vi))) { > > > int offset = buf - page_address(page) + header_offset; > > > unsigned int tlen = len + vi->hdr_len; > > > - u16 num_buf = 1; > > > + int num_buf = 1; > > > xdp_headroom = virtnet_get_headroom(vi); > > > header_offset = VIRTNET_RX_PAD + xdp_headroom; > > > @@ -989,7 +989,7 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, > > > void *buf, > > > unsigned int len, > > > unsigned int frame_sz, > > > - u16 *num_buf, > > > + int *num_buf, > > > unsigned int *xdp_frags_truesize, > > > struct virtnet_rq_stats *stats) > > > { > > > @@ -1007,6 +1007,9 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, > > > xdp_prepare_buff(xdp, buf - VIRTIO_XDP_HEADROOM, > > > VIRTIO_XDP_HEADROOM + vi->hdr_len, len - vi->hdr_len, true); > > > + if (!*num_buf) > > > + return 0; > > > + > > > if (*num_buf > 1) { > > > /* If we want to build multi-buffer xdp, we need > > > * to specify that the flags of xdp_buff have the > > Ouch. Why is this here? Merged so pls remove by a follow up patch, the > > rest of the code handles 0 fine. I'm not sure this introduces a bug by > > we don't want spaghetti code. > > Yes it would work without this, but I was keeping this because I wanted it > to handle 0 early and exit early. > > Do you want to remove this? > > Thanks. why do you want to exit early? > > > > > @@ -1020,10 +1023,10 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, > > > shinfo->xdp_frags_size = 0; > > > } > > > - if ((*num_buf - 1) > MAX_SKB_FRAGS) > > > + if (*num_buf > MAX_SKB_FRAGS + 1) > > > return -EINVAL; > > > - while ((--*num_buf) >= 1) { > > > + while (--*num_buf > 0) { > > > buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx); > > > if (unlikely(!buf)) { > > > pr_debug("%s: rx error: %d buffers out of %d missing\n", > > > @@ -1076,7 +1079,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, > > > struct virtnet_rq_stats *stats) > > > { > > > struct virtio_net_hdr_mrg_rxbuf *hdr = buf; > > > - u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers); > > > + int num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers); > > > struct page *page = virt_to_head_page(buf); > > > int offset = buf - page_address(page); > > > struct sk_buff *head_skb, *curr_skb; > > > -- > > > 2.19.1.6.gb485710b
在 2023/2/2 下午4:16, Michael S. Tsirkin 写道: > On Thu, Feb 02, 2023 at 04:14:51PM +0800, Heng Qi wrote: >> >> 在 2023/2/2 下午4:07, Michael S. Tsirkin 写道: >>> On Tue, Jan 31, 2023 at 04:50:04PM +0800, Heng Qi wrote: >>>> When the single-buffer xdp is loaded and after xdp_linearize_page() >>>> is called, *num_buf becomes 0 and (*num_buf - 1) may overflow into >>>> a large integer in virtnet_build_xdp_buff_mrg(), resulting in >>>> unexpected packet dropping. >>>> >>>> Fixes: ef75cb51f139 ("virtio-net: build xdp_buff with multi buffers") >>>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> >>>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> >>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>> --- >>>> v1->v2: >>>> - Change the type of num_buf from unsigned int to int. @Michael S . Tsirkin >>>> - Some cleaner codes. @Michael S . Tsirkin >>>> >>>> drivers/net/virtio_net.c | 15 +++++++++------ >>>> 1 file changed, 9 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >>>> index aaa6fe9b214a..8102861785a2 100644 >>>> --- a/drivers/net/virtio_net.c >>>> +++ b/drivers/net/virtio_net.c >>>> @@ -716,7 +716,7 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi) >>>> * have enough headroom. >>>> */ >>>> static struct page *xdp_linearize_page(struct receive_queue *rq, >>>> - u16 *num_buf, >>>> + int *num_buf, >>>> struct page *p, >>>> int offset, >>>> int page_off, >>>> @@ -816,7 +816,7 @@ static struct sk_buff *receive_small(struct net_device *dev, >>>> if (unlikely(xdp_headroom < virtnet_get_headroom(vi))) { >>>> int offset = buf - page_address(page) + header_offset; >>>> unsigned int tlen = len + vi->hdr_len; >>>> - u16 num_buf = 1; >>>> + int num_buf = 1; >>>> xdp_headroom = virtnet_get_headroom(vi); >>>> header_offset = VIRTNET_RX_PAD + xdp_headroom; >>>> @@ -989,7 +989,7 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, >>>> void *buf, >>>> unsigned int len, >>>> unsigned int frame_sz, >>>> - u16 *num_buf, >>>> + int *num_buf, >>>> unsigned int *xdp_frags_truesize, >>>> struct virtnet_rq_stats *stats) >>>> { >>>> @@ -1007,6 +1007,9 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, >>>> xdp_prepare_buff(xdp, buf - VIRTIO_XDP_HEADROOM, >>>> VIRTIO_XDP_HEADROOM + vi->hdr_len, len - vi->hdr_len, true); >>>> + if (!*num_buf) >>>> + return 0; >>>> + >>>> if (*num_buf > 1) { >>>> /* If we want to build multi-buffer xdp, we need >>>> * to specify that the flags of xdp_buff have the >>> Ouch. Why is this here? Merged so pls remove by a follow up patch, the >>> rest of the code handles 0 fine. I'm not sure this introduces a bug by >>> we don't want spaghetti code. >> Yes it would work without this, but I was keeping this because I wanted it >> to handle 0 early and exit early. >> >> Do you want to remove this? >> >> Thanks. > why do you want to exit early? If num_buf is 0, we don't need to judge the subsequent process, because the latter process is used to build multi-buffer xdp, but this fix solves the possible problems of single-buffer xdp. Thanks. > >>>> @@ -1020,10 +1023,10 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, >>>> shinfo->xdp_frags_size = 0; >>>> } >>>> - if ((*num_buf - 1) > MAX_SKB_FRAGS) >>>> + if (*num_buf > MAX_SKB_FRAGS + 1) >>>> return -EINVAL; >>>> - while ((--*num_buf) >= 1) { >>>> + while (--*num_buf > 0) { >>>> buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx); >>>> if (unlikely(!buf)) { >>>> pr_debug("%s: rx error: %d buffers out of %d missing\n", >>>> @@ -1076,7 +1079,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, >>>> struct virtnet_rq_stats *stats) >>>> { >>>> struct virtio_net_hdr_mrg_rxbuf *hdr = buf; >>>> - u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers); >>>> + int num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers); >>>> struct page *page = virt_to_head_page(buf); >>>> int offset = buf - page_address(page); >>>> struct sk_buff *head_skb, *curr_skb; >>>> -- >>>> 2.19.1.6.gb485710b
On Thu, Feb 02, 2023 at 05:07:04PM +0800, Heng Qi wrote: > > > 在 2023/2/2 下午4:16, Michael S. Tsirkin 写道: > > On Thu, Feb 02, 2023 at 04:14:51PM +0800, Heng Qi wrote: > > > > > > 在 2023/2/2 下午4:07, Michael S. Tsirkin 写道: > > > > On Tue, Jan 31, 2023 at 04:50:04PM +0800, Heng Qi wrote: > > > > > When the single-buffer xdp is loaded and after xdp_linearize_page() > > > > > is called, *num_buf becomes 0 and (*num_buf - 1) may overflow into > > > > > a large integer in virtnet_build_xdp_buff_mrg(), resulting in > > > > > unexpected packet dropping. > > > > > > > > > > Fixes: ef75cb51f139 ("virtio-net: build xdp_buff with multi buffers") > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > --- > > > > > v1->v2: > > > > > - Change the type of num_buf from unsigned int to int. @Michael S . Tsirkin > > > > > - Some cleaner codes. @Michael S . Tsirkin > > > > > > > > > > drivers/net/virtio_net.c | 15 +++++++++------ > > > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > > index aaa6fe9b214a..8102861785a2 100644 > > > > > --- a/drivers/net/virtio_net.c > > > > > +++ b/drivers/net/virtio_net.c > > > > > @@ -716,7 +716,7 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi) > > > > > * have enough headroom. > > > > > */ > > > > > static struct page *xdp_linearize_page(struct receive_queue *rq, > > > > > - u16 *num_buf, > > > > > + int *num_buf, > > > > > struct page *p, > > > > > int offset, > > > > > int page_off, > > > > > @@ -816,7 +816,7 @@ static struct sk_buff *receive_small(struct net_device *dev, > > > > > if (unlikely(xdp_headroom < virtnet_get_headroom(vi))) { > > > > > int offset = buf - page_address(page) + header_offset; > > > > > unsigned int tlen = len + vi->hdr_len; > > > > > - u16 num_buf = 1; > > > > > + int num_buf = 1; > > > > > xdp_headroom = virtnet_get_headroom(vi); > > > > > header_offset = VIRTNET_RX_PAD + xdp_headroom; > > > > > @@ -989,7 +989,7 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, > > > > > void *buf, > > > > > unsigned int len, > > > > > unsigned int frame_sz, > > > > > - u16 *num_buf, > > > > > + int *num_buf, > > > > > unsigned int *xdp_frags_truesize, > > > > > struct virtnet_rq_stats *stats) > > > > > { > > > > > @@ -1007,6 +1007,9 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, > > > > > xdp_prepare_buff(xdp, buf - VIRTIO_XDP_HEADROOM, > > > > > VIRTIO_XDP_HEADROOM + vi->hdr_len, len - vi->hdr_len, true); > > > > > + if (!*num_buf) > > > > > + return 0; > > > > > + > > > > > if (*num_buf > 1) { > > > > > /* If we want to build multi-buffer xdp, we need > > > > > * to specify that the flags of xdp_buff have the > > > > Ouch. Why is this here? Merged so pls remove by a follow up patch, the > > > > rest of the code handles 0 fine. I'm not sure this introduces a bug by > > > > we don't want spaghetti code. > > > Yes it would work without this, but I was keeping this because I wanted it > > > to handle 0 early and exit early. > > > > > > Do you want to remove this? > > > > > > Thanks. > > why do you want to exit early? > > If num_buf is 0, we don't need to judge the subsequent process, because the > latter process > is used to build multi-buffer xdp, but this fix solves the possible problems > of single-buffer xdp. > > Thanks. An optimization then? As any optimization I'd like to see some numbers. Should have been documented as such not as part of a bugfix. > > > > > > > @@ -1020,10 +1023,10 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, > > > > > shinfo->xdp_frags_size = 0; > > > > > } > > > > > - if ((*num_buf - 1) > MAX_SKB_FRAGS) > > > > > + if (*num_buf > MAX_SKB_FRAGS + 1) > > > > > return -EINVAL; > > > > > - while ((--*num_buf) >= 1) { > > > > > + while (--*num_buf > 0) { > > > > > buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx); > > > > > if (unlikely(!buf)) { > > > > > pr_debug("%s: rx error: %d buffers out of %d missing\n", > > > > > @@ -1076,7 +1079,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, > > > > > struct virtnet_rq_stats *stats) > > > > > { > > > > > struct virtio_net_hdr_mrg_rxbuf *hdr = buf; > > > > > - u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers); > > > > > + int num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers); > > > > > struct page *page = virt_to_head_page(buf); > > > > > int offset = buf - page_address(page); > > > > > struct sk_buff *head_skb, *curr_skb; > > > > > -- > > > > > 2.19.1.6.gb485710b
在 2023/2/2 下午6:31, Michael S. Tsirkin 写道: > On Thu, Feb 02, 2023 at 05:07:04PM +0800, Heng Qi wrote: >> >> 在 2023/2/2 下午4:16, Michael S. Tsirkin 写道: >>> On Thu, Feb 02, 2023 at 04:14:51PM +0800, Heng Qi wrote: >>>> 在 2023/2/2 下午4:07, Michael S. Tsirkin 写道: >>>>> On Tue, Jan 31, 2023 at 04:50:04PM +0800, Heng Qi wrote: >>>>>> When the single-buffer xdp is loaded and after xdp_linearize_page() >>>>>> is called, *num_buf becomes 0 and (*num_buf - 1) may overflow into >>>>>> a large integer in virtnet_build_xdp_buff_mrg(), resulting in >>>>>> unexpected packet dropping. >>>>>> >>>>>> Fixes: ef75cb51f139 ("virtio-net: build xdp_buff with multi buffers") >>>>>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> >>>>>> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> >>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>>>> --- >>>>>> v1->v2: >>>>>> - Change the type of num_buf from unsigned int to int. @Michael S . Tsirkin >>>>>> - Some cleaner codes. @Michael S . Tsirkin >>>>>> >>>>>> drivers/net/virtio_net.c | 15 +++++++++------ >>>>>> 1 file changed, 9 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >>>>>> index aaa6fe9b214a..8102861785a2 100644 >>>>>> --- a/drivers/net/virtio_net.c >>>>>> +++ b/drivers/net/virtio_net.c >>>>>> @@ -716,7 +716,7 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi) >>>>>> * have enough headroom. >>>>>> */ >>>>>> static struct page *xdp_linearize_page(struct receive_queue *rq, >>>>>> - u16 *num_buf, >>>>>> + int *num_buf, >>>>>> struct page *p, >>>>>> int offset, >>>>>> int page_off, >>>>>> @@ -816,7 +816,7 @@ static struct sk_buff *receive_small(struct net_device *dev, >>>>>> if (unlikely(xdp_headroom < virtnet_get_headroom(vi))) { >>>>>> int offset = buf - page_address(page) + header_offset; >>>>>> unsigned int tlen = len + vi->hdr_len; >>>>>> - u16 num_buf = 1; >>>>>> + int num_buf = 1; >>>>>> xdp_headroom = virtnet_get_headroom(vi); >>>>>> header_offset = VIRTNET_RX_PAD + xdp_headroom; >>>>>> @@ -989,7 +989,7 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, >>>>>> void *buf, >>>>>> unsigned int len, >>>>>> unsigned int frame_sz, >>>>>> - u16 *num_buf, >>>>>> + int *num_buf, >>>>>> unsigned int *xdp_frags_truesize, >>>>>> struct virtnet_rq_stats *stats) >>>>>> { >>>>>> @@ -1007,6 +1007,9 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, >>>>>> xdp_prepare_buff(xdp, buf - VIRTIO_XDP_HEADROOM, >>>>>> VIRTIO_XDP_HEADROOM + vi->hdr_len, len - vi->hdr_len, true); >>>>>> + if (!*num_buf) >>>>>> + return 0; >>>>>> + >>>>>> if (*num_buf > 1) { >>>>>> /* If we want to build multi-buffer xdp, we need >>>>>> * to specify that the flags of xdp_buff have the >>>>> Ouch. Why is this here? Merged so pls remove by a follow up patch, the >>>>> rest of the code handles 0 fine. I'm not sure this introduces a bug by >>>>> we don't want spaghetti code. >>>> Yes it would work without this, but I was keeping this because I wanted it >>>> to handle 0 early and exit early. >>>> >>>> Do you want to remove this? >>>> >>>> Thanks. >>> why do you want to exit early? >> If num_buf is 0, we don't need to judge the subsequent process, because the >> latter process >> is used to build multi-buffer xdp, but this fix solves the possible problems >> of single-buffer xdp. >> >> Thanks. > An optimization then? As any optimization I'd like to see some numbers. > > Should have been documented as such not as part of a bugfix. Sure, I'm going to remove this with a follow-up patch.
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index aaa6fe9b214a..8102861785a2 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -716,7 +716,7 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi) * have enough headroom. */ static struct page *xdp_linearize_page(struct receive_queue *rq, - u16 *num_buf, + int *num_buf, struct page *p, int offset, int page_off, @@ -816,7 +816,7 @@ static struct sk_buff *receive_small(struct net_device *dev, if (unlikely(xdp_headroom < virtnet_get_headroom(vi))) { int offset = buf - page_address(page) + header_offset; unsigned int tlen = len + vi->hdr_len; - u16 num_buf = 1; + int num_buf = 1; xdp_headroom = virtnet_get_headroom(vi); header_offset = VIRTNET_RX_PAD + xdp_headroom; @@ -989,7 +989,7 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, void *buf, unsigned int len, unsigned int frame_sz, - u16 *num_buf, + int *num_buf, unsigned int *xdp_frags_truesize, struct virtnet_rq_stats *stats) { @@ -1007,6 +1007,9 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, xdp_prepare_buff(xdp, buf - VIRTIO_XDP_HEADROOM, VIRTIO_XDP_HEADROOM + vi->hdr_len, len - vi->hdr_len, true); + if (!*num_buf) + return 0; + if (*num_buf > 1) { /* If we want to build multi-buffer xdp, we need * to specify that the flags of xdp_buff have the @@ -1020,10 +1023,10 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev, shinfo->xdp_frags_size = 0; } - if ((*num_buf - 1) > MAX_SKB_FRAGS) + if (*num_buf > MAX_SKB_FRAGS + 1) return -EINVAL; - while ((--*num_buf) >= 1) { + while (--*num_buf > 0) { buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx); if (unlikely(!buf)) { pr_debug("%s: rx error: %d buffers out of %d missing\n", @@ -1076,7 +1079,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, struct virtnet_rq_stats *stats) { struct virtio_net_hdr_mrg_rxbuf *hdr = buf; - u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers); + int num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers); struct page *page = virt_to_head_page(buf); int offset = buf - page_address(page); struct sk_buff *head_skb, *curr_skb;
When the single-buffer xdp is loaded and after xdp_linearize_page() is called, *num_buf becomes 0 and (*num_buf - 1) may overflow into a large integer in virtnet_build_xdp_buff_mrg(), resulting in unexpected packet dropping. Fixes: ef75cb51f139 ("virtio-net: build xdp_buff with multi buffers") Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- v1->v2: - Change the type of num_buf from unsigned int to int. @Michael S . Tsirkin - Some cleaner codes. @Michael S . Tsirkin drivers/net/virtio_net.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)