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'
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.
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?
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.
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...
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
--
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
