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
