Jason Wang
2021-Mar-02 04:05 UTC
[RFC PATCH 10/10] vhost/vdpa: return configuration bytes read and written to user space
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 at redhat.com> > --- > 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. Thanks> > static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
Stefano Garzarella
2021-Mar-02 14:06 UTC
[RFC PATCH 10/10] vhost/vdpa: return configuration bytes read and written to user space
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 at redhat.com> >>--- >> 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. Should we change also 'struct virtio_config_ops' to propagate this value also to virtio drivers? Thanks, Stefano