Check vring size and fail probe if a transmit/receive vring size is
smaller than MAX_SKB_FRAGS + 2.
At the moment, any vring size is accepted. This is problematic because
it may result in attempting to transmit a packet with more fragments
than there are descriptors in the ring.
Furthermore, it leads to an immediate bug:
The condition: (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) in
virtnet_poll_cleantx and virtnet_poll_tx always evaluates to false,
so netif_tx_wake_queue is not called, leading to TX timeouts.
Signed-off-by: Alvaro Karsz <alvaro.karsz at solid-run.com>
---
drivers/net/virtio_net.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 2396c28c012..59676252c5c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3745,6 +3745,26 @@ static int init_vqs(struct virtnet_info *vi)
return ret;
}
+static int virtnet_validate_vqs(struct virtnet_info *vi)
+{
+ u32 i, min_size = roundup_pow_of_two(MAX_SKB_FRAGS + 2);
+
+ /* Transmit/Receive vring size must be at least MAX_SKB_FRAGS + 2
+ * (fragments + linear part + virtio header)
+ */
+ for (i = 0; i < vi->max_queue_pairs; i++) {
+ if (virtqueue_get_vring_size(vi->sq[i].vq) < min_size ||
+ virtqueue_get_vring_size(vi->rq[i].vq) < min_size) {
+ dev_warn(&vi->vdev->dev,
+ "Transmit/Receive virtqueue vring size must be at least %u\n",
+ min_size);
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
#ifdef CONFIG_SYSFS
static ssize_t mergeable_rx_buffer_size_show(struct netdev_rx_queue *queue,
char *buf)
@@ -4056,6 +4076,10 @@ static int virtnet_probe(struct virtio_device *vdev)
if (err)
goto free;
+ err = virtnet_validate_vqs(vi);
+ if (err)
+ goto free_vqs;
+
#ifdef CONFIG_SYSFS
if (vi->mergeable_rx_bufs)
dev->sysfs_rx_queue_group = &virtio_net_mrg_rx_group;
--
2.34.1
After further consideration, other virtio drivers need a minimum limit to the vring size too. Maybe this can be more general, for example a new virtio_driver callback that is called (if implemented) during virtio_dev_probe, before drv->probe. What do you think? Thanks, Alvaro
On Sun, Apr 16, 2023 at 10:46:07AM +0300, Alvaro Karsz wrote:> Check vring size and fail probe if a transmit/receive vring size is > smaller than MAX_SKB_FRAGS + 2. > > At the moment, any vring size is accepted. This is problematic because > it may result in attempting to transmit a packet with more fragments > than there are descriptors in the ring. > > Furthermore, it leads to an immediate bug: > > The condition: (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) in > virtnet_poll_cleantx and virtnet_poll_tx always evaluates to false, > so netif_tx_wake_queue is not called, leading to TX timeouts. > > Signed-off-by: Alvaro Karsz <alvaro.karsz at solid-run.com> > --- > drivers/net/virtio_net.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 2396c28c012..59676252c5c 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -3745,6 +3745,26 @@ static int init_vqs(struct virtnet_info *vi) > return ret; > } > > +static int virtnet_validate_vqs(struct virtnet_info *vi) > +{ > + u32 i, min_size = roundup_pow_of_two(MAX_SKB_FRAGS + 2);why power of two?> + > + /* Transmit/Receive vring size must be at least MAX_SKB_FRAGS + 2 > + * (fragments + linear part + virtio header) > + */ > + for (i = 0; i < vi->max_queue_pairs; i++) { > + if (virtqueue_get_vring_size(vi->sq[i].vq) < min_size || > + virtqueue_get_vring_size(vi->rq[i].vq) < min_size) { > + dev_warn(&vi->vdev->dev, > + "Transmit/Receive virtqueue vring size must be at least %u\n", > + min_size); > + return -EINVAL; > + } > + } > + > + return 0; > +} > + > #ifdef CONFIG_SYSFS > static ssize_t mergeable_rx_buffer_size_show(struct netdev_rx_queue *queue, > char *buf) > @@ -4056,6 +4076,10 @@ static int virtnet_probe(struct virtio_device *vdev) > if (err) > goto free; > > + err = virtnet_validate_vqs(vi); > + if (err) > + goto free_vqs; > + > #ifdef CONFIG_SYSFS > if (vi->mergeable_rx_bufs) > dev->sysfs_rx_queue_group = &virtio_net_mrg_rx_group; > -- > 2.34.1
On Sun, 16 Apr 2023 10:46:07 +0300, Alvaro Karsz <alvaro.karsz at solid-run.com> wrote:> Check vring size and fail probe if a transmit/receive vring size is > smaller than MAX_SKB_FRAGS + 2. > > At the moment, any vring size is accepted. This is problematic because > it may result in attempting to transmit a packet with more fragments > than there are descriptors in the ring.So, why we check the rx ring? Thanks.> > Furthermore, it leads to an immediate bug: > > The condition: (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) in > virtnet_poll_cleantx and virtnet_poll_tx always evaluates to false, > so netif_tx_wake_queue is not called, leading to TX timeouts. > > Signed-off-by: Alvaro Karsz <alvaro.karsz at solid-run.com> > --- > drivers/net/virtio_net.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 2396c28c012..59676252c5c 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -3745,6 +3745,26 @@ static int init_vqs(struct virtnet_info *vi) > return ret; > } > > +static int virtnet_validate_vqs(struct virtnet_info *vi) > +{ > + u32 i, min_size = roundup_pow_of_two(MAX_SKB_FRAGS + 2); > + > + /* Transmit/Receive vring size must be at least MAX_SKB_FRAGS + 2 > + * (fragments + linear part + virtio header) > + */ > + for (i = 0; i < vi->max_queue_pairs; i++) { > + if (virtqueue_get_vring_size(vi->sq[i].vq) < min_size || > + virtqueue_get_vring_size(vi->rq[i].vq) < min_size) { > + dev_warn(&vi->vdev->dev, > + "Transmit/Receive virtqueue vring size must be at least %u\n", > + min_size); > + return -EINVAL; > + } > + } > + > + return 0; > +} > + > #ifdef CONFIG_SYSFS > static ssize_t mergeable_rx_buffer_size_show(struct netdev_rx_queue *queue, > char *buf) > @@ -4056,6 +4076,10 @@ static int virtnet_probe(struct virtio_device *vdev) > if (err) > goto free; > > + err = virtnet_validate_vqs(vi); > + if (err) > + goto free_vqs; > + > #ifdef CONFIG_SYSFS > if (vi->mergeable_rx_bufs) > dev->sysfs_rx_queue_group = &virtio_net_mrg_rx_group; > -- > 2.34.1 >
On Sun, 16 Apr 2023 10:46:07 +0300, Alvaro Karsz <alvaro.karsz at solid-run.com> wrote:> Check vring size and fail probe if a transmit/receive vring size is > smaller than MAX_SKB_FRAGS + 2. > > At the moment, any vring size is accepted. This is problematic because > it may result in attempting to transmit a packet with more fragments > than there are descriptors in the ring. > > Furthermore, it leads to an immediate bug: > > The condition: (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) in > virtnet_poll_cleantx and virtnet_poll_tx always evaluates to false, > so netif_tx_wake_queue is not called, leading to TX timeouts. > > Signed-off-by: Alvaro Karsz <alvaro.karsz at solid-run.com> > --- > drivers/net/virtio_net.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 2396c28c012..59676252c5c 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -3745,6 +3745,26 @@ static int init_vqs(struct virtnet_info *vi) > return ret; > } > > +static int virtnet_validate_vqs(struct virtnet_info *vi) > +{ > + u32 i, min_size = roundup_pow_of_two(MAX_SKB_FRAGS + 2); > + > + /* Transmit/Receive vring size must be at least MAX_SKB_FRAGS + 2 > + * (fragments + linear part + virtio header) > + */ > + for (i = 0; i < vi->max_queue_pairs; i++) { > + if (virtqueue_get_vring_size(vi->sq[i].vq) < min_size || > + virtqueue_get_vring_size(vi->rq[i].vq) < min_size) { > + dev_warn(&vi->vdev->dev, > + "Transmit/Receive virtqueue vring size must be at least %u\n", > + min_size); > + return -EINVAL; > + } > + } > + > + return 0; > +} > + > #ifdef CONFIG_SYSFS > static ssize_t mergeable_rx_buffer_size_show(struct netdev_rx_queue *queue, > char *buf) > @@ -4056,6 +4076,10 @@ static int virtnet_probe(struct virtio_device *vdev) > if (err) > goto free; > > + err = virtnet_validate_vqs(vi); > + if (err) > + goto free_vqs; > +I wonder whether is better moving this to virtnet_find_vqs? Thanks> #ifdef CONFIG_SYSFS > if (vi->mergeable_rx_bufs) > dev->sysfs_rx_queue_group = &virtio_net_mrg_rx_group; > -- > 2.34.1 >