Re: [PATCH 1/3] virtio_ring: always warn when descriptor chain exceeds queue size

在 2021/3/18 下午9:52, Connor Kuehl 写道:

 From section 2.6.5.3.1 (Driver Requirements: Indirect Descriptors)
of the virtio spec:

   "A driver MUST NOT create a descriptor chain longer than the Queue
   Size of the device."

This text suggests that the warning should trigger even if
indirect descriptors are in use.

So I think at least the commit log needs some tweak.

For split virtqueue. We had:

2.6.5.2 Driver Requirements: The Virtqueue Descriptor Table

Drivers MUST NOT add a descriptor chain longer than 2^32 bytes in total; 
this implies that loops in the descriptor chain are forbidden!

2.6.5.3.1 Driver Requirements: Indirect Descriptors

A driver MUST NOT create a descriptor chain longer than the Queue Size 
of the device.

If I understand the spec correctly, the check is only needed for a 
single indirect descriptor table?

For packed virtqueue. We had:

2.7.17 Driver Requirements: Scatter-Gather Support

A driver MUST NOT create a descriptor list longer than allowed by the 
device.

A driver MUST NOT create a descriptor list longer than the Queue Size.

2.7.19 Driver Requirements: Indirect Descriptors

A driver MUST NOT create a descriptor chain longer than allowed by the 
device.

So it looks to me the packed part is fine.

Note that if I understand the spec correctly 2.7.17 implies 2.7.19.

Thanks

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

在 2021/3/22 上午11:22, Jason Wang 写道:

在 2021/3/18 下午9:52, Connor Kuehl 写道:

quoted

 From section 2.6.5.3.1 (Driver Requirements: Indirect Descriptors)
of the virtio spec:

   "A driver MUST NOT create a descriptor chain longer than the Queue
   Size of the device."

This text suggests that the warning should trigger even if
indirect descriptors are in use.

So I think at least the commit log needs some tweak.

For split virtqueue. We had:

2.6.5.2 Driver Requirements: The Virtqueue Descriptor Table

Drivers MUST NOT add a descriptor chain longer than 2^32 bytes in 
total; this implies that loops in the descriptor chain are forbidden!

2.6.5.3.1 Driver Requirements: Indirect Descriptors

A driver MUST NOT create a descriptor chain longer than the Queue Size 
of the device.

If I understand the spec correctly, the check is only needed for a 
single indirect descriptor table?

For packed virtqueue. We had:

2.7.17 Driver Requirements: Scatter-Gather Support

A driver MUST NOT create a descriptor list longer than allowed by the 
device.

A driver MUST NOT create a descriptor list longer than the Queue Size.

2.7.19 Driver Requirements: Indirect Descriptors

A driver MUST NOT create a descriptor chain longer than allowed by the 
device.

So it looks to me the packed part is fine.

Note that if I understand the spec correctly 2.7.17 implies 2.7.19.

Actually not. So in 2.7.7, spec said:

"

Some devices benefit by concurrently dispatching a large number of large 
requests. The VIRTIO_F_INDIRECT_DESC feature allows this. To increase 
ring capacity the driver can store a (read-only by the device) table of 
indirect descriptors anywhere in memory, and insert a descriptor in the 
main virtqueue (with Flags bit VIRTQ_DESC_F_INDIRECT on) that refers to 
a buffer element containing this indirect descriptor table; addr and len 
refer to the indirect table address and length in bytes, respectively.

"

And in 2.7.5, spec said

"

While unusual (most implementations either create all lists solely using 
non-indirect descriptors, or always use a single indirect element), if 
both features have been negotiated, mixing indirect and non-indirect 
descriptors in a ring is valid, as long as each list only contains 
descriptors of a given type.
"

So my understanding is that the indirect descriptor is used to sumbit 
the request whose #buffers is greater than the virtqueue size. And the 
spec allows the driver to create a list of indirect descriptors just 
need to make sure the number of indirect descriptors in this list must 
not exceed the size of the virtqueue (2.7.17). And for each indirector 
descriptor, the number of chained descriptor must not exceed the 
virtqueue size. So actually this aligns with split virtqueue.

So if I understand the spec correctly, what we need to do is to make 
sure the descriptor chained in the indirect descriptor table does not 
exceed the virtqueue size. That means we probably need to chain indirect 
descriptors instead of a warn here.

Thanks

Thanks

quoted

Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Connor Kuehl <redacted>
---
  drivers/virtio/virtio_ring.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 71e16b53e9c1..1bc290f9ba13 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c

@@ -444,11 +444,12 @@ static inline int virtqueue_add_split(struct 

virtqueue *_vq,
        head = vq->free_head;
  +    WARN_ON_ONCE(total_sg > vq->split.vring.num);
+
      if (virtqueue_use_indirect(_vq, total_sg))
          desc = alloc_indirect_split(_vq, total_sg, gfp);
      else {
          desc = NULL;
-        WARN_ON_ONCE(total_sg > vq->split.vring.num && !vq->indirect);
      }
        if (desc) {

@@ -1118,6 +1119,8 @@ static inline int virtqueue_add_packed(struct 

virtqueue *_vq,
        BUG_ON(total_sg == 0);
  +    WARN_ON_ONCE(total_sg > vq->packed.vring.num);
+
      if (virtqueue_use_indirect(_vq, total_sg))
          return virtqueue_add_indirect_packed(vq, sgs, total_sg,
                  out_sgs, in_sgs, data, gfp);

@@ -1125,8 +1128,6 @@ static inline int virtqueue_add_packed(struct 

virtqueue *_vq,
      head = vq->packed.next_avail_idx;
      avail_used_flags = vq->packed.avail_used_flags;
  -    WARN_ON_ONCE(total_sg > vq->packed.vring.num && !vq->indirect);
-
      desc = vq->packed.vring.desc;
      i = head;
      descs_used = total_sg;

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On Mon, Mar 22, 2021 at 11:22:15AM +0800, Jason Wang wrote:

在 2021/3/18 下午9:52, Connor Kuehl 写道:

quoted

 From section 2.6.5.3.1 (Driver Requirements: Indirect Descriptors)
of the virtio spec:

   "A driver MUST NOT create a descriptor chain longer than the Queue
   Size of the device."

This text suggests that the warning should trigger even if
indirect descriptors are in use.

So I think at least the commit log needs some tweak.

For split virtqueue. We had:

2.6.5.2 Driver Requirements: The Virtqueue Descriptor Table

Drivers MUST NOT add a descriptor chain longer than 2^32 bytes in total;
this implies that loops in the descriptor chain are forbidden!

2.6.5.3.1 Driver Requirements: Indirect Descriptors

A driver MUST NOT create a descriptor chain longer than the Queue Size of
the device.

If I understand the spec correctly, the check is only needed for a single
indirect descriptor table?

For packed virtqueue. We had:

2.7.17 Driver Requirements: Scatter-Gather Support

A driver MUST NOT create a descriptor list longer than allowed by the
device.

A driver MUST NOT create a descriptor list longer than the Queue Size.

2.7.19 Driver Requirements: Indirect Descriptors

A driver MUST NOT create a descriptor chain longer than allowed by the
device.

So it looks to me the packed part is fine.

Note that if I understand the spec correctly 2.7.17 implies 2.7.19.

Thanks

It would be quite strange for packed and split to differ here:
so for packed would you say there's no limit on # of descriptors at all?

I am guessing I just forgot to move this part from
the format specific to the common part of the spec.

This needs discussion in the TC mailing list - want to start a thread
there?

quoted

Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Connor Kuehl <redacted>
---
  drivers/virtio/virtio_ring.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 71e16b53e9c1..1bc290f9ba13 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c

@@ -444,11 +444,12 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
  	head = vq->free_head;
+	WARN_ON_ONCE(total_sg > vq->split.vring.num);
+
  	if (virtqueue_use_indirect(_vq, total_sg))
  		desc = alloc_indirect_split(_vq, total_sg, gfp);
  	else {
  		desc = NULL;
-		WARN_ON_ONCE(total_sg > vq->split.vring.num && !vq->indirect);
  	}
  	if (desc) {

@@ -1118,6 +1119,8 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
  	BUG_ON(total_sg == 0);
+	WARN_ON_ONCE(total_sg > vq->packed.vring.num);
+
  	if (virtqueue_use_indirect(_vq, total_sg))
  		return virtqueue_add_indirect_packed(vq, sgs, total_sg,
  				out_sgs, in_sgs, data, gfp);

@@ -1125,8 +1128,6 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
  	head = vq->packed.next_avail_idx;
  	avail_used_flags = vq->packed.avail_used_flags;
-	WARN_ON_ONCE(total_sg > vq->packed.vring.num && !vq->indirect);
-
  	desc = vq->packed.vring.desc;
  	i = head;
  	descs_used = total_sg;

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

在 2021/3/22 下午4:17, Michael S. Tsirkin 写道:

On Mon, Mar 22, 2021 at 11:22:15AM +0800, Jason Wang wrote:

quoted

在 2021/3/18 下午9:52, Connor Kuehl 写道:

quoted

  From section 2.6.5.3.1 (Driver Requirements: Indirect Descriptors)
of the virtio spec:

    "A driver MUST NOT create a descriptor chain longer than the Queue
    Size of the device."

This text suggests that the warning should trigger even if
indirect descriptors are in use.

So I think at least the commit log needs some tweak.

For split virtqueue. We had:

2.6.5.2 Driver Requirements: The Virtqueue Descriptor Table

Drivers MUST NOT add a descriptor chain longer than 2^32 bytes in total;
this implies that loops in the descriptor chain are forbidden!

2.6.5.3.1 Driver Requirements: Indirect Descriptors

A driver MUST NOT create a descriptor chain longer than the Queue Size of
the device.

If I understand the spec correctly, the check is only needed for a single
indirect descriptor table?

For packed virtqueue. We had:

2.7.17 Driver Requirements: Scatter-Gather Support

A driver MUST NOT create a descriptor list longer than allowed by the
device.

A driver MUST NOT create a descriptor list longer than the Queue Size.

2.7.19 Driver Requirements: Indirect Descriptors

A driver MUST NOT create a descriptor chain longer than allowed by the
device.

So it looks to me the packed part is fine.

Note that if I understand the spec correctly 2.7.17 implies 2.7.19.

Thanks

It would be quite strange for packed and split to differ here:
so for packed would you say there's no limit on # of descriptors at all?

I am guessing I just forgot to move this part from
the format specific to the common part of the spec.

This needs discussion in the TC mailing list - want to start a thread
there?

Will do.

Thanks

quoted

quoted

Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Connor Kuehl <redacted>
---
   drivers/virtio/virtio_ring.c | 7 ++++---
   1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 71e16b53e9c1..1bc290f9ba13 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c

@@ -444,11 +444,12 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
   	head = vq->free_head;
+	WARN_ON_ONCE(total_sg > vq->split.vring.num);
+
   	if (virtqueue_use_indirect(_vq, total_sg))
   		desc = alloc_indirect_split(_vq, total_sg, gfp);
   	else {
   		desc = NULL;
-		WARN_ON_ONCE(total_sg > vq->split.vring.num && !vq->indirect);
   	}
   	if (desc) {

@@ -1118,6 +1119,8 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
   	BUG_ON(total_sg == 0);
+	WARN_ON_ONCE(total_sg > vq->packed.vring.num);
+
   	if (virtqueue_use_indirect(_vq, total_sg))
   		return virtqueue_add_indirect_packed(vq, sgs, total_sg,
   				out_sgs, in_sgs, data, gfp);

@@ -1125,8 +1128,6 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
   	head = vq->packed.next_avail_idx;
   	avail_used_flags = vq->packed.avail_used_flags;
-	WARN_ON_ONCE(total_sg > vq->packed.vring.num && !vq->indirect);
-
   	desc = vq->packed.vring.desc;
   	i = head;
   	descs_used = total_sg;

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization