linux-kernel - Re: [PATCH v6 6/8] virtio_vdpa: setup correct vq size with callbacks get_vq_num

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>] [day] [month] [year] [list]

Date:   Mon, 25 Oct 2021 12:45:44 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Wu Zongyong <wuzongyong@...ux.alibaba.com>
Cc:     virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org, mst@...hat.com,
        wei.yang1@...ux.alibaba.com
Subject: Re: [PATCH v6 6/8] virtio_vdpa: setup correct vq size with callbacks
 get_vq_num_{max,min}


在 2021/10/25 上午10:44, Wu Zongyong 写道:
> On Mon, Oct 25, 2021 at 10:22:30AM +0800, Jason Wang wrote:
>> On Fri, Oct 22, 2021 at 10:45 AM Wu Zongyong
>> <wuzongyong@...ux.alibaba.com> wrote:
>>> For the devices which implement the get_vq_num_min callback, the driver
>>> should not negotiate with virtqueue size with the backend vdpa device if
>>> the value returned by get_vq_num_min equals to the value returned by
>>> get_vq_num_max.
>>> This is useful for vdpa devices based on legacy virtio specfication.
>>>
>>> Signed-off-by: Wu Zongyong <wuzongyong@...ux.alibaba.com>
>>> ---
>>>   drivers/virtio/virtio_vdpa.c | 21 ++++++++++++++++-----
>>>   1 file changed, 16 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
>>> index 72eaef2caeb1..e42ace29daa1 100644
>>> --- a/drivers/virtio/virtio_vdpa.c
>>> +++ b/drivers/virtio/virtio_vdpa.c
>>> @@ -145,7 +145,8 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
>>>          /* Assume split virtqueue, switch to packed if necessary */
>>>          struct vdpa_vq_state state = {0};
>>>          unsigned long flags;
>>> -       u32 align, num;
>>> +       u32 align, max_num, min_num = 0;
>>> +       bool may_reduce_num = true;
>>>          int err;
>>>
>>>          if (!name)
>>> @@ -163,22 +164,32 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
>>>          if (!info)
>>>                  return ERR_PTR(-ENOMEM);
>>>
>>> -       num = ops->get_vq_num_max(vdpa);
>>> -       if (num == 0) {
>>> +       max_num = ops->get_vq_num_max(vdpa);
>>> +       if (max_num == 0) {
>>>                  err = -ENOENT;
>>>                  goto error_new_virtqueue;
>>>          }
>>>
>>> +       if (ops->get_vq_num_min)
>>> +               min_num = ops->get_vq_num_min(vdpa);
>>> +
>>> +       may_reduce_num = (max_num == min_num) ? false : true;
>>> +
>>>          /* Create the vring */
>>>          align = ops->get_vq_align(vdpa);
>>> -       vq = vring_create_virtqueue(index, num, align, vdev,
>>> -                                   true, true, ctx,
>>> +       vq = vring_create_virtqueue(index, max_num, align, vdev,
>>> +                                   true, may_reduce_num, ctx,
>>>                                      virtio_vdpa_notify, callback, name);
>>>          if (!vq) {
>>>                  err = -ENOMEM;
>>>                  goto error_new_virtqueue;
>>>          }
>>>
>>> +       if (virtqueue_get_vring_size(vq) < min_num) {
>>> +               err = -EINVAL;
>>> +               goto err_vq;
>>> +       }
>> I wonder under which case can we hit this?
>>
>> Thanks
> If min_vq_num < max_vq_num, may_reduce_num should be true, then it is
> possible to allocate a virtqueue with a small size which value is less
> than the min_vq_num since we only set the upper bound for virtqueue size
> when creating virtqueue.
>
> Refers to vring_create_virtqueue_split in driver/virtio/virtio_vring.c:
>
> 	for (; num && vring_size(num, vring_align) > PAGE_SIZE; num /= 2) {
> 		queue = vring_alloc_queue(vdev, vring_size(num, vring_align),
> 					  &dma_addr,
> 					  GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
> 		if (queue)
> 			break;
> 		if (!may_reduce_num)
> 			return NULL;
> 	}


It looks to me it's better to fix this function instead of checking it 
in the caller?


>
> BTW, I have replied this mail on Nov.18, have you ever received it?


For some reason I dont' get that.

Thanks


>>> +
>>>          /* Setup virtqueue callback */
>>>          cb.callback = virtio_vdpa_virtqueue_cb;
>>>          cb.private = info;
>>> --
>>> 2.31.1
>>>