| Message ID | 1488091097-12328-2-git-send-email-logang@deltatee.com |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, Feb 25, 2017 at 10:38 PM, Logan Gunthorpe <logang@deltatee.com> wrote: > 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]. Nice history! > > 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> > --- > fs/char_dev.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/cdev.h | 4 ++++ > 2 files changed, 71 insertions(+) > > diff --git a/fs/char_dev.c b/fs/char_dev.c > index 44a240c..471d480 100644 > --- a/fs/char_dev.c > +++ b/fs/char_dev.c > @@ -471,6 +471,70 @@ int cdev_add(struct cdev *p, dev_t dev, unsigned count) > return 0; > } > > +/** > + * cdev_set_parent() - set the parent kobject for a char device > + * @p: the cdev structure > + * @kobj: the kobject to take a reference to > + * > + * cdev_set_parent() sets a parent kobject which will be referenced > + * appropriately so the parent is not freed before the cdev. This > + * should be called before cdev_add. > + */ > +void cdev_set_parent(struct cdev *p, struct kobject *kobj) > +{ > + WARN_ON(!kobj->state_initialized); > + p->kobj.parent = kobj; > +} > + > +/** > + * cdev_device_add() - add a char device and it's corresponding > + * struct device, linkink "add a cdev and it's corresponding struct device" > + * @dev: the device structure > + * @cdev: the cdev structure > + * > + * cdev_device_add() adds the char device represented by @cdev to the system, > + * just as cdev_add does. It then adds @dev to the system using device_add > + * The dev_t for the char device will be taken from the struct device which > + * needs to be initialized first. This helper function correctly takes a > + * reference to the parent device so the parent will not get released until > + * all references to the cdev are released. > + * > + * This function should be used whenever the struct cdev and the > + * struct device are members of the same structure whose lifetime is > + * managed by the struct device. > + */ Perhaps add a note here that userspace may have invoked file operations between cdev_add() and a failing device_add(), so additional cleanup beyond put_device() (like mmap invalidation) might be needed. That can be a later follow-on patch. > +int cdev_device_add(struct cdev *cdev, struct device *dev) > +{ > + int rc = 0; > + > + cdev_set_parent(cdev, &dev->kobj); > + > + rc = cdev_add(cdev, dev->devt, 1); > + if (rc) > + return rc; > + > + rc = device_add(dev); > + if (rc) > + cdev_del(cdev); > + > + return rc; > +} > +
On 02/26/2017 07:38 AM, Logan Gunthorpe wrote: > 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> Nice! Regards, Hans > --- > fs/char_dev.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/cdev.h | 4 ++++ > 2 files changed, 71 insertions(+) > > diff --git a/fs/char_dev.c b/fs/char_dev.c > index 44a240c..471d480 100644 > --- a/fs/char_dev.c > +++ b/fs/char_dev.c > @@ -471,6 +471,70 @@ int cdev_add(struct cdev *p, dev_t dev, unsigned count) > return 0; > } > > +/** > + * cdev_set_parent() - set the parent kobject for a char device > + * @p: the cdev structure > + * @kobj: the kobject to take a reference to > + * > + * cdev_set_parent() sets a parent kobject which will be referenced > + * appropriately so the parent is not freed before the cdev. This > + * should be called before cdev_add. > + */ > +void cdev_set_parent(struct cdev *p, struct kobject *kobj) > +{ > + WARN_ON(!kobj->state_initialized); > + p->kobj.parent = kobj; > +} > + > +/** > + * cdev_device_add() - add a char device and it's corresponding > + * struct device, linkink > + * @dev: the device structure > + * @cdev: the cdev structure > + * > + * cdev_device_add() adds the char device represented by @cdev to the system, > + * just as cdev_add does. It then adds @dev to the system using device_add > + * The dev_t for the char device will be taken from the struct device which > + * needs to be initialized first. This helper function correctly takes a > + * reference to the parent device so the parent will not get released until > + * all references to the cdev are released. > + * > + * This function should be used whenever the struct cdev and the > + * struct device are members of the same structure whose lifetime is > + * managed by the struct device. > + */ > +int cdev_device_add(struct cdev *cdev, struct device *dev) > +{ > + int rc = 0; > + > + cdev_set_parent(cdev, &dev->kobj); > + > + rc = cdev_add(cdev, dev->devt, 1); > + if (rc) > + return rc; > + > + rc = device_add(dev); > + if (rc) > + cdev_del(cdev); > + > + return rc; > +} > + > +/** > + * cdev_device_del() - inverse of cdev_device_add > + * @dev: the device structure > + * @cdev: the cdev structure > + * > + * cdev_device_del() is a helper function to call cdev_del > + * and device_del. It should be used whenever cdev_device_add > + * is used. > + */ > +void cdev_device_del(struct cdev *cdev, struct device *dev) > +{ > + device_del(dev); > + cdev_del(cdev); > +} > + > static void cdev_unmap(dev_t dev, unsigned count) > { > kobj_unmap(cdev_map, dev, count); > @@ -570,5 +634,8 @@ EXPORT_SYMBOL(cdev_init); > EXPORT_SYMBOL(cdev_alloc); > EXPORT_SYMBOL(cdev_del); > EXPORT_SYMBOL(cdev_add); > +EXPORT_SYMBOL(cdev_set_parent); > +EXPORT_SYMBOL(cdev_device_add); > +EXPORT_SYMBOL(cdev_device_del); > EXPORT_SYMBOL(__register_chrdev); > EXPORT_SYMBOL(__unregister_chrdev); > diff --git a/include/linux/cdev.h b/include/linux/cdev.h > index f876361..4c99579 100644 > --- a/include/linux/cdev.h > +++ b/include/linux/cdev.h > @@ -26,6 +26,10 @@ void cdev_put(struct cdev *p); > > int cdev_add(struct cdev *, dev_t, unsigned); > > +void cdev_set_parent(struct cdev *p, struct kobject *kobj); > +int cdev_device_add(struct cdev *cdev, struct device *dev); > +void cdev_device_del(struct cdev *cdev, struct device *dev); > + > void cdev_del(struct cdev *); > > void cd_forget(struct inode *); >
On 25/02/2017 at 23:38:02 -0700, Logan Gunthorpe wrote: > 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: Alexandre Belloni <alexandre.belloni@free-electrons.com>
On Sun, Feb 26, 2017 at 10:21:25AM -0800, Dan Williams wrote: > > + * cdev_device_add() adds the char device represented by @cdev to the system, > > + * just as cdev_add does. It then adds @dev to the system using device_add > > + * The dev_t for the char device will be taken from the struct device which > > + * needs to be initialized first. This helper function correctly takes a > > + * reference to the parent device so the parent will not get released until > > + * all references to the cdev are released. > > + * > > + * This function should be used whenever the struct cdev and the > > + * struct device are members of the same structure whose lifetime is > > + * managed by the struct device. > > + */ > > Perhaps add a note here that userspace may have invoked file > operations between cdev_add() and a failing device_add(), so > additional cleanup beyond put_device() (like mmap invalidation) might > be needed. That can be a later follow-on patch. Yes please, that is way too subtle. Suggest: NOTE: Callers must assume that userspace was able to open the cdev and can call cdev fops callbacks at any time, even if this function fails. I would also add a note to cdev_device_del: NOTE: This guarantees that associated sysfs callbacks are not running or runnable, however any open cdevs will remain and their fops remain callable even after this returns. Since I have seen a lot of confusion on that point as well.. Jason
On 27/02/17 09:56 AM, Jason Gunthorpe wrote: > Yes please, that is way too subtle. Suggest: > > NOTE: Callers must assume that userspace was able to open the cdev and > can call cdev fops callbacks at any time, even if this function fails. > > I would also add a note to cdev_device_del: > > NOTE: This guarantees that associated sysfs callbacks are not running > or runnable, however any open cdevs will remain and their fops remain > callable even after this returns. > > Since I have seen a lot of confusion on that point as well.. Sure, I'll add that verbiage to a v3 sometime after rc1. Logan
On Mon, Feb 27, 2017 at 7:27 PM, Logan Gunthorpe <logang@deltatee.com> wrote: > > > On 27/02/17 09:56 AM, Jason Gunthorpe wrote: >> Yes please, that is way too subtle. Suggest: >> >> NOTE: Callers must assume that userspace was able to open the cdev and >> can call cdev fops callbacks at any time, even if this function fails. >> >> I would also add a note to cdev_device_del: >> >> NOTE: This guarantees that associated sysfs callbacks are not running >> or runnable, however any open cdevs will remain and their fops remain >> callable even after this returns. >> >> Since I have seen a lot of confusion on that point as well.. > > Sure, I'll add that verbiage to a v3 sometime after rc1. It would also be great to patch1 into a stable non-rebasing git tree so it can be used as a baseline for other 4.12 targeted development. The device-dax patches in particular collide with some pending development.
On Mon, Feb 27, 2017 at 07:36:09PM -0800, Dan Williams wrote: > On Mon, Feb 27, 2017 at 7:27 PM, Logan Gunthorpe <logang@deltatee.com> wrote: > > > > > > On 27/02/17 09:56 AM, Jason Gunthorpe wrote: > >> Yes please, that is way too subtle. Suggest: > >> > >> NOTE: Callers must assume that userspace was able to open the cdev and > >> can call cdev fops callbacks at any time, even if this function fails. > >> > >> I would also add a note to cdev_device_del: > >> > >> NOTE: This guarantees that associated sysfs callbacks are not running > >> or runnable, however any open cdevs will remain and their fops remain > >> callable even after this returns. > >> > >> Since I have seen a lot of confusion on that point as well.. > > > > Sure, I'll add that verbiage to a v3 sometime after rc1. > > It would also be great to patch1 into a stable non-rebasing git tree > so it can be used as a baseline for other 4.12 targeted development. > The device-dax patches in particular collide with some pending > development. I'll do that once -rc1 is out, I can't take any new patches right now :) thanks, greg k-h
diff --git a/fs/char_dev.c b/fs/char_dev.c index 44a240c..471d480 100644 --- a/fs/char_dev.c +++ b/fs/char_dev.c @@ -471,6 +471,70 @@ int cdev_add(struct cdev *p, dev_t dev, unsigned count) return 0; } +/** + * cdev_set_parent() - set the parent kobject for a char device + * @p: the cdev structure + * @kobj: the kobject to take a reference to + * + * cdev_set_parent() sets a parent kobject which will be referenced + * appropriately so the parent is not freed before the cdev. This + * should be called before cdev_add. + */ +void cdev_set_parent(struct cdev *p, struct kobject *kobj) +{ + WARN_ON(!kobj->state_initialized); + p->kobj.parent = kobj; +} + +/** + * cdev_device_add() - add a char device and it's corresponding + * struct device, linkink + * @dev: the device structure + * @cdev: the cdev structure + * + * cdev_device_add() adds the char device represented by @cdev to the system, + * just as cdev_add does. It then adds @dev to the system using device_add + * The dev_t for the char device will be taken from the struct device which + * needs to be initialized first. This helper function correctly takes a + * reference to the parent device so the parent will not get released until + * all references to the cdev are released. + * + * This function should be used whenever the struct cdev and the + * struct device are members of the same structure whose lifetime is + * managed by the struct device. + */ +int cdev_device_add(struct cdev *cdev, struct device *dev) +{ + int rc = 0; + + cdev_set_parent(cdev, &dev->kobj); + + rc = cdev_add(cdev, dev->devt, 1); + if (rc) + return rc; + + rc = device_add(dev); + if (rc) + cdev_del(cdev); + + return rc; +} + +/** + * cdev_device_del() - inverse of cdev_device_add + * @dev: the device structure + * @cdev: the cdev structure + * + * cdev_device_del() is a helper function to call cdev_del + * and device_del. It should be used whenever cdev_device_add + * is used. + */ +void cdev_device_del(struct cdev *cdev, struct device *dev) +{ + device_del(dev); + cdev_del(cdev); +} + static void cdev_unmap(dev_t dev, unsigned count) { kobj_unmap(cdev_map, dev, count); @@ -570,5 +634,8 @@ EXPORT_SYMBOL(cdev_init); EXPORT_SYMBOL(cdev_alloc); EXPORT_SYMBOL(cdev_del); EXPORT_SYMBOL(cdev_add); +EXPORT_SYMBOL(cdev_set_parent); +EXPORT_SYMBOL(cdev_device_add); +EXPORT_SYMBOL(cdev_device_del); EXPORT_SYMBOL(__register_chrdev); EXPORT_SYMBOL(__unregister_chrdev); diff --git a/include/linux/cdev.h b/include/linux/cdev.h index f876361..4c99579 100644 --- a/include/linux/cdev.h +++ b/include/linux/cdev.h @@ -26,6 +26,10 @@ void cdev_put(struct cdev *p); int cdev_add(struct cdev *, dev_t, unsigned); +void cdev_set_parent(struct cdev *p, struct kobject *kobj); +int cdev_device_add(struct cdev *cdev, struct device *dev); +void cdev_device_del(struct cdev *cdev, struct device *dev); + void cdev_del(struct cdev *); void cd_forget(struct inode *);