lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 7 Mar 2022 17:17:51 -0500
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Cc:     virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
        Jeff Dike <jdike@...toit.com>,
        Richard Weinberger <richard@....at>,
        Anton Ivanov <anton.ivanov@...bridgegreys.com>,
        Jason Wang <jasowang@...hat.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>,
        Vadim Pasternak <vadimp@...dia.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Cornelia Huck <cohuck@...hat.com>,
        Halil Pasic <pasic@...ux.ibm.com>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        Sven Schnelle <svens@...ux.ibm.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        Johannes Berg <johannes.berg@...el.com>,
        Vincent Whitchurch <vincent.whitchurch@...s.com>,
        linux-um@...ts.infradead.org, platform-driver-x86@...r.kernel.org,
        linux-remoteproc@...r.kernel.org, linux-s390@...r.kernel.org,
        kvm@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH v6 06/26] virtio_ring: packed: extrace the logic of
 creating vring

On Thu, Feb 24, 2022 at 04:10:42PM +0800, Xuan Zhuo wrote:
> Separate the logic of packed to create vring queue.
> 
> For the convenience of passing parameters, add a structure
> vring_packed.
> 
> This feature is required for subsequent virtuqueue reset vring.
> 
> Signed-off-by: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>

Subject has a typo.
Besides:

> ---
>  drivers/virtio/virtio_ring.c | 121 ++++++++++++++++++++++++++---------
>  1 file changed, 92 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index dc6313b79305..41864c5e665f 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -92,6 +92,18 @@ struct vring_split {
>  	struct vring vring;
>  };
>  
> +struct vring_packed {
> +	u32 num;
> +	struct vring_packed_desc *ring;
> +	struct vring_packed_desc_event *driver;
> +	struct vring_packed_desc_event *device;
> +	dma_addr_t ring_dma_addr;
> +	dma_addr_t driver_event_dma_addr;
> +	dma_addr_t device_event_dma_addr;
> +	size_t ring_size_in_bytes;
> +	size_t event_size_in_bytes;
> +};
> +
>  struct vring_virtqueue {
>  	struct virtqueue vq;
>  
> @@ -1683,45 +1695,101 @@ static struct vring_desc_extra *vring_alloc_desc_extra(struct vring_virtqueue *v
>  	return desc_extra;
>  }
>  
> -static struct virtqueue *vring_create_virtqueue_packed(
> -	unsigned int index,
> -	unsigned int num,
> -	unsigned int vring_align,
> -	struct virtio_device *vdev,
> -	bool weak_barriers,
> -	bool may_reduce_num,
> -	bool context,
> -	bool (*notify)(struct virtqueue *),
> -	void (*callback)(struct virtqueue *),
> -	const char *name)
> +static void vring_free_vring_packed(struct vring_packed *vring,
> +				    struct virtio_device *vdev)
> +{
> +	dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
> +	struct vring_packed_desc_event *driver, *device;
> +	size_t ring_size_in_bytes, event_size_in_bytes;
> +	struct vring_packed_desc *ring;
> +
> +	ring                  = vring->ring;
> +	driver                = vring->driver;
> +	device                = vring->device;
> +	ring_dma_addr         = vring->ring_size_in_bytes;
> +	event_size_in_bytes   = vring->event_size_in_bytes;
> +	ring_dma_addr         = vring->ring_dma_addr;
> +	driver_event_dma_addr = vring->driver_event_dma_addr;
> +	device_event_dma_addr = vring->device_event_dma_addr;
> +
> +	if (device)
> +		vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr);
> +
> +	if (driver)
> +		vring_free_queue(vdev, event_size_in_bytes, driver, driver_event_dma_addr);
> +
> +	if (ring)
> +		vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);

ring_size_in_bytes is uninitialized here.

Which begs the question how was this tested patchset generally and
this patch in particular.
Please add note on tested configurations and tests run to the patchset.

> +}
> +
> +static int vring_create_vring_packed(struct vring_packed *vring,
> +				    struct virtio_device *vdev,
> +				    u32 num)
>  {
> -	struct vring_virtqueue *vq;
>  	struct vring_packed_desc *ring;
>  	struct vring_packed_desc_event *driver, *device;
>  	dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
>  	size_t ring_size_in_bytes, event_size_in_bytes;
>  
> +	memset(vring, 0, sizeof(*vring));
> +
>  	ring_size_in_bytes = num * sizeof(struct vring_packed_desc);
>  
>  	ring = vring_alloc_queue(vdev, ring_size_in_bytes,
>  				 &ring_dma_addr,
>  				 GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
>  	if (!ring)
> -		goto err_ring;
> +		goto err;
> +
> +	vring->num = num;
> +	vring->ring = ring;
> +	vring->ring_size_in_bytes = ring_size_in_bytes;
> +	vring->ring_dma_addr = ring_dma_addr;
>  
>  	event_size_in_bytes = sizeof(struct vring_packed_desc_event);
> +	vring->event_size_in_bytes = event_size_in_bytes;
>  
>  	driver = vring_alloc_queue(vdev, event_size_in_bytes,
>  				   &driver_event_dma_addr,
>  				   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
>  	if (!driver)
> -		goto err_driver;
> +		goto err;
> +
> +	vring->driver = driver;
> +	vring->driver_event_dma_addr = driver_event_dma_addr;
>  
>  	device = vring_alloc_queue(vdev, event_size_in_bytes,
>  				   &device_event_dma_addr,
>  				   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
>  	if (!device)
> -		goto err_device;
> +		goto err;
> +
> +	vring->device = device;
> +	vring->device_event_dma_addr = device_event_dma_addr;
> +	return 0;
> +
> +err:
> +	vring_free_vring_packed(vring, vdev);
> +	return -ENOMEM;
> +}
> +
> +static struct virtqueue *vring_create_virtqueue_packed(
> +	unsigned int index,
> +	unsigned int num,
> +	unsigned int vring_align,
> +	struct virtio_device *vdev,
> +	bool weak_barriers,
> +	bool may_reduce_num,
> +	bool context,
> +	bool (*notify)(struct virtqueue *),
> +	void (*callback)(struct virtqueue *),
> +	const char *name)
> +{
> +	struct vring_virtqueue *vq;
> +	struct vring_packed vring;
> +
> +	if (vring_create_vring_packed(&vring, vdev, num))
> +		goto err_vq;
>  
>  	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
>  	if (!vq)
> @@ -1753,17 +1821,17 @@ static struct virtqueue *vring_create_virtqueue_packed(
>  	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
>  		vq->weak_barriers = false;
>  
> -	vq->packed.ring_dma_addr = ring_dma_addr;
> -	vq->packed.driver_event_dma_addr = driver_event_dma_addr;
> -	vq->packed.device_event_dma_addr = device_event_dma_addr;
> +	vq->packed.ring_dma_addr = vring.ring_dma_addr;
> +	vq->packed.driver_event_dma_addr = vring.driver_event_dma_addr;
> +	vq->packed.device_event_dma_addr = vring.device_event_dma_addr;
>  
> -	vq->packed.ring_size_in_bytes = ring_size_in_bytes;
> -	vq->packed.event_size_in_bytes = event_size_in_bytes;
> +	vq->packed.ring_size_in_bytes = vring.ring_size_in_bytes;
> +	vq->packed.event_size_in_bytes = vring.event_size_in_bytes;
>  
>  	vq->packed.vring.num = num;
> -	vq->packed.vring.desc = ring;
> -	vq->packed.vring.driver = driver;
> -	vq->packed.vring.device = device;
> +	vq->packed.vring.desc = vring.ring;
> +	vq->packed.vring.driver = vring.driver;
> +	vq->packed.vring.device = vring.device;
>  
>  	vq->packed.next_avail_idx = 0;
>  	vq->packed.avail_wrap_counter = 1;
> @@ -1804,12 +1872,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
>  err_desc_state:
>  	kfree(vq);
>  err_vq:
> -	vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr);
> -err_device:
> -	vring_free_queue(vdev, event_size_in_bytes, driver, driver_event_dma_addr);
> -err_driver:
> -	vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
> -err_ring:
> +	vring_free_vring_packed(&vring, vdev);
>  	return NULL;
>  }
>  
> -- 
> 2.31.0

Powered by blists - more mailing lists