Re: [bug report] dax: refactor dax-fs into a generic provider of 'struct dax_device' instances

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jul 11, 2017 at 12:49 AM, Dan Carpenter
<dan.carpenter@xxxxxxxxxx> wrote:
> Hello Dan Williams,
>
> The patch 7b6be8444e0f: "dax: refactor dax-fs into a generic provider
> of 'struct dax_device' instances" from Apr 11, 2017, leads to the
> following static checker warning:
>
>         drivers/dax/device.c:643 devm_create_dev_dax()
>         warn: passing zero to 'ERR_PTR'

Yes, there's a couple things to clean up here:

>
> drivers/dax/device.c
>    561  struct dev_dax *devm_create_dev_dax(struct dax_region *dax_region,
>    562                  struct resource *res, int count)
>    563  {
>    564          struct device *parent = dax_region->dev;
>    565          struct dax_device *dax_dev;
>    566          struct dev_dax *dev_dax;
>    567          struct inode *inode;
>    568          struct device *dev;
>    569          struct cdev *cdev;
>    570          int rc = 0, i;
>                     ^^^^^^
> This makes me think it's deliberate.

I think it would be fine to make this default to -ENOMEM.

>
>    571
>    572          dev_dax = kzalloc(sizeof(*dev_dax) + sizeof(*res) * count, GFP_KERNEL);
>    573          if (!dev_dax)
>    574                  return ERR_PTR(-ENOMEM);
>    575
>    576          for (i = 0; i < count; i++) {
>    577                  if (!IS_ALIGNED(res[i].start, dax_region->align)
>    578                                  || !IS_ALIGNED(resource_size(&res[i]),
>    579                                          dax_region->align)) {
>    580                          rc = -EINVAL;
>    581                          break;
>    582                  }
>    583                  dev_dax->res[i].start = res[i].start;
>    584                  dev_dax->res[i].end = res[i].end;
>    585          }
>    586
>    587          if (i < count)
>    588                  goto err_id;
>
> We return NULL here.  Probably that's intended?

No, I would expect if 'i' is less than count rc is -EINVAL. Hmm,
unless count is 0 which it can't be, but the static analyzer would
need to audit all callers to find that out.

>
>    589
>    590          dev_dax->id = ida_simple_get(&dax_region->ida, 0, 0, GFP_KERNEL);
>    591          if (dev_dax->id < 0) {
>    592                  rc = dev_dax->id;
>    593                  goto err_id;
>    594          }
>    595
>    596          /*
>    597           * No 'host' or dax_operations since there is no access to this
>    598           * device outside of mmap of the resulting character device.
>    599           */
>    600          dax_dev = alloc_dax(dev_dax, NULL, NULL);
>    601          if (!dax_dev)
>    602                  goto err_dax;
>
> There is a comment in front of this allocation, but allocation failures
> almost always mean we return an error...  I'm not sure.  The caller
> treats this path as success.  I don't see an immediate problem with
> that.

This should be a -ENOMEM failure.

>
>    603
>    604          /* from here on we're committed to teardown via dax_dev_release() */
>    605          dev = &dev_dax->dev;
>    606          device_initialize(dev);
>    607
>    608          inode = dax_inode(dax_dev);
>    609          cdev = inode->i_cdev;
>    610          cdev_init(cdev, &dax_fops);
>    611          cdev->owner = parent->driver->owner;
>    612
>    613          dev_dax->num_resources = count;
>    614          dev_dax->dax_dev = dax_dev;
>    615          dev_dax->region = dax_region;
>    616          kref_get(&dax_region->kref);
>    617
>    618          dev->devt = inode->i_rdev;
>    619          dev->class = dax_class;
>    620          dev->parent = parent;
>    621          dev->groups = dax_attribute_groups;
>    622          dev->release = dev_dax_release;
>    623          dev_set_name(dev, "dax%d.%d", dax_region->id, dev_dax->id);
>    624
>    625          rc = cdev_device_add(cdev, dev);
>    626          if (rc) {
>    627                  kill_dev_dax(dev_dax);
>    628                  put_device(dev);
>    629                  return ERR_PTR(rc);
>    630          }
>    631
>    632          rc = devm_add_action_or_reset(dax_region->dev, unregister_dev_dax, dev);
>    633          if (rc)
>    634                  return ERR_PTR(rc);
>
> It really feels like there should be more error handling here...

unregister_dev_dax() is called if devm_add_action_or_reset() fails,
does that satisfy your concern?

>
>    635
>    636          return dev_dax;
>    637
>    638   err_dax:
>    639          ida_simple_remove(&dax_region->ida, dev_dax->id);
>    640   err_id:
>    641          kfree(dev_dax);
>    642
>    643          return ERR_PTR(rc);
>    644  }
>
> regards,
> dan carpenter

Thanks for the report!
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux