| Message ID | 2-v3-6c9e19cc7d44+15613-vfio_reflck_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Provide core infrastructure for managing open/release | expand |
Hi Jason,
url: https://github.com/0day-ci/linux/commits/Jason-Gunthorpe/Provide-core-infrastructure-for-managing-open-release/20210729-085124
base: https://github.com/awilliam/linux-vfio.git next
config: x86_64-randconfig-m001-20210728 (attached as .config)
compiler: gcc-10 (Ubuntu 10.3.0-1ubuntu1~20.04) 10.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
samples/vfio-mdev/mbochs.c:566 mbochs_probe() error: we previously assumed 'mdev_state' could be null (see line 524)
samples/vfio-mdev/mbochs.c:566 mbochs_probe() error: dereferencing freed memory 'mdev_state'
vim +/mdev_state +566 samples/vfio-mdev/mbochs.c
681c1615f89144 Jason Gunthorpe 2021-06-17 508 static int mbochs_probe(struct mdev_device *mdev)
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 509 {
909fe1e3ec15f4 Jason Gunthorpe 2021-07-28 510 int avail_mbytes = atomic_read(&mbochs_avail_mbytes);
3d3a360e570616 Jason Gunthorpe 2021-04-06 511 const struct mbochs_type *type =
3d3a360e570616 Jason Gunthorpe 2021-04-06 512 &mbochs_types[mdev_get_type_group_id(mdev)];
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 513 struct device *dev = mdev_dev(mdev);
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 514 struct mdev_state *mdev_state;
681c1615f89144 Jason Gunthorpe 2021-06-17 515 int ret = -ENOMEM;
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 516
909fe1e3ec15f4 Jason Gunthorpe 2021-07-28 517 do {
909fe1e3ec15f4 Jason Gunthorpe 2021-07-28 518 if (avail_mbytes < type->mbytes)
909fe1e3ec15f4 Jason Gunthorpe 2021-07-28 519 return -ENOSPC;
909fe1e3ec15f4 Jason Gunthorpe 2021-07-28 520 } while (!atomic_try_cmpxchg(&mbochs_avail_mbytes, &avail_mbytes,
909fe1e3ec15f4 Jason Gunthorpe 2021-07-28 521 avail_mbytes - type->mbytes));
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 522
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 523 mdev_state = kzalloc(sizeof(struct mdev_state), GFP_KERNEL);
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 @524 if (mdev_state == NULL)
909fe1e3ec15f4 Jason Gunthorpe 2021-07-28 525 goto err_avail;
This goto leads to a NULL deref
681c1615f89144 Jason Gunthorpe 2021-06-17 526 vfio_init_group_dev(&mdev_state->vdev, &mdev->dev, &mbochs_dev_ops);
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 527
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 528 mdev_state->vconfig = kzalloc(MBOCHS_CONFIG_SPACE_SIZE, GFP_KERNEL);
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 529 if (mdev_state->vconfig == NULL)
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 530 goto err_mem;
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 531
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 532 mdev_state->memsize = type->mbytes * 1024 * 1024;
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 533 mdev_state->pagecount = mdev_state->memsize >> PAGE_SHIFT;
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 534 mdev_state->pages = kcalloc(mdev_state->pagecount,
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 535 sizeof(struct page *),
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 536 GFP_KERNEL);
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 537 if (!mdev_state->pages)
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 538 goto err_mem;
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 539
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 540 dev_info(dev, "%s: %s, %d MB, %ld pages\n", __func__,
3d3a360e570616 Jason Gunthorpe 2021-04-06 541 type->name, type->mbytes, mdev_state->pagecount);
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 542
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 543 mutex_init(&mdev_state->ops_lock);
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 544 mdev_state->mdev = mdev;
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 545 INIT_LIST_HEAD(&mdev_state->dmabufs);
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 546 mdev_state->next_id = 1;
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 547
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 548 mdev_state->type = type;
104c7405a64d93 Gerd Hoffmann 2018-09-21 549 mdev_state->edid_regs.max_xres = type->max_x;
104c7405a64d93 Gerd Hoffmann 2018-09-21 550 mdev_state->edid_regs.max_yres = type->max_y;
104c7405a64d93 Gerd Hoffmann 2018-09-21 551 mdev_state->edid_regs.edid_offset = MBOCHS_EDID_BLOB_OFFSET;
104c7405a64d93 Gerd Hoffmann 2018-09-21 552 mdev_state->edid_regs.edid_max_size = sizeof(mdev_state->edid_blob);
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 553 mbochs_create_config_space(mdev_state);
681c1615f89144 Jason Gunthorpe 2021-06-17 554 mbochs_reset(mdev_state);
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 555
681c1615f89144 Jason Gunthorpe 2021-06-17 556 ret = vfio_register_group_dev(&mdev_state->vdev);
681c1615f89144 Jason Gunthorpe 2021-06-17 557 if (ret)
681c1615f89144 Jason Gunthorpe 2021-06-17 558 goto err_mem;
681c1615f89144 Jason Gunthorpe 2021-06-17 559 dev_set_drvdata(&mdev->dev, mdev_state);
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 560 return 0;
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 561 err_mem:
909fe1e3ec15f4 Jason Gunthorpe 2021-07-28 562 kfree(mdev_state->pages);
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 563 kfree(mdev_state->vconfig);
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 564 kfree(mdev_state);
^^^^^^^^^^
Freed
909fe1e3ec15f4 Jason Gunthorpe 2021-07-28 565 err_avail:
909fe1e3ec15f4 Jason Gunthorpe 2021-07-28 @566 atomic_add(mdev_state->type->mbytes, &mbochs_avail_mbytes);
^^^^^^^^^^
This should just be:
atomic_add(type->mbytes, &mbochs_avail_mbytes);
681c1615f89144 Jason Gunthorpe 2021-06-17 567 return ret;
a5e6e6505f38f7 Gerd Hoffmann 2018-05-11 568 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Thu, Jul 29, 2021 at 12:38:12PM +0300, Dan Carpenter wrote: > This should just be: > atomic_add(type->mbytes, &mbochs_avail_mbytes); Arg, yes, thanks Dan - I thought I got all of these. Jason
diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c index e81b875b4d87b4..a12b1fc94ed402 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 { @@ -507,18 +507,22 @@ static int mbochs_reset(struct mdev_state *mdev_state) static int mbochs_probe(struct mdev_device *mdev) { + int avail_mbytes = atomic_read(&mbochs_avail_mbytes); const struct mbochs_type *type = &mbochs_types[mdev_get_type_group_id(mdev)]; struct device *dev = mdev_dev(mdev); struct mdev_state *mdev_state; int ret = -ENOMEM; - if (type->mbytes + mbochs_used_mbytes > max_mbytes) - return -ENOMEM; + do { + if (avail_mbytes < type->mbytes) + return -ENOSPC; + } 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_avail; vfio_init_group_dev(&mdev_state->vdev, &mdev->dev, &mbochs_dev_ops); mdev_state->vconfig = kzalloc(MBOCHS_CONFIG_SPACE_SIZE, GFP_KERNEL); @@ -549,17 +553,17 @@ 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; dev_set_drvdata(&mdev->dev, mdev_state); return 0; - err_mem: + kfree(mdev_state->pages); kfree(mdev_state->vconfig); kfree(mdev_state); +err_avail: + atomic_add(mdev_state->type->mbytes, &mbochs_avail_mbytes); return ret; } @@ -567,8 +571,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); + atomic_add(mdev_state->type->mbytes, &mbochs_avail_mbytes); kfree(mdev_state->pages); kfree(mdev_state->vconfig); kfree(mdev_state); @@ -1351,7 +1355,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); } @@ -1433,6 +1437,8 @@ static int __init mbochs_dev_init(void) { int ret = 0; + atomic_set(&mbochs_avail_mbytes, max_mbytes); + ret = alloc_chrdev_region(&mbochs_devt, 0, MINORMASK + 1, MBOCHS_NAME); if (ret < 0) { pr_err("Error: failed to register mbochs_dev, err: %d\n", ret);
Convert mbochs to use an atomic scheme for this like mtty was changed into. The atomic fixes various race conditions with probing. Add the missing error unwind. Also add the missing kfree of mdev_state->pages. Fixes: 681c1615f891 ("vfio/mbochs: Convert to use vfio_register_group_dev()") Reported-by: Cornelia Huck <cohuck@redhat.com> Co-developed-by: Alex Williamson <alex.williamson@redhat.com> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- samples/vfio-mdev/mbochs.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)