[PATCH v4 0/2] virtio_ring: check desc == NULL when packed and indirect

[PATCH v4 0/2] virtio_ring: check desc == NULL when packed and indirect

[Xuan Zhuo]

In the case of packed, use indirect desc, since desc is allocated by
kmalloc_array(), we should check whether its return value is NULL.

v4:
   Inside the #2 patch, virtqueue_add_indirect_packed() return -EAGAIN when
   desc == NULL.

v3:
    Update commit message of the #1 patch.

v2:
    Separate the style fix into a single patch.


Xuan Zhuo (2):
  virtio_ring: fix style of virtqueue_add_indirect_packed
  virtio_ring: check desc == NULL when using indirect with packed

 drivers/virtio/virtio_ring.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

--
2.31.0

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

[PATCH v4 1/2] virtio_ring: fix style of virtqueue_add_indirect_packed

[Xuan Zhuo]

Align the arguments of virtqueue_add_indirect_packed() to the open ( to
make it look prettier.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio_ring.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index dd95dfd85e98..91a46c4da87d 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1050,12 +1050,12 @@ static struct vring_packed_desc *alloc_indirect_packed(unsigned int total_sg,
 }
 
 static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
-				       struct scatterlist *sgs[],
-				       unsigned int total_sg,
-				       unsigned int out_sgs,
-				       unsigned int in_sgs,
-				       void *data,
-				       gfp_t gfp)
+					 struct scatterlist *sgs[],
+					 unsigned int total_sg,
+					 unsigned int out_sgs,
+					 unsigned int in_sgs,
+					 void *data,
+					 gfp_t gfp)
 {
 	struct vring_packed_desc *desc;
 	struct scatterlist *sg;
-- 
2.31.0

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

[PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed

[Xuan Zhuo]

When using indirect with packed, we don't check for allocation failures.
This patch checks that and fall back on direct.

Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support")
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_ring.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 91a46c4da87d..44a03b6e4dc4 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1065,6 +1065,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 
 	head = vq->packed.next_avail_idx;
 	desc = alloc_indirect_packed(total_sg, gfp);
+	if (!desc)
+		/* fall back on direct */
+		return -EAGAIN;
 
 	if (unlikely(vq->vq.num_free < 1)) {
 		pr_debug("Can't add buf len 1 - avail = 0\n");
@@ -1176,6 +1179,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
 	unsigned int i, n, c, descs_used, err_idx;
 	__le16 head_flags, flags;
 	u16 head, id, prev, curr, avail_used_flags;
+	int err;
 
 	START_USE(vq);
 
@@ -1191,9 +1195,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
 
 	BUG_ON(total_sg == 0);
 
-	if (virtqueue_use_indirect(_vq, total_sg))
-		return virtqueue_add_indirect_packed(vq, sgs, total_sg,
-				out_sgs, in_sgs, data, gfp);
+	if (virtqueue_use_indirect(_vq, total_sg)) {
+		err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs,
+						    in_sgs, data, gfp);
+		if (err != -EAGAIN)
+			return err;
+	}
 
 	head = vq->packed.next_avail_idx;
 	avail_used_flags = vq->packed.avail_used_flags;
-- 
2.31.0

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

Re: [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed

[Michael S. Tsirkin]

On Tue, Oct 19, 2021 at 07:52:35PM +0800, Xuan Zhuo wrote:
> When using indirect with packed, we don't check for allocation failures.
> This patch checks that and fall back on direct.
> 
> Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support")
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  drivers/virtio/virtio_ring.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 91a46c4da87d..44a03b6e4dc4 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1065,6 +1065,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>  
>  	head = vq->packed.next_avail_idx;
>  	desc = alloc_indirect_packed(total_sg, gfp);
> +	if (!desc)
> +		/* fall back on direct */

this comment belongs in virtqueue_add_packed right after
return.

> +		return -EAGAIN;
>  

-ENOMEM surely?

>  	if (unlikely(vq->vq.num_free < 1)) {
>  		pr_debug("Can't add buf len 1 - avail = 0\n");
> @@ -1176,6 +1179,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  	unsigned int i, n, c, descs_used, err_idx;
>  	__le16 head_flags, flags;
>  	u16 head, id, prev, curr, avail_used_flags;
> +	int err;
>  
>  	START_USE(vq);
>  
> @@ -1191,9 +1195,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  
>  	BUG_ON(total_sg == 0);
>  
> -	if (virtqueue_use_indirect(_vq, total_sg))
> -		return virtqueue_add_indirect_packed(vq, sgs, total_sg,
> -				out_sgs, in_sgs, data, gfp);
> +	if (virtqueue_use_indirect(_vq, total_sg)) {
> +		err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs,
> +						    in_sgs, data, gfp);
> +		if (err != -EAGAIN)
> +			return err;
> +	}
>  
>  	head = vq->packed.next_avail_idx;
>  	avail_used_flags = vq->packed.avail_used_flags;
> -- 
> 2.31.0

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

Re: [PATCH v4 1/2] virtio_ring: fix style of virtqueue_add_indirect_packed

[Michael S. Tsirkin]

On Tue, Oct 19, 2021 at 07:52:34PM +0800, Xuan Zhuo wrote:
> Align the arguments of virtqueue_add_indirect_packed() to the open ( to
> make it look prettier.
> 
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Acked-by: Jason Wang <jasowang@redhat.com>

ok now update subject to match pls

> ---
>  drivers/virtio/virtio_ring.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index dd95dfd85e98..91a46c4da87d 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1050,12 +1050,12 @@ static struct vring_packed_desc *alloc_indirect_packed(unsigned int total_sg,
>  }
>  
>  static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> -				       struct scatterlist *sgs[],
> -				       unsigned int total_sg,
> -				       unsigned int out_sgs,
> -				       unsigned int in_sgs,
> -				       void *data,
> -				       gfp_t gfp)
> +					 struct scatterlist *sgs[],
> +					 unsigned int total_sg,
> +					 unsigned int out_sgs,
> +					 unsigned int in_sgs,
> +					 void *data,
> +					 gfp_t gfp)
>  {
>  	struct vring_packed_desc *desc;
>  	struct scatterlist *sg;
> -- 
> 2.31.0

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

Re: [PATCH v4 0/2] virtio_ring: check desc == NULL when packed and indirect

[Michael S. Tsirkin]

On Tue, Oct 19, 2021 at 07:52:33PM +0800, Xuan Zhuo wrote:
> In the case of packed, use indirect desc, since desc is allocated by
> kmalloc_array(), we should check whether its return value is NULL.
> 

a better description here:

fix theoretical issues in virtio_ring (so I'm guessing - or did
you observe any null pointer dereferences?)

> v4:
>    Inside the #2 patch, virtqueue_add_indirect_packed() return -EAGAIN when
>    desc == NULL.
> 
> v3:
>     Update commit message of the #1 patch.
> 
> v2:
>     Separate the style fix into a single patch.
> 
> 
> Xuan Zhuo (2):
>   virtio_ring: fix style of virtqueue_add_indirect_packed
>   virtio_ring: check desc == NULL when using indirect with packed
> 
>  drivers/virtio/virtio_ring.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> --
> 2.31.0

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

Re: [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed

[Michael S. Tsirkin]

On Tue, Oct 19, 2021 at 07:52:35PM +0800, Xuan Zhuo wrote:
> When using indirect with packed, we don't check for allocation failures.
> This patch checks that and fall back on direct.
> 
> Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support")
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

Was a crash observed with this? It seems quite likely ...


> ---
>  drivers/virtio/virtio_ring.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 91a46c4da87d..44a03b6e4dc4 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1065,6 +1065,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>  
>  	head = vq->packed.next_avail_idx;
>  	desc = alloc_indirect_packed(total_sg, gfp);
> +	if (!desc)
> +		/* fall back on direct */
> +		return -EAGAIN;
>  
>  	if (unlikely(vq->vq.num_free < 1)) {
>  		pr_debug("Can't add buf len 1 - avail = 0\n");
> @@ -1176,6 +1179,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  	unsigned int i, n, c, descs_used, err_idx;
>  	__le16 head_flags, flags;
>  	u16 head, id, prev, curr, avail_used_flags;
> +	int err;
>  
>  	START_USE(vq);
>  
> @@ -1191,9 +1195,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  
>  	BUG_ON(total_sg == 0);
>  
> -	if (virtqueue_use_indirect(_vq, total_sg))
> -		return virtqueue_add_indirect_packed(vq, sgs, total_sg,
> -				out_sgs, in_sgs, data, gfp);
> +	if (virtqueue_use_indirect(_vq, total_sg)) {
> +		err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs,
> +						    in_sgs, data, gfp);
> +		if (err != -EAGAIN)
> +			return err;
> +	}
>  
>  	head = vq->packed.next_avail_idx;
>  	avail_used_flags = vq->packed.avail_used_flags;
> -- 
> 2.31.0

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

Re: [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed

[Xuan Zhuo]

On Tue, 19 Oct 2021 09:21:51 -0400, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Oct 19, 2021 at 07:52:35PM +0800, Xuan Zhuo wrote:
> > When using indirect with packed, we don't check for allocation failures.
> > This patch checks that and fall back on direct.
> >
> > Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support")
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >  drivers/virtio/virtio_ring.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 91a46c4da87d..44a03b6e4dc4 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -1065,6 +1065,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> >
> >  	head = vq->packed.next_avail_idx;
> >  	desc = alloc_indirect_packed(total_sg, gfp);
> > +	if (!desc)
> > +		/* fall back on direct */
>
> this comment belongs in virtqueue_add_packed right after
> return.
>
> > +		return -EAGAIN;
> >
>
> -ENOMEM surely?

virtqueue_add_indirect_packed() will return -ENOMEM when dma mmap fails, so we
have to choose a different error code.

Thanks.

>
> >  	if (unlikely(vq->vq.num_free < 1)) {
> >  		pr_debug("Can't add buf len 1 - avail = 0\n");
> > @@ -1176,6 +1179,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> >  	unsigned int i, n, c, descs_used, err_idx;
> >  	__le16 head_flags, flags;
> >  	u16 head, id, prev, curr, avail_used_flags;
> > +	int err;
> >
> >  	START_USE(vq);
> >
> > @@ -1191,9 +1195,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> >
> >  	BUG_ON(total_sg == 0);
> >
> > -	if (virtqueue_use_indirect(_vq, total_sg))
> > -		return virtqueue_add_indirect_packed(vq, sgs, total_sg,
> > -				out_sgs, in_sgs, data, gfp);
> > +	if (virtqueue_use_indirect(_vq, total_sg)) {
> > +		err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs,
> > +						    in_sgs, data, gfp);
> > +		if (err != -EAGAIN)
> > +			return err;
> > +	}
> >
> >  	head = vq->packed.next_avail_idx;
> >  	avail_used_flags = vq->packed.avail_used_flags;
> > --
> > 2.31.0
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed

[Michael S. Tsirkin]

On Wed, Oct 20, 2021 at 10:19:46AM +0800, Xuan Zhuo wrote:
> On Tue, 19 Oct 2021 09:21:51 -0400, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, Oct 19, 2021 at 07:52:35PM +0800, Xuan Zhuo wrote:
> > > When using indirect with packed, we don't check for allocation failures.
> > > This patch checks that and fall back on direct.
> > >
> > > Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support")
> > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > ---
> > >  drivers/virtio/virtio_ring.c | 13 ++++++++++---
> > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index 91a46c4da87d..44a03b6e4dc4 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -1065,6 +1065,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> > >
> > >  	head = vq->packed.next_avail_idx;
> > >  	desc = alloc_indirect_packed(total_sg, gfp);
> > > +	if (!desc)
> > > +		/* fall back on direct */
> >
> > this comment belongs in virtqueue_add_packed right after
> > return.
> >
> > > +		return -EAGAIN;
> > >
> >
> > -ENOMEM surely?
> 
> virtqueue_add_indirect_packed() will return -ENOMEM when dma mmap fails, so we
> have to choose a different error code.
> 
> Thanks.

That's exactly the point.  Why would we want to recover from allocation
failure but not DMA map failure?

> >
> > >  	if (unlikely(vq->vq.num_free < 1)) {
> > >  		pr_debug("Can't add buf len 1 - avail = 0\n");
> > > @@ -1176,6 +1179,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > >  	unsigned int i, n, c, descs_used, err_idx;
> > >  	__le16 head_flags, flags;
> > >  	u16 head, id, prev, curr, avail_used_flags;
> > > +	int err;
> > >
> > >  	START_USE(vq);
> > >
> > > @@ -1191,9 +1195,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > >
> > >  	BUG_ON(total_sg == 0);
> > >
> > > -	if (virtqueue_use_indirect(_vq, total_sg))
> > > -		return virtqueue_add_indirect_packed(vq, sgs, total_sg,
> > > -				out_sgs, in_sgs, data, gfp);
> > > +	if (virtqueue_use_indirect(_vq, total_sg)) {
> > > +		err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs,
> > > +						    in_sgs, data, gfp);
> > > +		if (err != -EAGAIN)
> > > +			return err;
> > > +	}
> > >
> > >  	head = vq->packed.next_avail_idx;
> > >  	avail_used_flags = vq->packed.avail_used_flags;
> > > --
> > > 2.31.0
> >

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

Re: [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed

[Xuan Zhuo]

On Wed, 20 Oct 2021 04:24:33 -0400, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Oct 20, 2021 at 10:19:46AM +0800, Xuan Zhuo wrote:
> > On Tue, 19 Oct 2021 09:21:51 -0400, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Tue, Oct 19, 2021 at 07:52:35PM +0800, Xuan Zhuo wrote:
> > > > When using indirect with packed, we don't check for allocation failures.
> > > > This patch checks that and fall back on direct.
> > > >
> > > > Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support")
> > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > ---
> > > >  drivers/virtio/virtio_ring.c | 13 ++++++++++---
> > > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index 91a46c4da87d..44a03b6e4dc4 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -1065,6 +1065,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> > > >
> > > >  	head = vq->packed.next_avail_idx;
> > > >  	desc = alloc_indirect_packed(total_sg, gfp);
> > > > +	if (!desc)
> > > > +		/* fall back on direct */
> > >
> > > this comment belongs in virtqueue_add_packed right after
> > > return.
> > >
> > > > +		return -EAGAIN;
> > > >
> > >
> > > -ENOMEM surely?
> >
> > virtqueue_add_indirect_packed() will return -ENOMEM when dma mmap fails, so we
> > have to choose a different error code.
> >
> > Thanks.
>
> That's exactly the point.  Why would we want to recover from allocation
> failure but not DMA map failure?

From indirect to direct mode, there is no need to allocate memory, so if we
encounter memory allocation failure, we can fall back from indirect to direct
mode. Memory allocation failure is a temporary problem.

And if you encounter a dma mmap error, this situation should be notified to the
upper layer.

Thanks.

>
> > >
> > > >  	if (unlikely(vq->vq.num_free < 1)) {
> > > >  		pr_debug("Can't add buf len 1 - avail = 0\n");
> > > > @@ -1176,6 +1179,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > > >  	unsigned int i, n, c, descs_used, err_idx;
> > > >  	__le16 head_flags, flags;
> > > >  	u16 head, id, prev, curr, avail_used_flags;
> > > > +	int err;
> > > >
> > > >  	START_USE(vq);
> > > >
> > > > @@ -1191,9 +1195,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > > >
> > > >  	BUG_ON(total_sg == 0);
> > > >
> > > > -	if (virtqueue_use_indirect(_vq, total_sg))
> > > > -		return virtqueue_add_indirect_packed(vq, sgs, total_sg,
> > > > -				out_sgs, in_sgs, data, gfp);
> > > > +	if (virtqueue_use_indirect(_vq, total_sg)) {
> > > > +		err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs,
> > > > +						    in_sgs, data, gfp);
> > > > +		if (err != -EAGAIN)
> > > > +			return err;
> > > > +	}
> > > >
> > > >  	head = vq->packed.next_avail_idx;
> > > >  	avail_used_flags = vq->packed.avail_used_flags;
> > > > --
> > > > 2.31.0
> > >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed

[Michael S. Tsirkin]

On Wed, Oct 20, 2021 at 07:07:43PM +0800, Xuan Zhuo wrote:
> On Wed, 20 Oct 2021 04:24:33 -0400, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Wed, Oct 20, 2021 at 10:19:46AM +0800, Xuan Zhuo wrote:
> > > On Tue, 19 Oct 2021 09:21:51 -0400, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > On Tue, Oct 19, 2021 at 07:52:35PM +0800, Xuan Zhuo wrote:
> > > > > When using indirect with packed, we don't check for allocation failures.
> > > > > This patch checks that and fall back on direct.
> > > > >
> > > > > Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support")
> > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > ---
> > > > >  drivers/virtio/virtio_ring.c | 13 ++++++++++---
> > > > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > index 91a46c4da87d..44a03b6e4dc4 100644
> > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > @@ -1065,6 +1065,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> > > > >
> > > > >  	head = vq->packed.next_avail_idx;
> > > > >  	desc = alloc_indirect_packed(total_sg, gfp);
> > > > > +	if (!desc)
> > > > > +		/* fall back on direct */
> > > >
> > > > this comment belongs in virtqueue_add_packed right after
> > > > return.
> > > >
> > > > > +		return -EAGAIN;
> > > > >
> > > >
> > > > -ENOMEM surely?
> > >
> > > virtqueue_add_indirect_packed() will return -ENOMEM when dma mmap fails, so we
> > > have to choose a different error code.
> > >
> > > Thanks.
> >
> > That's exactly the point.  Why would we want to recover from allocation
> > failure but not DMA map failure?
> 
> >From indirect to direct mode, there is no need to allocate memory, so if we
> encounter memory allocation failure, we can fall back from indirect to direct
> mode. Memory allocation failure is a temporary problem.
> 
> And if you encounter a dma mmap error, this situation should be notified to the
> upper layer.
> 
> Thanks.

Why did dma map fail?
A common reason is precisely that we are running
out of buffer space. Using direct which does not
need extra buffer space thus has a chance to work.


> >
> > > >
> > > > >  	if (unlikely(vq->vq.num_free < 1)) {
> > > > >  		pr_debug("Can't add buf len 1 - avail = 0\n");
> > > > > @@ -1176,6 +1179,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > > > >  	unsigned int i, n, c, descs_used, err_idx;
> > > > >  	__le16 head_flags, flags;
> > > > >  	u16 head, id, prev, curr, avail_used_flags;
> > > > > +	int err;
> > > > >
> > > > >  	START_USE(vq);
> > > > >
> > > > > @@ -1191,9 +1195,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > > > >
> > > > >  	BUG_ON(total_sg == 0);
> > > > >
> > > > > -	if (virtqueue_use_indirect(_vq, total_sg))
> > > > > -		return virtqueue_add_indirect_packed(vq, sgs, total_sg,
> > > > > -				out_sgs, in_sgs, data, gfp);
> > > > > +	if (virtqueue_use_indirect(_vq, total_sg)) {
> > > > > +		err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs,
> > > > > +						    in_sgs, data, gfp);
> > > > > +		if (err != -EAGAIN)
> > > > > +			return err;
> > > > > +	}
> > > > >
> > > > >  	head = vq->packed.next_avail_idx;
> > > > >  	avail_used_flags = vq->packed.avail_used_flags;
> > > > > --
> > > > > 2.31.0
> > > >
> >

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

Re: [PATCH v4 2/2] virtio_ring: check desc == NULL when using indirect with packed

[Xuan Zhuo]

On Wed, 20 Oct 2021 10:52:24 -0400, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Oct 20, 2021 at 07:07:43PM +0800, Xuan Zhuo wrote:
> > On Wed, 20 Oct 2021 04:24:33 -0400, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Wed, Oct 20, 2021 at 10:19:46AM +0800, Xuan Zhuo wrote:
> > > > On Tue, 19 Oct 2021 09:21:51 -0400, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > On Tue, Oct 19, 2021 at 07:52:35PM +0800, Xuan Zhuo wrote:
> > > > > > When using indirect with packed, we don't check for allocation failures.
> > > > > > This patch checks that and fall back on direct.
> > > > > >
> > > > > > Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support")
> > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > > ---
> > > > > >  drivers/virtio/virtio_ring.c | 13 ++++++++++---
> > > > > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > > index 91a46c4da87d..44a03b6e4dc4 100644
> > > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > > @@ -1065,6 +1065,9 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> > > > > >
> > > > > >  	head = vq->packed.next_avail_idx;
> > > > > >  	desc = alloc_indirect_packed(total_sg, gfp);
> > > > > > +	if (!desc)
> > > > > > +		/* fall back on direct */
> > > > >
> > > > > this comment belongs in virtqueue_add_packed right after
> > > > > return.
> > > > >
> > > > > > +		return -EAGAIN;
> > > > > >
> > > > >
> > > > > -ENOMEM surely?
> > > >
> > > > virtqueue_add_indirect_packed() will return -ENOMEM when dma mmap fails, so we
> > > > have to choose a different error code.
> > > >
> > > > Thanks.
> > >
> > > That's exactly the point.  Why would we want to recover from allocation
> > > failure but not DMA map failure?
> >
> > >From indirect to direct mode, there is no need to allocate memory, so if we
> > encounter memory allocation failure, we can fall back from indirect to direct
> > mode. Memory allocation failure is a temporary problem.
> >
> > And if you encounter a dma mmap error, this situation should be notified to the
> > upper layer.
> >
> > Thanks.
>
> Why did dma map fail?
> A common reason is precisely that we are running
> out of buffer space. Using direct which does not
> need extra buffer space thus has a chance to work.

It turns out that this is the case, then I have no problem.

Thanks.

>
>
> > >
> > > > >
> > > > > >  	if (unlikely(vq->vq.num_free < 1)) {
> > > > > >  		pr_debug("Can't add buf len 1 - avail = 0\n");
> > > > > > @@ -1176,6 +1179,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > > > > >  	unsigned int i, n, c, descs_used, err_idx;
> > > > > >  	__le16 head_flags, flags;
> > > > > >  	u16 head, id, prev, curr, avail_used_flags;
> > > > > > +	int err;
> > > > > >
> > > > > >  	START_USE(vq);
> > > > > >
> > > > > > @@ -1191,9 +1195,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > > > > >
> > > > > >  	BUG_ON(total_sg == 0);
> > > > > >
> > > > > > -	if (virtqueue_use_indirect(_vq, total_sg))
> > > > > > -		return virtqueue_add_indirect_packed(vq, sgs, total_sg,
> > > > > > -				out_sgs, in_sgs, data, gfp);
> > > > > > +	if (virtqueue_use_indirect(_vq, total_sg)) {
> > > > > > +		err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs,
> > > > > > +						    in_sgs, data, gfp);
> > > > > > +		if (err != -EAGAIN)
> > > > > > +			return err;
> > > > > > +	}
> > > > > >
> > > > > >  	head = vq->packed.next_avail_idx;
> > > > > >  	avail_used_flags = vq->packed.avail_used_flags;
> > > > > > --
> > > > > > 2.31.0
> > > > >
> > >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization