Jason Wang
2021-Sep-27 03:20 UTC
[PATCH] virtio_ring: check desc == NULL when packed and indirect
On Mon, Sep 27, 2021 at 11:06 AM Xuan Zhuo <xuanzhuo at linux.alibaba.com> 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. > > Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support") > Signed-off-by: Xuan Zhuo <xuanzhuo at 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.> { > - 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. Thanks> > head = vq->packed.next_avail_idx; > avail_used_flags = vq->packed.avail_used_flags; > -- > 2.31.0 >
Xuan Zhuo
2021-Sep-27 03:25 UTC
[PATCH] virtio_ring: check desc == NULL when packed and indirect
On Mon, 27 Sep 2021 11:20:16 +0800, Jason Wang <jasowang at redhat.com> wrote:> On Mon, Sep 27, 2021 at 11:06 AM Xuan Zhuo <xuanzhuo at linux.alibaba.com> 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. > > > > Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support") > > Signed-off-by: Xuan Zhuo <xuanzhuo at 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.> > > { > > - 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. Thanks.> > Thanks > > > > > head = vq->packed.next_avail_idx; > > avail_used_flags = vq->packed.avail_used_flags; > > -- > > 2.31.0 > > >