Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

chardev: add helper function to register char devs with a struct device

Credit for this patch goes is shared with Dan Williams [1]. I've
taken things one step further to make the helper function more
useful and clean up calling code.

There's a common pattern in the kernel whereby a struct cdev is placed
in a structure along side a struct device which manages the life-cycle
of both. In the naive approach, the reference counting is broken and
the struct device can free everything before the chardev code
is entirely released.

Many developers have solved this problem by linking the internal kobjs
in this fashion:

cdev.kobj.parent = &parent_dev.kobj;

The cdev code explicitly gets and puts a reference to it's kobj parent.
So this seems like it was intended to be used this way. Dmitrty Torokhov
first put this in place in 2012 with this commit:

2f0157f char_dev: pin parent kobject

and the first instance of the fix was then done in the input subsystem
in the following commit:

4a215aa Input: fix use-after-free introduced with dynamic minor changes

Subsequently over the years, however, this issue seems to have tripped
up multiple developers independently. For example, see these commits:

0d5b7da iio: Prevent race between IIO chardev opening and IIO device
(by Lars-Peter Clausen in 2013)

ba0ef85 tpm: Fix initialization of the cdev
(by Jason Gunthorpe in 2015)

5b28dde [media] media: fix use-after-free in cdev_put() when app exits
after driver unbind
(by Shauh Khan in 2016)

This technique is similarly done in at least 15 places within the kernel
and probably should have been done so in another, at least, 5 places.
The kobj line also looks very suspect in that one would not expect
drivers to have to mess with kobject internals in this way.
Even highly experienced kernel developers can be surprised by this
code, as seen in [2].

To help alleviate this situation, and hopefully prevent future
wasted effort on this problem, this patch introduces a helper function
to register a char device along with its parent struct device.
This creates a more regular API for tying a char device to its parent
without the developer having to set members in the underlying kobject.

This patch introduce cdev_device_add and cdev_device_del which
replaces a common pattern including setting the kobj parent, calling
cdev_add and then calling device_add. It also introduces cdev_set_parent
for the few cases that set the kobject parent without using device_add.

[1] https://lkml.org/lkml/2017/2/13/700
[2] https://lkml.org/lkml/2017/2/10/370

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

authored by

Logan Gunthorpe and committed by
Greg Kroah-Hartman
233ed09d d47d8836

+91
+86
fs/char_dev.c
··· 471 471 return 0; 472 472 } 473 473 474 + /** 475 + * cdev_set_parent() - set the parent kobject for a char device 476 + * @p: the cdev structure 477 + * @kobj: the kobject to take a reference to 478 + * 479 + * cdev_set_parent() sets a parent kobject which will be referenced 480 + * appropriately so the parent is not freed before the cdev. This 481 + * should be called before cdev_add. 482 + */ 483 + void cdev_set_parent(struct cdev *p, struct kobject *kobj) 484 + { 485 + WARN_ON(!kobj->state_initialized); 486 + p->kobj.parent = kobj; 487 + } 488 + 489 + /** 490 + * cdev_device_add() - add a char device and it's corresponding 491 + * struct device, linkink 492 + * @dev: the device structure 493 + * @cdev: the cdev structure 494 + * 495 + * cdev_device_add() adds the char device represented by @cdev to the system, 496 + * just as cdev_add does. It then adds @dev to the system using device_add 497 + * The dev_t for the char device will be taken from the struct device which 498 + * needs to be initialized first. This helper function correctly takes a 499 + * reference to the parent device so the parent will not get released until 500 + * all references to the cdev are released. 501 + * 502 + * This helper uses dev->devt for the device number. If it is not set 503 + * it will not add the cdev and it will be equivalent to device_add. 504 + * 505 + * This function should be used whenever the struct cdev and the 506 + * struct device are members of the same structure whose lifetime is 507 + * managed by the struct device. 508 + * 509 + * NOTE: Callers must assume that userspace was able to open the cdev and 510 + * can call cdev fops callbacks at any time, even if this function fails. 511 + */ 512 + int cdev_device_add(struct cdev *cdev, struct device *dev) 513 + { 514 + int rc = 0; 515 + 516 + if (dev->devt) { 517 + cdev_set_parent(cdev, &dev->kobj); 518 + 519 + rc = cdev_add(cdev, dev->devt, 1); 520 + if (rc) 521 + return rc; 522 + } 523 + 524 + rc = device_add(dev); 525 + if (rc) 526 + cdev_del(cdev); 527 + 528 + return rc; 529 + } 530 + 531 + /** 532 + * cdev_device_del() - inverse of cdev_device_add 533 + * @dev: the device structure 534 + * @cdev: the cdev structure 535 + * 536 + * cdev_device_del() is a helper function to call cdev_del and device_del. 537 + * It should be used whenever cdev_device_add is used. 538 + * 539 + * If dev->devt is not set it will not remove the cdev and will be equivalent 540 + * to device_del. 541 + * 542 + * NOTE: This guarantees that associated sysfs callbacks are not running 543 + * or runnable, however any cdevs already open will remain and their fops 544 + * will still be callable even after this function returns. 545 + */ 546 + void cdev_device_del(struct cdev *cdev, struct device *dev) 547 + { 548 + device_del(dev); 549 + if (dev->devt) 550 + cdev_del(cdev); 551 + } 552 + 474 553 static void cdev_unmap(dev_t dev, unsigned count) 475 554 { 476 555 kobj_unmap(cdev_map, dev, count); ··· 561 482 * 562 483 * cdev_del() removes @p from the system, possibly freeing the structure 563 484 * itself. 485 + * 486 + * NOTE: This guarantees that cdev device will no longer be able to be 487 + * opened, however any cdevs already open will remain and their fops will 488 + * still be callable even after cdev_del returns. 564 489 */ 565 490 void cdev_del(struct cdev *p) 566 491 { ··· 653 570 EXPORT_SYMBOL(cdev_alloc); 654 571 EXPORT_SYMBOL(cdev_del); 655 572 EXPORT_SYMBOL(cdev_add); 573 + EXPORT_SYMBOL(cdev_set_parent); 574 + EXPORT_SYMBOL(cdev_device_add); 575 + EXPORT_SYMBOL(cdev_device_del); 656 576 EXPORT_SYMBOL(__register_chrdev); 657 577 EXPORT_SYMBOL(__unregister_chrdev);
+5
include/linux/cdev.h
··· 4 4 #include <linux/kobject.h> 5 5 #include <linux/kdev_t.h> 6 6 #include <linux/list.h> 7 + #include <linux/device.h> 7 8 8 9 struct file_operations; 9 10 struct inode; ··· 26 25 void cdev_put(struct cdev *p); 27 26 28 27 int cdev_add(struct cdev *, dev_t, unsigned); 28 + 29 + void cdev_set_parent(struct cdev *p, struct kobject *kobj); 30 + int cdev_device_add(struct cdev *cdev, struct device *dev); 31 + void cdev_device_del(struct cdev *cdev, struct device *dev); 29 32 30 33 void cdev_del(struct cdev *); 31 34