Re: [PATCH v2 02/14] vfio/mbochs: Fix missing error unwind in mbochs_probe()

Alex Williamson
2021-07-20T22:02:16Z

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;