| Message ID | 20210204172230.85853-10-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vdpa: add vdpa simulator for block device | expand |
On 2021/2/5 上午1:22, Stefano Garzarella wrote: > get_config() and set_config() callbacks in the 'struct vdpa_config_ops' > usually already validated the inputs. Also now they can return an error, > so we don't need to validate them here anymore. > > Let's use the return value of these callbacks and return it in case of > error in vhost_vdpa_get_config() and vhost_vdpa_set_config(). > > Originally-by: Xie Yongji <xieyongji@bytedance.com> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > drivers/vhost/vdpa.c | 41 +++++++++++++---------------------------- > 1 file changed, 13 insertions(+), 28 deletions(-) > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c > index ef688c8c0e0e..d61e779000a8 100644 > --- a/drivers/vhost/vdpa.c > +++ b/drivers/vhost/vdpa.c > @@ -185,51 +185,35 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp) > return 0; > } > > -static int vhost_vdpa_config_validate(struct vhost_vdpa *v, > - struct vhost_vdpa_config *c) > -{ > - long size = 0; > - > - switch (v->virtio_id) { > - case VIRTIO_ID_NET: > - size = sizeof(struct virtio_net_config); > - break; > - } > - > - if (c->len == 0) > - return -EINVAL; > - > - if (c->len > size - c->off) > - return -E2BIG; > - > - return 0; > -} > - > static long vhost_vdpa_get_config(struct vhost_vdpa *v, > struct vhost_vdpa_config __user *c) > { > struct vdpa_device *vdpa = v->vdpa; > struct vhost_vdpa_config config; > unsigned long size = offsetof(struct vhost_vdpa_config, buf); > + long ret; > u8 *buf; > > if (copy_from_user(&config, c, size)) > return -EFAULT; > - if (vhost_vdpa_config_validate(v, &config)) > + if (config.len == 0) > return -EINVAL; > buf = kvzalloc(config.len, GFP_KERNEL); Then it means usersapce can allocate a very large memory. Rethink about this, we should limit the size here (e.g PAGE_SIZE) or fetch the config size first (either through a config ops as you suggested or a variable in the vdpa device that is initialized during device creation). Thanks > if (!buf) > return -ENOMEM; > > - vdpa_get_config(vdpa, config.off, buf, config.len); > + ret = vdpa_get_config(vdpa, config.off, buf, config.len); > + if (ret) > + goto out; > > if (copy_to_user(c->buf, buf, config.len)) { > - kvfree(buf); > - return -EFAULT; > + ret = -EFAULT; > + goto out; > } > > +out: > kvfree(buf); > - return 0; > + return ret; > } > > static long vhost_vdpa_set_config(struct vhost_vdpa *v, > @@ -239,21 +223,22 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v, > const struct vdpa_config_ops *ops = vdpa->config; > struct vhost_vdpa_config config; > unsigned long size = offsetof(struct vhost_vdpa_config, buf); > + long ret; > u8 *buf; > > if (copy_from_user(&config, c, size)) > return -EFAULT; > - if (vhost_vdpa_config_validate(v, &config)) > + if (config.len == 0) > return -EINVAL; > > buf = vmemdup_user(c->buf, config.len); > if (IS_ERR(buf)) > return PTR_ERR(buf); > > - ops->set_config(vdpa, config.off, buf, config.len); > + ret = ops->set_config(vdpa, config.off, buf, config.len); > > kvfree(buf); > - return 0; > + return ret; > } > > static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
On Fri, Feb 05, 2021 at 11:27:32AM +0800, Jason Wang wrote: > >On 2021/2/5 上午1:22, Stefano Garzarella wrote: >>get_config() and set_config() callbacks in the 'struct vdpa_config_ops' >>usually already validated the inputs. Also now they can return an error, >>so we don't need to validate them here anymore. >> >>Let's use the return value of these callbacks and return it in case of >>error in vhost_vdpa_get_config() and vhost_vdpa_set_config(). >> >>Originally-by: Xie Yongji <xieyongji@bytedance.com> >>Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >>--- >> drivers/vhost/vdpa.c | 41 +++++++++++++---------------------------- >> 1 file changed, 13 insertions(+), 28 deletions(-) >> >>diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c >>index ef688c8c0e0e..d61e779000a8 100644 >>--- a/drivers/vhost/vdpa.c >>+++ b/drivers/vhost/vdpa.c >>@@ -185,51 +185,35 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp) >> return 0; >> } >>-static int vhost_vdpa_config_validate(struct vhost_vdpa *v, >>- struct vhost_vdpa_config *c) >>-{ >>- long size = 0; >>- >>- switch (v->virtio_id) { >>- case VIRTIO_ID_NET: >>- size = sizeof(struct virtio_net_config); >>- break; >>- } >>- >>- if (c->len == 0) >>- return -EINVAL; >>- >>- if (c->len > size - c->off) >>- return -E2BIG; >>- >>- return 0; >>-} >>- >> static long vhost_vdpa_get_config(struct vhost_vdpa *v, >> struct vhost_vdpa_config __user *c) >> { >> struct vdpa_device *vdpa = v->vdpa; >> struct vhost_vdpa_config config; >> unsigned long size = offsetof(struct vhost_vdpa_config, buf); >>+ long ret; >> u8 *buf; >> if (copy_from_user(&config, c, size)) >> return -EFAULT; >>- if (vhost_vdpa_config_validate(v, &config)) >>+ if (config.len == 0) >> return -EINVAL; >> buf = kvzalloc(config.len, GFP_KERNEL); > > >Then it means usersapce can allocate a very large memory. Good point. > >Rethink about this, we should limit the size here (e.g PAGE_SIZE) or >fetch the config size first (either through a config ops as you >suggested or a variable in the vdpa device that is initialized during >device creation). Maybe PAGE_SIZE is okay as a limit. If instead we want to fetch the config size, then better a config ops in my opinion, to avoid adding a new parameter to __vdpa_alloc_device(). I vote for PAGE_SIZE, but it isn't a strong opinion. What do you and @Michael suggest? Thanks, Stefano
On Fri, Feb 05, 2021 at 10:16:51AM +0100, Stefano Garzarella wrote: > On Fri, Feb 05, 2021 at 11:27:32AM +0800, Jason Wang wrote: > > > > On 2021/2/5 上午1:22, Stefano Garzarella wrote: > > > get_config() and set_config() callbacks in the 'struct vdpa_config_ops' > > > usually already validated the inputs. Also now they can return an error, > > > so we don't need to validate them here anymore. > > > > > > Let's use the return value of these callbacks and return it in case of > > > error in vhost_vdpa_get_config() and vhost_vdpa_set_config(). > > > > > > Originally-by: Xie Yongji <xieyongji@bytedance.com> > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > > > --- > > > drivers/vhost/vdpa.c | 41 +++++++++++++---------------------------- > > > 1 file changed, 13 insertions(+), 28 deletions(-) > > > > > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c > > > index ef688c8c0e0e..d61e779000a8 100644 > > > --- a/drivers/vhost/vdpa.c > > > +++ b/drivers/vhost/vdpa.c > > > @@ -185,51 +185,35 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp) > > > return 0; > > > } > > > -static int vhost_vdpa_config_validate(struct vhost_vdpa *v, > > > - struct vhost_vdpa_config *c) > > > -{ > > > - long size = 0; > > > - > > > - switch (v->virtio_id) { > > > - case VIRTIO_ID_NET: > > > - size = sizeof(struct virtio_net_config); > > > - break; > > > - } > > > - > > > - if (c->len == 0) > > > - return -EINVAL; > > > - > > > - if (c->len > size - c->off) > > > - return -E2BIG; > > > - > > > - return 0; > > > -} > > > - > > > static long vhost_vdpa_get_config(struct vhost_vdpa *v, > > > struct vhost_vdpa_config __user *c) > > > { > > > struct vdpa_device *vdpa = v->vdpa; > > > struct vhost_vdpa_config config; > > > unsigned long size = offsetof(struct vhost_vdpa_config, buf); > > > + long ret; > > > u8 *buf; > > > if (copy_from_user(&config, c, size)) > > > return -EFAULT; > > > - if (vhost_vdpa_config_validate(v, &config)) > > > + if (config.len == 0) > > > return -EINVAL; > > > buf = kvzalloc(config.len, GFP_KERNEL); > > > > > > Then it means usersapce can allocate a very large memory. > > Good point. > > > > > Rethink about this, we should limit the size here (e.g PAGE_SIZE) or > > fetch the config size first (either through a config ops as you > > suggested or a variable in the vdpa device that is initialized during > > device creation). > > Maybe PAGE_SIZE is okay as a limit. > > If instead we want to fetch the config size, then better a config ops in my > opinion, to avoid adding a new parameter to __vdpa_alloc_device(). > > I vote for PAGE_SIZE, but it isn't a strong opinion. > > What do you and @Michael suggest? > > Thanks, > Stefano Devices know what the config size is. Just have them provide it.
On Fri, Feb 05, 2021 at 08:32:37AM -0500, Michael S. Tsirkin wrote: >On Fri, Feb 05, 2021 at 10:16:51AM +0100, Stefano Garzarella wrote: >> On Fri, Feb 05, 2021 at 11:27:32AM +0800, Jason Wang wrote: >> > >> > On 2021/2/5 上午1:22, Stefano Garzarella wrote: >> > > get_config() and set_config() callbacks in the 'struct vdpa_config_ops' >> > > usually already validated the inputs. Also now they can return an error, >> > > so we don't need to validate them here anymore. >> > > >> > > Let's use the return value of these callbacks and return it in case of >> > > error in vhost_vdpa_get_config() and vhost_vdpa_set_config(). >> > > >> > > Originally-by: Xie Yongji <xieyongji@bytedance.com> >> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> > > --- >> > > drivers/vhost/vdpa.c | 41 +++++++++++++---------------------------- >> > > 1 file changed, 13 insertions(+), 28 deletions(-) >> > > >> > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c >> > > index ef688c8c0e0e..d61e779000a8 100644 >> > > --- a/drivers/vhost/vdpa.c >> > > +++ b/drivers/vhost/vdpa.c >> > > @@ -185,51 +185,35 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp) >> > > return 0; >> > > } >> > > -static int vhost_vdpa_config_validate(struct vhost_vdpa *v, >> > > - struct vhost_vdpa_config *c) >> > > -{ >> > > - long size = 0; >> > > - >> > > - switch (v->virtio_id) { >> > > - case VIRTIO_ID_NET: >> > > - size = sizeof(struct virtio_net_config); >> > > - break; >> > > - } >> > > - >> > > - if (c->len == 0) >> > > - return -EINVAL; >> > > - >> > > - if (c->len > size - c->off) >> > > - return -E2BIG; >> > > - >> > > - return 0; >> > > -} >> > > - >> > > static long vhost_vdpa_get_config(struct vhost_vdpa *v, >> > > struct vhost_vdpa_config __user *c) >> > > { >> > > struct vdpa_device *vdpa = v->vdpa; >> > > struct vhost_vdpa_config config; >> > > unsigned long size = offsetof(struct vhost_vdpa_config, buf); >> > > + long ret; >> > > u8 *buf; >> > > if (copy_from_user(&config, c, size)) >> > > return -EFAULT; >> > > - if (vhost_vdpa_config_validate(v, &config)) >> > > + if (config.len == 0) >> > > return -EINVAL; >> > > buf = kvzalloc(config.len, GFP_KERNEL); >> > >> > >> > Then it means usersapce can allocate a very large memory. >> >> Good point. >> >> > >> > Rethink about this, we should limit the size here (e.g PAGE_SIZE) or >> > fetch the config size first (either through a config ops as you >> > suggested or a variable in the vdpa device that is initialized during >> > device creation). >> >> Maybe PAGE_SIZE is okay as a limit. >> >> If instead we want to fetch the config size, then better a config ops in my >> opinion, to avoid adding a new parameter to __vdpa_alloc_device(). >> >> I vote for PAGE_SIZE, but it isn't a strong opinion. >> >> What do you and @Michael suggest? >> >> Thanks, >> Stefano > >Devices know what the config size is. Just have them provide it. > Okay, I'll add get_config_size() callback in vdpa_config_ops and I'll leave vhost_vdpa_config_validate() that will use that callback instead of 'virtio_id' to get the config size from the device. At this point I think I can remove the "vdpa: add return value to get_config/set_config callbacks" patch and leave void return to get_config/set_config callbacks. Does this make sense? Thanks, Stefano
On 2021/2/5 下午10:17, Stefano Garzarella wrote: > On Fri, Feb 05, 2021 at 08:32:37AM -0500, Michael S. Tsirkin wrote: >> On Fri, Feb 05, 2021 at 10:16:51AM +0100, Stefano Garzarella wrote: >>> On Fri, Feb 05, 2021 at 11:27:32AM +0800, Jason Wang wrote: >>> > >>> > On 2021/2/5 上午1:22, Stefano Garzarella wrote: >>> > > get_config() and set_config() callbacks in the 'struct >>> vdpa_config_ops' >>> > > usually already validated the inputs. Also now they can return >>> an error, >>> > > so we don't need to validate them here anymore. >>> > > >>> > > Let's use the return value of these callbacks and return it in >>> case of >>> > > error in vhost_vdpa_get_config() and vhost_vdpa_set_config(). >>> > > >>> > > Originally-by: Xie Yongji <xieyongji@bytedance.com> >>> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >>> > > --- >>> > > drivers/vhost/vdpa.c | 41 >>> +++++++++++++---------------------------- >>> > > 1 file changed, 13 insertions(+), 28 deletions(-) >>> > > >>> > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c >>> > > index ef688c8c0e0e..d61e779000a8 100644 >>> > > --- a/drivers/vhost/vdpa.c >>> > > +++ b/drivers/vhost/vdpa.c >>> > > @@ -185,51 +185,35 @@ static long vhost_vdpa_set_status(struct >>> vhost_vdpa *v, u8 __user *statusp) >>> > > return 0; >>> > > } >>> > > -static int vhost_vdpa_config_validate(struct vhost_vdpa *v, >>> > > - struct vhost_vdpa_config *c) >>> > > -{ >>> > > - long size = 0; >>> > > - >>> > > - switch (v->virtio_id) { >>> > > - case VIRTIO_ID_NET: >>> > > - size = sizeof(struct virtio_net_config); >>> > > - break; >>> > > - } >>> > > - >>> > > - if (c->len == 0) >>> > > - return -EINVAL; >>> > > - >>> > > - if (c->len > size - c->off) >>> > > - return -E2BIG; >>> > > - >>> > > - return 0; >>> > > -} >>> > > - >>> > > static long vhost_vdpa_get_config(struct vhost_vdpa *v, >>> > > struct vhost_vdpa_config __user *c) >>> > > { >>> > > struct vdpa_device *vdpa = v->vdpa; >>> > > struct vhost_vdpa_config config; >>> > > unsigned long size = offsetof(struct vhost_vdpa_config, buf); >>> > > + long ret; >>> > > u8 *buf; >>> > > if (copy_from_user(&config, c, size)) >>> > > return -EFAULT; >>> > > - if (vhost_vdpa_config_validate(v, &config)) >>> > > + if (config.len == 0) >>> > > return -EINVAL; >>> > > buf = kvzalloc(config.len, GFP_KERNEL); >>> > >>> > >>> > Then it means usersapce can allocate a very large memory. >>> >>> Good point. >>> >>> > >>> > Rethink about this, we should limit the size here (e.g PAGE_SIZE) or >>> > fetch the config size first (either through a config ops as you >>> > suggested or a variable in the vdpa device that is initialized during >>> > device creation). >>> >>> Maybe PAGE_SIZE is okay as a limit. >>> >>> If instead we want to fetch the config size, then better a config >>> ops in my >>> opinion, to avoid adding a new parameter to __vdpa_alloc_device(). >>> >>> I vote for PAGE_SIZE, but it isn't a strong opinion. >>> >>> What do you and @Michael suggest? >>> >>> Thanks, >>> Stefano >> >> Devices know what the config size is. Just have them provide it. >> > > Okay, I'll add get_config_size() callback in vdpa_config_ops and I'll > leave vhost_vdpa_config_validate() that will use that callback instead > of 'virtio_id' to get the config size from the device. > > At this point I think I can remove the "vdpa: add return value to > get_config/set_config callbacks" patch and leave void return to > get_config/set_config callbacks. > > Does this make sense? > > Thanks, > Stefano Yes I think so. Thanks
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index ef688c8c0e0e..d61e779000a8 100644 --- a/drivers/vhost/vdpa.c +++ b/drivers/vhost/vdpa.c @@ -185,51 +185,35 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp) return 0; } -static int vhost_vdpa_config_validate(struct vhost_vdpa *v, - struct vhost_vdpa_config *c) -{ - long size = 0; - - switch (v->virtio_id) { - case VIRTIO_ID_NET: - size = sizeof(struct virtio_net_config); - break; - } - - if (c->len == 0) - return -EINVAL; - - if (c->len > size - c->off) - return -E2BIG; - - return 0; -} - static long vhost_vdpa_get_config(struct vhost_vdpa *v, struct vhost_vdpa_config __user *c) { struct vdpa_device *vdpa = v->vdpa; struct vhost_vdpa_config config; unsigned long size = offsetof(struct vhost_vdpa_config, buf); + long ret; u8 *buf; if (copy_from_user(&config, c, size)) return -EFAULT; - if (vhost_vdpa_config_validate(v, &config)) + if (config.len == 0) return -EINVAL; buf = kvzalloc(config.len, GFP_KERNEL); if (!buf) return -ENOMEM; - vdpa_get_config(vdpa, config.off, buf, config.len); + ret = vdpa_get_config(vdpa, config.off, buf, config.len); + if (ret) + goto out; if (copy_to_user(c->buf, buf, config.len)) { - kvfree(buf); - return -EFAULT; + ret = -EFAULT; + goto out; } +out: kvfree(buf); - return 0; + return ret; } static long vhost_vdpa_set_config(struct vhost_vdpa *v, @@ -239,21 +223,22 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v, const struct vdpa_config_ops *ops = vdpa->config; struct vhost_vdpa_config config; unsigned long size = offsetof(struct vhost_vdpa_config, buf); + long ret; u8 *buf; if (copy_from_user(&config, c, size)) return -EFAULT; - if (vhost_vdpa_config_validate(v, &config)) + if (config.len == 0) return -EINVAL; buf = vmemdup_user(c->buf, config.len); if (IS_ERR(buf)) return PTR_ERR(buf); - ops->set_config(vdpa, config.off, buf, config.len); + ret = ops->set_config(vdpa, config.off, buf, config.len); kvfree(buf); - return 0; + return ret; } static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
get_config() and set_config() callbacks in the 'struct vdpa_config_ops' usually already validated the inputs. Also now they can return an error, so we don't need to validate them here anymore. Let's use the return value of these callbacks and return it in case of error in vhost_vdpa_get_config() and vhost_vdpa_set_config(). Originally-by: Xie Yongji <xieyongji@bytedance.com> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- drivers/vhost/vdpa.c | 41 +++++++++++++---------------------------- 1 file changed, 13 insertions(+), 28 deletions(-)