linux-kernel - Re: [PATCH v6 6/7] vduse: add vq group asid support

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]
Message-ID: <CACGkMEuRMXrt3zyQtrW8QvHJSVPqh5XGXtxBtir3UxGzjSmXOQ@mail.gmail.com>
Date: Thu, 9 Oct 2025 15:42:59 +0800
From: Jason Wang <jasowang@...hat.com>
To: Eugenio Pérez <eperezma@...hat.com>
Cc: "Michael S . Tsirkin" <mst@...hat.com>, linux-kernel@...r.kernel.org, 
	Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Laurent Vivier <lvivier@...hat.com>, 
	Yongji Xie <xieyongji@...edance.com>, Stefano Garzarella <sgarzare@...hat.com>, 
	Cindy Lu <lulu@...hat.com>, Maxime Coquelin <mcoqueli@...hat.com>, virtualization@...ts.linux.dev
Subject: Re: [PATCH v6 6/7] vduse: add vq group asid support

On Tue, Sep 30, 2025 at 12:52 AM Eugenio Pérez <eperezma@...hat.com> wrote:
>
> Add support for assigning Address Space Identifiers (ASIDs) to each VQ
> group.  This enables mapping each group into a distinct memory space.
>
> Now that the driver can change ASID in the middle of operation, the
> domain that each vq address point is also protected by domain_lock.
>
> Signed-off-by: Eugenio Pérez <eperezma@...hat.com>
> ---
> v6:
> * Make vdpa_dev_add use gotos for error handling (MST).
> * s/(dev->api_version < 1) ?/(dev->api_version < VDUSE_API_VERSION_1) ?/
>   (MST).
> * Fix struct name not matching in the doc.
>
> v5:
> * Properly return errno if copy_to_user returns >0 in VDUSE_IOTLB_GET_FD
>   ioctl (Jason).
> * Properly set domain bounce size to divide equally between nas (Jason).
> * Exclude "padding" member from the only >V1 members in
>   vduse_dev_request.
>
> v4:
> * Divide each domain bounce size between the device bounce size (Jason).
> * revert unneeded addr = NULL assignment (Jason)
> * Change if (x && (y || z)) return to if (x) { if (y) return; if (z)
>   return; } (Jason)
> * Change a bad multiline comment, using @ caracter instead of * (Jason).
> * Consider config->nas == 0 as a fail (Jason).
>
> v3:
> * Get the vduse domain through the vduse_as in the map functions
>   (Jason).
> * Squash with the patch creating the vduse_as struct (Jason).
> * Create VDUSE_DEV_MAX_AS instead of comparing agains a magic number
>   (Jason)
>
> v2:
> * Convert the use of mutex to rwlock.
>
> RFC v3:
> * Increase VDUSE_MAX_VQ_GROUPS to 0xffff (Jason). It was set to a lower
>   value to reduce memory consumption, but vqs are already limited to
>   that value and userspace VDUSE is able to allocate that many vqs.
> * Remove TODO about merging VDUSE_IOTLB_GET_FD ioctl with
>   VDUSE_IOTLB_GET_INFO.
> * Use of array_index_nospec in VDUSE device ioctls.
> * Embed vduse_iotlb_entry into vduse_iotlb_entry_v2.
> * Move the umem mutex to asid struct so there is no contention between
>   ASIDs.
>
> RFC v2:
> * Make iotlb entry the last one of vduse_iotlb_entry_v2 so the first
>   part of the struct is the same.
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 347 ++++++++++++++++++++---------
>  include/uapi/linux/vduse.h         |  53 ++++-
>  2 files changed, 290 insertions(+), 110 deletions(-)
>

[...]

> @@ -2154,7 +2280,8 @@ static int vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
>                         const struct vdpa_dev_set_config *config)
>  {
>         struct vduse_dev *dev;
> -       int ret;
> +       size_t domain_bounce_size;
> +       int ret, i;
>
>         mutex_lock(&vduse_lock);
>         dev = vduse_find_dev(name);
> @@ -2168,29 +2295,35 @@ static int vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
>                 return ret;
>
>         write_lock(&dev->domain_lock);
> -       if (!dev->domain)
> -               dev->domain = vduse_domain_create(VDUSE_IOVA_SIZE - 1,
> -                                                 dev->bounce_size);
> -       write_unlock(&dev->domain_lock);
> -       if (!dev->domain) {
> -               ret = -ENOMEM;
> -               goto domain_err;
> +       ret = 0;
> +
> +       domain_bounce_size = dev->bounce_size / dev->nas;

Could dev->nas be zero here?

It looks like we only have the check like this:

        dev->nas = (dev->api_version < VDUSE_API_VERSION_1) ? 1 : config->nas;

When API version >= 1, userspace could fill zero here.

> +       for (i = 0; i < dev->nas; ++i) {
> +               dev->as[i].domain = vduse_domain_create(VDUSE_IOVA_SIZE - 1,
> +                                                       domain_bounce_size);
> +               if (!dev->as[i].domain) {
> +                       ret = -ENOMEM;
> +                       goto err;
> +               }
>         }
>
> +       write_unlock(&dev->domain_lock);
> +
>         ret = _vdpa_register_device(&dev->vdev->vdpa, dev->vq_num);
> -       if (ret) {
> -               goto register_err;
> -       }
> +       if (ret)
> +               goto err;

We don't hold domain_lock here but err tries to unlock it.

>
>         return 0;
>
> -register_err:
> -       write_lock(&dev->domain_lock);
> -       vduse_domain_destroy(dev->domain);
> -       dev->domain = NULL;
> +err:
> +       for (int j = 0; j < i; j++) {
> +               if (dev->as[j].domain) {
> +                       vduse_domain_destroy(dev->as[j].domain);
> +                       dev->as[j].domain = NULL;
> +               }
> +       }
>         write_unlock(&dev->domain_lock);
>

Thanks