thr3ads.net - Virtualization - [PATCH 1/3] virtio_ring: always warn when descriptor chain exceeds queue size [Mar 2021]

If this information is useful, please help other people find it:
Share via:

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

head link

[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

head link

[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.
> 
> 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?


> 
> > 
> > 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 - Mar 2021 - [PATCH 1/3] virtio_ring: always warn when descriptor chain exceeds queue size

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

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

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