linux-kernel - Re: [PATCH v3 5/5] vduse: Support querying information of IOVA regions

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

Message-ID: <CACycT3vKFUdOSF7n79Cxt3pg0hF3uxdMjiN+=mNLwUJeVHtdxw@mail.gmail.com>
Date:   Thu, 21 Jul 2022 13:37:53 +0800
From:   Yongji Xie <xieyongji@...edance.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     mst <mst@...hat.com>, Liu Xiaodong <xiaodong.liu@...el.com>,
        Maxime Coquelin <maxime.coquelin@...hat.com>,
        Stefan Hajnoczi <stefanha@...hat.com>,
        songmuchun@...edance.com,
        virtualization <virtualization@...ts.linux-foundation.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 5/5] vduse: Support querying information of IOVA regions

On Wed, Jul 20, 2022 at 5:15 PM Jason Wang <jasowang@...hat.com> wrote:
>
> On Wed, Jul 20, 2022 at 12:42 PM Xie Yongji <xieyongji@...edance.com> wrote:
> >
> > This introduces a new ioctl: VDUSE_IOTLB_GET_INFO to
> > support querying some information of IOVA regions.
> >
> > Now it can be used to query whether the IOVA region
> > supports userspace memory registration.
> >
> > Signed-off-by: Xie Yongji <xieyongji@...edance.com>
> > ---
> >  drivers/vdpa/vdpa_user/vduse_dev.c | 39 ++++++++++++++++++++++++++++++
> >  include/uapi/linux/vduse.h         | 25 +++++++++++++++++++
> >  2 files changed, 64 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index eedff0a3885a..cc4a9a700c24 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -1228,6 +1228,45 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> >                                            umem.size);
> >                 break;
> >         }
> > +       case VDUSE_IOTLB_GET_INFO: {
> > +               struct vduse_iova_info info;
> > +               struct vhost_iotlb_map *map;
> > +               struct vduse_iova_domain *domain = dev->domain;
> > +
> > +               ret = -EFAULT;
> > +               if (copy_from_user(&info, argp, sizeof(info)))
> > +                       break;
> > +
> > +               ret = -EINVAL;
> > +               if (info.start > info.last)
> > +                       break;
> > +
> > +               if (!is_mem_zero((const char *)info.reserved,
> > +                                sizeof(info.reserved)))
> > +                       break;
> > +
> > +               spin_lock(&domain->iotlb_lock);
> > +               map = vhost_iotlb_itree_first(domain->iotlb,
> > +                                             info.start, info.last);
> > +               if (map) {
> > +                       info.start = map->start;
> > +                       info.last = map->last;
> > +                       info.capability = 0;
> > +                       if (domain->bounce_map && map->start >= 0 &&
> > +                           map->last < domain->bounce_size)
>
> We don't check map->start and map->last in GET_FD, any reason for
> those checks here?
>

Since we only support userspace memory registration for the bounce
buffer region. We don't need to set VDUSE_IOVA_CAP_UMEM_SUPPORT for
any other region.

> > +                               info.capability |= VDUSE_IOVA_CAP_UMEM_SUPPORT;
>
> Not a native speaker, but I think CAP is duplicated with SUPPORT.
>
> Something like VDUSE_IOVA_CAP_UMEM might be better.
>

OK.

> > +               }
> > +               spin_unlock(&domain->iotlb_lock);
> > +               if (!map)
> > +                       break;
> > +
> > +               ret = -EFAULT;
> > +               if (copy_to_user(argp, &info, sizeof(info)))
> > +                       break;
> > +
> > +               ret = 0;
> > +               break;
> > +       }
> >         default:
> >                 ret = -ENOIOCTLCMD;
> >                 break;
> > diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > index 9885e0571f09..720fb0b6d8ff 100644
> > --- a/include/uapi/linux/vduse.h
> > +++ b/include/uapi/linux/vduse.h
> > @@ -233,6 +233,31 @@ struct vduse_iova_umem {
> >  /* De-register the userspace memory. Caller should set iova and size field. */
> >  #define VDUSE_IOTLB_DEREG_UMEM _IOW(VDUSE_BASE, 0x19, struct vduse_iova_umem)
> >
> > +/**
> > + * struct vduse_iova_info - information of one IOVA region
> > + * @start: start of the IOVA region
> > + * @last: last of the IOVA region
> > + * @capability: capability of the IOVA regsion
> > + * @reserved: for future use, needs to be initialized to zero
> > + *
> > + * Structure used by VDUSE_IOTLB_GET_INFO ioctl to get information of
> > + * one IOVA region.
> > + */
> > +struct vduse_iova_info {
> > +       __u64 start;
> > +       __u64 last;
> > +#define VDUSE_IOVA_CAP_UMEM_SUPPORT (1 << 0)
> > +       __u64 capability;
> > +       __u64 reserved[3];
> > +};
> > +
> > +/*
> > + * Find the first IOVA region that overlaps with the range [start, last]
> > + * and return some information on it. Caller should set start and last fields.
> > + */
> > +#define VDUSE_IOTLB_GET_INFO   _IOW(VDUSE_BASE, 0x1a, struct vduse_iova_info)
>
> This should be _IOWR() ?
>

Will fix it.

Thanks,
Yongji