On Tue, May 05, 2015 at 06:36:00PM -0400, Benjamin Romer wrote:
> +int
> +devmajorminor_create_file(struct visor_device *dev, const char *name,
> + int major, int minor)
> +{
> + int maxdevnodes = ARRAY_SIZE(dev->devnodes) / sizeof(dev->devnodes[0]);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Nope. Also don't use a variable for this. Just use ARRAY_SIZE()
directly in the code.
> + struct devmajorminor_attribute *myattr = NULL;
> + int x = -1, rc = 0, slot = -1;
^
Why do we have an X coordinate without a Y coordinate?
Don't initialize variables for no reason.
> +
> + register_devmajorminor_attributes(dev);
> + for (slot = 0; slot < maxdevnodes; slot++)
> + if (!dev->devnodes[slot].attr)
> + break;
> + if (slot == maxdevnodes) {
> + rc = -ENOMEM;
> + goto away;
> + }
> + myattr = kmalloc(sizeof(*myattr), GFP_KERNEL);
> + if (!myattr) {
> + rc = -ENOMEM;
> + goto away;
> + }
> + memset(myattr, 0, sizeof(struct devmajorminor_attribute));
> + myattr->show = DEVMAJORMINOR_ATTR;
> + myattr->store = NULL;
> + myattr->slot = slot;
> + myattr->attr.name = name;
> + myattr->attr.mode = S_IRUGO;
> + dev->devnodes[slot].attr = myattr;
> + dev->devnodes[slot].major = major;
> + dev->devnodes[slot].minor = minor;
> + x = sysfs_create_file(&dev->kobjdevmajorminor, &myattr->attr);
> + if (x < 0) {
> + rc = x;
> + goto away;
> + }
> + kobject_uevent(&dev->device.kobj, KOBJ_ONLINE);
> +away:
> + if (rc < 0) {
> + kfree(myattr);
> + myattr = NULL;
> + dev->devnodes[slot].attr = NULL;
> + }
> + return rc;
> +}
When you only have one error label it causes One Err Bugs. These are
one of the most common type of error handling bugs in the linux kernel.
Some people don't like using direct returns because they think it means
that in the future people will forget to add error handling. I have
studied this a bit. If you look through the kernel git log for
bug where we return with a lock held bugs, then many of those functions
had an out: label already but people still just added a return. My
conclusion is that the kind of people who forget to add error handling
will mostly forget regardless of if there is an away: label or not.
The problem with jumbling all the error paths together into one label is
that it's a pile of spaghetti. But if you think about error handling
closer to where the error happens, it's easier to get it right.
Anyway, the bug here is that:
dev->devnodes[slot].attr = NULL;
is corrupting beyond the end of the array.
Also remove the "myattr = NULL;" since that is a pointless thing.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
