[bug report] scsi: mpi3mr: Add bsg device support

[bug report] scsi: mpi3mr: Add bsg device support

[Dan Carpenter]

Hello Sumit Saxena,

The patch 4268fa751365: "scsi: mpi3mr: Add bsg device support" from
Apr 29, 2022, leads to the following Smatch static checker warning:

	drivers/scsi/mpi3mr/mpi3mr_app.c:1539 mpi3mr_bsg_init()
	warn: double put_device() 'mrioc->bsg_dev->kobj' (see line 1539)

[ This warning is from development versions of Smatch that I have not
  published yet ]

drivers/scsi/mpi3mr/mpi3mr_app.c
  1480  /**
  1481   * mpi3mr_bsg_exit - de-registration from bsg layer
  1482   *
  1483   * This will be called during driver unload and all
  1484   * bsg resources allocated during load will be freed.
  1485   *
  1486   * Return:Nothing
  1487   */
  1488  void mpi3mr_bsg_exit(struct mpi3mr_ioc *mrioc)
  1489  {
  1490          if (!mrioc->bsg_queue)
  1491                  return;
  1492  
  1493          bsg_remove_queue(mrioc->bsg_queue);
  1494          mrioc->bsg_queue = NULL;
  1495  
  1496          device_del(mrioc->bsg_dev);
  1497          put_device(mrioc->bsg_dev);
  1498          kfree(mrioc->bsg_dev);
  1499  }

I'm pretty sure this code is buggy.  The put_device() has a config
option which adds a delay to calling the release() function
CONFIG_DEBUG_KOBJECT_RELEASE.  So if you enable the delay, I think it
will dereference "mrioc->bsg_dev" after it is freed.

[ snip ]

  1501  /**
  1502   * mpi3mr_bsg_node_release -release bsg device node
  1503   * @dev: bsg device node
  1504   *
  1505   * decrements bsg dev reference count
  1506   *
  1507   * Return:Nothing
  1508   */
  1509  static void mpi3mr_bsg_node_release(struct device *dev)
  1510  {
  1511          put_device(dev);
  1512  }


Normally, the device struct would just be embedded into the
mrioc struct.  (Not a pointer).  So this function would normally be
something like:

	struct mpi3mr_ioc *mrioc = container_of(dev, ...);

	kfree(mrioc);

But here it's a separate pointer.  So I guess it should just be:

	kfree(dev);

[ snip ]

    1522 void mpi3mr_bsg_init(struct mpi3mr_ioc *mrioc)
    1523 {
    1524         mrioc->bsg_dev = kzalloc(sizeof(struct device), GFP_KERNEL);
    1525         if (!mrioc->bsg_dev) {
    1526                 ioc_err(mrioc, "bsg device mem allocation failed\n");
    1527                 return;
    1528         }
    1529 
    1530         device_initialize(mrioc->bsg_dev);
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The device model is very tricky and it confuses me too.

After you do a device initialize, the you must free mrioc->bsg_dev
using put_device().  Calling kfree(mrioc->bsg_dev) is a bug.  I'm not
sure all the implications but at a minimum it's a small memory leak.

But we haven't set up a ->release function at this point so that's
another thing.

    1531         dev_set_name(mrioc->bsg_dev, "mpi3mrctl%u", mrioc->id);
    1532 
    1533         if (device_add(mrioc->bsg_dev)) {
    1534                 ioc_err(mrioc, "%s: bsg device add failed\n",
    1535                     dev_name(mrioc->bsg_dev));
    1536                 goto err_device_add;
    1537         }
    1538 
--> 1539         mrioc->bsg_dev->release = mpi3mr_bsg_node_release;
    1540 
    1541         mrioc->bsg_queue = bsg_setup_queue(mrioc->bsg_dev, dev_name(mrioc->bsg_dev),
    1542                         mpi3mr_bsg_request, NULL, 0);
    1543         if (!mrioc->bsg_queue) {
    1544                 ioc_err(mrioc, "%s: bsg registration failed\n",
    1545                     dev_name(mrioc->bsg_dev));
    1546                 goto err_setup_queue;
    1547         }
    1548 
    1549         blk_queue_max_segments(mrioc->bsg_queue, MPI3MR_MAX_APP_XFER_SEGMENTS);
    1550         blk_queue_max_hw_sectors(mrioc->bsg_queue, MPI3MR_MAX_APP_XFER_SECTORS);
    1551 
    1552         return;
    1553 
    1554 err_setup_queue:
    1555         device_del(mrioc->bsg_dev);
    1556         put_device(mrioc->bsg_dev);

Smatch is saying that this put_device() will trigger a call to
mpi3mr_bsg_node_release() which will make the refcount negative.  That
seems like a reasonable warning.

    1557 err_device_add:
    1558         kfree(mrioc->bsg_dev);
    1559 }

regards,
dan carpenter