| Message ID | 2-v2-b6a5582525c9+ff96-vfio_reflck_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Provide core infrastructure for managing open/release | expand |
On Tue, 20 Jul 2021 14:42:48 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > Compared to mbochs_remove() two cases are missing from the > vfio_register_group_dev() unwind. Add them in. > > Fixes: 681c1615f891 ("vfio/mbochs: Convert to use vfio_register_group_dev()") > Reported-by: Cornelia Huck <cohuck@redhat.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > samples/vfio-mdev/mbochs.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c > index e81b875b4d87b4..501845b08c0974 100644 > --- a/samples/vfio-mdev/mbochs.c > +++ b/samples/vfio-mdev/mbochs.c > @@ -553,11 +553,14 @@ static int mbochs_probe(struct mdev_device *mdev) > > ret = vfio_register_group_dev(&mdev_state->vdev); > if (ret) > - goto err_mem; > + goto err_bytes; > dev_set_drvdata(&mdev->dev, mdev_state); > return 0; > > +err_bytes: > + mbochs_used_mbytes -= mdev_state->type->mbytes; > err_mem: > + kfree(mdev_state->pages); > kfree(mdev_state->vconfig); > kfree(mdev_state); > return ret; > @@ -567,8 +570,8 @@ static void mbochs_remove(struct mdev_device *mdev) > { > struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev); > > - mbochs_used_mbytes -= mdev_state->type->mbytes; > vfio_unregister_group_dev(&mdev_state->vdev); > + mbochs_used_mbytes -= mdev_state->type->mbytes; > kfree(mdev_state->pages); > kfree(mdev_state->vconfig); > kfree(mdev_state); Hmm, doesn't this suggest we need another atomic conversion? (untested) diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c index e81b875b4d87..842819e29c6b 100644 --- a/samples/vfio-mdev/mbochs.c +++ b/samples/vfio-mdev/mbochs.c @@ -129,7 +129,7 @@ static dev_t mbochs_devt; static struct class *mbochs_class; static struct cdev mbochs_cdev; static struct device mbochs_dev; -static int mbochs_used_mbytes; +static atomic_t mbochs_avail_mbytes; static const struct vfio_device_ops mbochs_dev_ops; struct vfio_region_info_ext { @@ -511,14 +511,19 @@ static int mbochs_probe(struct mdev_device *mdev) &mbochs_types[mdev_get_type_group_id(mdev)]; struct device *dev = mdev_dev(mdev); struct mdev_state *mdev_state; + int avail_mbytes = atomic_read(&mbochs_avail_mbytes); int ret = -ENOMEM; - if (type->mbytes + mbochs_used_mbytes > max_mbytes) - return -ENOMEM; + do { + if (avail_mbytes < type->mbytes) + return ret; + } while (!atomic_try_cmpxchg(&mbochs_avail_mbytes, &avail_mbytes, + avail_mbytes - type->mbytes)); mdev_state = kzalloc(sizeof(struct mdev_state), GFP_KERNEL); if (mdev_state == NULL) - return -ENOMEM; + goto err_resv; + vfio_init_group_dev(&mdev_state->vdev, &mdev->dev, &mbochs_dev_ops); mdev_state->vconfig = kzalloc(MBOCHS_CONFIG_SPACE_SIZE, GFP_KERNEL); @@ -549,8 +554,6 @@ static int mbochs_probe(struct mdev_device *mdev) mbochs_create_config_space(mdev_state); mbochs_reset(mdev_state); - mbochs_used_mbytes += type->mbytes; - ret = vfio_register_group_dev(&mdev_state->vdev); if (ret) goto err_mem; @@ -558,8 +561,11 @@ static int mbochs_probe(struct mdev_device *mdev) return 0; err_mem: + kfree(mdev_state->pages); kfree(mdev_state->vconfig); kfree(mdev_state); +err_resv: + atomic_add(mdev_state->type->mbytes, &mbochs_avail_mbytes); return ret; } @@ -567,11 +573,11 @@ static void mbochs_remove(struct mdev_device *mdev) { struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev); - mbochs_used_mbytes -= mdev_state->type->mbytes; vfio_unregister_group_dev(&mdev_state->vdev); kfree(mdev_state->pages); kfree(mdev_state->vconfig); kfree(mdev_state); + atomic_add(mdev_state->type->mbytes, &mbochs_avail_mbytes); } static ssize_t mbochs_read(struct vfio_device *vdev, char __user *buf, @@ -1351,7 +1357,7 @@ static ssize_t available_instances_show(struct mdev_type *mtype, { const struct mbochs_type *type = &mbochs_types[mtype_get_type_group_id(mtype)]; - int count = (max_mbytes - mbochs_used_mbytes) / type->mbytes; + int count = atomic_read(&mbochs_avail_mbytes) / type->mbytes; return sprintf(buf, "%d\n", count); } @@ -1460,6 +1466,8 @@ static int __init mbochs_dev_init(void) if (ret) goto err_class; + atomic_set(&mbochs_avail_mbytes, max_mbytes); + ret = mdev_register_device(&mbochs_dev, &mdev_fops); if (ret) goto err_device;
On Tue, Jul 20, 2021 at 04:01:27PM -0600, Alex Williamson wrote: > On Tue, 20 Jul 2021 14:42:48 -0300 > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > Compared to mbochs_remove() two cases are missing from the > > vfio_register_group_dev() unwind. Add them in. > > > > Fixes: 681c1615f891 ("vfio/mbochs: Convert to use vfio_register_group_dev()") > > Reported-by: Cornelia Huck <cohuck@redhat.com> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > samples/vfio-mdev/mbochs.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c > > index e81b875b4d87b4..501845b08c0974 100644 > > +++ b/samples/vfio-mdev/mbochs.c > > @@ -553,11 +553,14 @@ static int mbochs_probe(struct mdev_device *mdev) > > > > ret = vfio_register_group_dev(&mdev_state->vdev); > > if (ret) > > - goto err_mem; > > + goto err_bytes; > > dev_set_drvdata(&mdev->dev, mdev_state); > > return 0; > > > > +err_bytes: > > + mbochs_used_mbytes -= mdev_state->type->mbytes; > > err_mem: > > + kfree(mdev_state->pages); > > kfree(mdev_state->vconfig); > > kfree(mdev_state); > > return ret; > > @@ -567,8 +570,8 @@ static void mbochs_remove(struct mdev_device *mdev) > > { > > struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev); > > > > - mbochs_used_mbytes -= mdev_state->type->mbytes; > > vfio_unregister_group_dev(&mdev_state->vdev); > > + mbochs_used_mbytes -= mdev_state->type->mbytes; > > kfree(mdev_state->pages); > > kfree(mdev_state->vconfig); > > kfree(mdev_state); > > Hmm, doesn't this suggest we need another atomic conversion? (untested) Sure why not, I can add this as another patch > @@ -567,11 +573,11 @@ static void mbochs_remove(struct mdev_device *mdev) > { > struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev); > > - mbochs_used_mbytes -= mdev_state->type->mbytes; > vfio_unregister_group_dev(&mdev_state->vdev); > kfree(mdev_state->pages); > kfree(mdev_state->vconfig); > kfree(mdev_state); > + atomic_add(mdev_state->type->mbytes, &mbochs_avail_mbytes); This should be up after the vfio_unregister_group_dev(), it is a use after free? Jason
On Tue, 20 Jul 2021 19:49:55 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > On Tue, Jul 20, 2021 at 04:01:27PM -0600, Alex Williamson wrote: > > On Tue, 20 Jul 2021 14:42:48 -0300 > > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > > Compared to mbochs_remove() two cases are missing from the > > > vfio_register_group_dev() unwind. Add them in. > > > > > > Fixes: 681c1615f891 ("vfio/mbochs: Convert to use vfio_register_group_dev()") > > > Reported-by: Cornelia Huck <cohuck@redhat.com> > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > > samples/vfio-mdev/mbochs.c | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c > > > index e81b875b4d87b4..501845b08c0974 100644 > > > +++ b/samples/vfio-mdev/mbochs.c > > > @@ -553,11 +553,14 @@ static int mbochs_probe(struct mdev_device *mdev) > > > > > > ret = vfio_register_group_dev(&mdev_state->vdev); > > > if (ret) > > > - goto err_mem; > > > + goto err_bytes; > > > dev_set_drvdata(&mdev->dev, mdev_state); > > > return 0; > > > > > > +err_bytes: > > > + mbochs_used_mbytes -= mdev_state->type->mbytes; > > > err_mem: > > > + kfree(mdev_state->pages); > > > kfree(mdev_state->vconfig); > > > kfree(mdev_state); > > > return ret; > > > @@ -567,8 +570,8 @@ static void mbochs_remove(struct mdev_device *mdev) > > > { > > > struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev); > > > > > > - mbochs_used_mbytes -= mdev_state->type->mbytes; > > > vfio_unregister_group_dev(&mdev_state->vdev); > > > + mbochs_used_mbytes -= mdev_state->type->mbytes; > > > kfree(mdev_state->pages); > > > kfree(mdev_state->vconfig); > > > kfree(mdev_state); > > > > Hmm, doesn't this suggest we need another atomic conversion? (untested) > > Sure why not, I can add this as another patch > > > @@ -567,11 +573,11 @@ static void mbochs_remove(struct mdev_device *mdev) > > { > > struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev); > > > > - mbochs_used_mbytes -= mdev_state->type->mbytes; > > vfio_unregister_group_dev(&mdev_state->vdev); > > kfree(mdev_state->pages); > > kfree(mdev_state->vconfig); > > kfree(mdev_state); > > + atomic_add(mdev_state->type->mbytes, &mbochs_avail_mbytes); > > This should be up after the vfio_unregister_group_dev(), it is a use after free? Oops, yep. That or get the mbochs_type so we can mirror the _probe setup. Same on the _probe unwind, but we've already got type->mbytes there. Thanks, Alex
On Tue, Jul 20 2021, Jason Gunthorpe <jgg@nvidia.com> wrote: > Compared to mbochs_remove() two cases are missing from the > vfio_register_group_dev() unwind. Add them in. > > Fixes: 681c1615f891 ("vfio/mbochs: Convert to use vfio_register_group_dev()") > Reported-by: Cornelia Huck <cohuck@redhat.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > samples/vfio-mdev/mbochs.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On Tue, Jul 20 2021, Jason Gunthorpe <jgg@nvidia.com> wrote: > On Tue, Jul 20, 2021 at 04:01:27PM -0600, Alex Williamson wrote: >> Hmm, doesn't this suggest we need another atomic conversion? (untested) > > Sure why not, I can add this as another patch Yes, I think that should be another patch.
diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c index e81b875b4d87b4..501845b08c0974 100644 --- a/samples/vfio-mdev/mbochs.c +++ b/samples/vfio-mdev/mbochs.c @@ -553,11 +553,14 @@ static int mbochs_probe(struct mdev_device *mdev) ret = vfio_register_group_dev(&mdev_state->vdev); if (ret) - goto err_mem; + goto err_bytes; dev_set_drvdata(&mdev->dev, mdev_state); return 0; +err_bytes: + mbochs_used_mbytes -= mdev_state->type->mbytes; err_mem: + kfree(mdev_state->pages); kfree(mdev_state->vconfig); kfree(mdev_state); return ret; @@ -567,8 +570,8 @@ static void mbochs_remove(struct mdev_device *mdev) { struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev); - mbochs_used_mbytes -= mdev_state->type->mbytes; vfio_unregister_group_dev(&mdev_state->vdev); + mbochs_used_mbytes -= mdev_state->type->mbytes; kfree(mdev_state->pages); kfree(mdev_state->vconfig); kfree(mdev_state);
Compared to mbochs_remove() two cases are missing from the vfio_register_group_dev() unwind. Add them in. Fixes: 681c1615f891 ("vfio/mbochs: Convert to use vfio_register_group_dev()") Reported-by: Cornelia Huck <cohuck@redhat.com> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- samples/vfio-mdev/mbochs.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)