Michael S. Tsirkin
2021-Oct-19 11:07 UTC
[PATCH v3 2/2] virtio_ring: check desc == NULL when packed and indirect
On Tue, Oct 19, 2021 at 06:56:57PM +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. > > This patch alloc desc inside virtqueue_add_packe(), if desc == NULL,Can we manage without typos in commit log please?> fall back to not using indirect.It should say why is the patch created, and how it's fixed, as opposed to repating what patch does. E.g. when using indirect with packed, we don't check for allocation failures. Check and fall back on direct.> Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support") > Signed-off-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com> > Acked-by: Jason Wang <jasowang at redhat.com> > --- > drivers/virtio/virtio_ring.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 91a46c4da87d..62323c27bfe4 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -1051,20 +1051,19 @@ 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[], > + struct vring_packed_desc *desc, > unsigned int total_sg, > unsigned int out_sgs, > unsigned int in_sgs, > void *data, > gfp_t gfp)So this gets desc and will free it. I don't much like this.> { > - 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,14 @@ 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); > + } > > head = vq->packed.next_avail_idx; > avail_used_flags = vq->packed.avail_used_flags; > -- > 2.31.0
Xuan Zhuo
2021-Oct-19 11:12 UTC
[PATCH v3 2/2] virtio_ring: check desc == NULL when packed and indirect
On Tue, 19 Oct 2021 07:07:58 -0400, Michael S. Tsirkin <mst at redhat.com> wrote:> On Tue, Oct 19, 2021 at 06:56:57PM +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. > > > > This patch alloc desc inside virtqueue_add_packe(), if desc == NULL, > > Can we manage without typos in commit log please?I'm sorry. virtqueue_add_packe => virtqueue_add_packed> > > fall back to not using indirect. > > > It should say why is the patch created, and how it's fixed, > as opposed to repating what patch does. > > E.g. > > when using indirect with packed, we don't check for > allocation failures. Check and fall back on direct.It's better. I know how to do it.> > > > > Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support") > > Signed-off-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com> > > Acked-by: Jason Wang <jasowang at redhat.com> > > --- > > drivers/virtio/virtio_ring.c | 14 +++++++++----- > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index 91a46c4da87d..62323c27bfe4 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -1051,20 +1051,19 @@ 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[], > > + struct vring_packed_desc *desc, > > unsigned int total_sg, > > unsigned int out_sgs, > > unsigned int in_sgs, > > void *data, > > gfp_t gfp) > > So this gets desc and will free it. I don't much like this.Under normal circumstances, this function will not release desc. It will actually release "desc" in detach_buf_packed(). Of course, if you encounter an error in this function, it will also release desc in this function. If we call alloc_indirect_packed() in virtqueue_add_indirect_packed() it is also possible, we need to make virtqueue_add_indirect_packed() return a special return value. Thanks.> > > > { > > - 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,14 @@ 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); > > + } > > > > head = vq->packed.next_avail_idx; > > avail_used_flags = vq->packed.avail_used_flags; > > -- > > 2.31.0 >