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
Jason Wang
2021-Mar-04 08:31 UTC
[RFC PATCH 10/10] vhost/vdpa: return configuration bytes read and written to user space
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 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.Yes, but it looks to it's too late to change since it's a userspace noticble behaviour.> > 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... Thanks> > Thanks, > Stefano >