Re: [RFC PATCH 10/10] vhost/vdpa: return configuration bytes read and written to user space

[Jason Wang]

On 2021/3/5 4:37 下午, Stefano Garzarella wrote:
> On Thu, Mar 04, 2021 at 04:31:22PM +0800, Jason Wang wrote:
> > On 2021/3/2 10:06 下午, Stefano Garzarella wrote:
> > > On Tue, Mar 02, 2021 at 12:05:35PM +0800, Jason Wang wrote:
> > > > On 2021/2/16 5:44 下午, Stefano Garzarella wrote:
> > > > > vdpa_get_config() and vdpa_set_config() now return the amount
> > > > > of bytes read and written, so let's return them to the user space.
> > > > >
> > > > > We also modify vhost_vdpa_config_validate() to return 0 (bytes read
> > > > > or written) instead of an error, when the buffer length is 0.
> > > > >
> > > > > Signed-off-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> > > > > ---
> > > > >  drivers/vhost/vdpa.c | 26 +++++++++++++++-----------
> > > > >  1 file changed, 15 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > > > > index 21eea2be5afa..b754c53171a7 100644
> > > > > --- a/drivers/vhost/vdpa.c
> > > > > +++ b/drivers/vhost/vdpa.c
> > > > > @@ -191,9 +191,6 @@ static ssize_t 
> > > > > vhost_vdpa_config_validate(struct vhost_vdpa *v,
> > > > >      struct vdpa_device *vdpa = v->vdpa;
> > > > >      u32 size = vdpa->config->get_config_size(vdpa);
> > > > > -    if (c->len == 0)
> > > > > -        return -EINVAL;
> > > > > -
> > > > >      return min(c->len, size);
> > > > >  }
> > > > > @@ -204,6 +201,7 @@ static long vhost_vdpa_get_config(struct 
> > > > > vhost_vdpa *v,
> > > > >      struct vhost_vdpa_config config;
> > > > >      unsigned long size = offsetof(struct vhost_vdpa_config, buf);
> > > > >      ssize_t config_size;
> > > > > +    long ret;
> > > > >      u8 *buf;
> > > > >      if (copy_from_user(&config, c, size))
> > > > > @@ -217,15 +215,18 @@ static long vhost_vdpa_get_config(struct 
> > > > > vhost_vdpa *v,
> > > > >      if (!buf)
> > > > >          return -ENOMEM;
> > > > > -    vdpa_get_config(vdpa, config.off, buf, config_size);
> > > > > -
> > > > > -    if (copy_to_user(c->buf, buf, config_size)) {
> > > > > -        kvfree(buf);
> > > > > -        return -EFAULT;
> > > > > +    ret = vdpa_get_config(vdpa, config.off, buf, config_size);
> > > > > +    if (ret < 0) {
> > > > > +        ret = -EFAULT;
> > > > > +        goto out;
> > > > >      }
> > > > > +    if (copy_to_user(c->buf, buf, config_size))
> > > > > +        ret = -EFAULT;
> > > > > +
> > > > > +out:
> > > > >      kvfree(buf);
> > > > > -    return 0;
> > > > > +    return ret;
> > > > >  }
> > > > >  static long vhost_vdpa_set_config(struct vhost_vdpa *v,
> > > > > @@ -235,6 +236,7 @@ static long vhost_vdpa_set_config(struct 
> > > > > vhost_vdpa *v,
> > > > >      struct vhost_vdpa_config config;
> > > > >      unsigned long size = offsetof(struct vhost_vdpa_config, buf);
> > > > >      ssize_t config_size;
> > > > > +    long ret;
> > > > >      u8 *buf;
> > > > >      if (copy_from_user(&config, c, size))
> > > > > @@ -248,10 +250,12 @@ static long vhost_vdpa_set_config(struct 
> > > > > vhost_vdpa *v,
> > > > >      if (IS_ERR(buf))
> > > > >          return PTR_ERR(buf);
> > > > > -    vdpa_set_config(vdpa, config.off, buf, config_size);
> > > > > +    ret = vdpa_set_config(vdpa, config.off, buf, config_size);
> > > > > +    if (ret < 0)
> > > > > +        ret = -EFAULT;
> > > > >      kvfree(buf);
> > > > > -    return 0;
> > > > > +    return ret;
> > > > >  }
> > > >
> > > >
> > > > So I wonder whether it's worth to return the number of bytes since 
> > > > we can't propogate the result to driver or driver doesn't care 
> > > > about that.
> > >
> > > Okay, but IIUC user space application that issue 
> > > VHOST_VDPA_GET_CONFIG ioctl can use the return value.
> >
> >
> > Yes, but it looks to it's too late to change since it's a userspace 
> > noticble behaviour.
>
> Yeah, this is a good point.
> I looked at QEMU and we only check if the value is not negative, so it 
> should work, but for other applications it could be a real change.
>
> Do we leave it as is?


Yes, I think we'd better be conservative here.

Thanks


> > > Should we change also 'struct virtio_config_ops' to propagate this 
> > > value also to virtio drivers?
> >
> >
> > I think not, the reason is the driver doesn't expect the get()/set() 
> > can fail...
>
> Got it.
>
> Thanks,
> Stefano