Re: [PATCH] virtio_ring: check desc == NULL when packed and indirect

On Mon, Sep 27, 2021 at 11:29 AM Xuan Zhuo [off-list ref] wrote:

On Mon, 27 Sep 2021 11:20:16 +0800, Jason Wang [off-list ref] wrote:

quoted

On Mon, Sep 27, 2021 at 11:06 AM Xuan Zhuo [off-list ref] wrote:

quoted

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

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index dd95dfd85e98..7dd381eae725 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c

@@ -1050,21 +1050,20 @@ 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[],
+                                        struct vring_packed_desc *desc,
+                                        unsigned int total_sg,
+                                        unsigned int out_sgs,
+                                        unsigned int in_sgs,
+                                        void *data,
+                                        gfp_t gfp)

It looks to me the style changes are mixed with bug fix here.

I will make a separate patch for the next version.

Please make it a patch on top of the fix to ease the backport.

quoted

quoted

 {
-       struct vring_packed_desc *desc;
        struct scatterlist *sg;
        unsigned int i, n, err_idx;
        u16 head, id;
        dma_addr_t addr;

        head = vq->packed.next_avail_idx;
-       desc = alloc_indirect_packed(total_sg, gfp);

        if (unlikely(vq->vq.num_free < 1)) {
                pr_debug("Can't add buf len 1 - avail = 0\n");

@@ -1191,9 +1190,15 @@ 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)) {
+               desc = alloc_indirect_packed(total_sg, gfp);
+               if (desc) {
+                       return virtqueue_add_indirect_packed(vq, sgs, desc,
+                                                            total_sg,
+                                                            out_sgs, in_sgs,
+                                                            data, gfp);
+               }
+       }

Any reason that we can't fix virtqueue_add_indirect_packed()? It can
help to reduce the changeset and ease the backport.

The purpose of this is to fall back to not using indirect when
alloc_indirect_packed returns NULL.

If we check the return value of alloc_indirect_packed() in
virtqueue_add_indirect_packed(), then MUST check the error returned by
virtqueue_add_indirect_packed() to determine whether to fall back to the mode
that does not use indirect.

Right, this aligns the behaviour of the split path. But please
describe this in the commit log.

Thanks

Thanks.

quoted

Thanks

quoted

        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