Jason Wang
2021-Mar-22 03:22 UTC
[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> > Reported-by: Stefan Hajnoczi <stefanha at redhat.com> > Signed-off-by: Connor Kuehl <ckuehl at redhat.com> > --- > 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;
Jason Wang
2021-Mar-22 03:41 UTC
[PATCH 1/3] virtio_ring: always warn when descriptor chain exceeds queue size
? 2021/3/22 ??11:22, Jason Wang ??:> > ? 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.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 > > >> >> Reported-by: Stefan Hajnoczi <stefanha at redhat.com> >> Signed-off-by: Connor Kuehl <ckuehl at redhat.com> >> --- >> ? 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 at lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Michael S. Tsirkin
2021-Mar-22 08:17 UTC
[PATCH 1/3] virtio_ring: always warn when descriptor chain exceeds queue size
On Mon, Mar 22, 2021 at 11:22:15AM +0800, Jason Wang wrote:> > ? 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. > > ThanksIt 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?> > > > > Reported-by: Stefan Hajnoczi <stefanha at redhat.com> > > Signed-off-by: Connor Kuehl <ckuehl at redhat.com> > > --- > > 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;