From mboxrd@z Thu Jan 1 00:00:00 1970 From: Logan Gunthorpe Subject: [PATCH 01/14] chardev: add helper function to register char devs with a struct device Date: Mon, 20 Feb 2017 22:00:40 -0700 Message-ID: <1487653253-11497-2-git-send-email-logang@deltatee.com> References: <1487653253-11497-1-git-send-email-logang@deltatee.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1487653253-11497-1-git-send-email-logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linux-nvdimm-bounces-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org Sender: "Linux-nvdimm" To: Greg Kroah-Hartman , Dan Williams , Alexander Viro , Johannes Thumshirn , Jan Kara , Arnd Bergmann , Sajjan Vikas C , Dmitry Torokhov , Linus Walleij , Alexandre Courbot , Peter Huewe , Marcel Selhorst , Jarkko Sakkinen , Jason Gunthorpe , Olof Johansson , Doug Ledford , Sean Hefty , Hal Rosenstock , Dmitry Vyukov , Haggai Eran , Parav Pandit , Leon Romanovsky , Jonathan Cc: linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, rtc-linux-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, osd-dev-yNzVSZO3znNg9hUCZPvPmw@public.gmane.org, linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-gpio@vger.kernel.org Credit for this patch goes entirely to Dan Williams [1]. I've just fleshed out the comments and created the patch, but the premise remains exactly the same. 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 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. In [1], Dan notes he took inspiration for the form of the API device_add_disk. [1] https://lkml.org/lkml/2017/2/13/700 [2] https://lkml.org/lkml/2017/2/10/370 Signed-off-by: Logan Gunthorpe --- fs/char_dev.c | 24 ++++++++++++++++++++++++ include/linux/cdev.h | 1 + 2 files changed, 25 insertions(+) diff --git a/fs/char_dev.c b/fs/char_dev.c index 44a240c..1f9246c 100644 --- a/fs/char_dev.c +++ b/fs/char_dev.c @@ -471,6 +471,29 @@ int cdev_add(struct cdev *p, dev_t dev, unsigned count) return 0; } +/** + * device_add_cdev() - add a char device to the system with a parent + * struct device + * @parent: the device structure of the parent + * @cdev: the cdev structure for the device + * @count: the number of consecutive minor numbers corresponding to this + * + * device_add_cdev() adds the char device represented by @p to the system, + * just as cdev_add does. 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. + * (Thus, cdev_del should be called before device_unregister.) 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 device_add_cdev(struct device *parent, struct cdev *cdev) +{ + cdev->kobj.parent = &parent->kobj; + return cdev_add(cdev, parent->devt, 1); +} + static void cdev_unmap(dev_t dev, unsigned count) { kobj_unmap(cdev_map, dev, count); @@ -570,5 +593,6 @@ EXPORT_SYMBOL(cdev_init); EXPORT_SYMBOL(cdev_alloc); EXPORT_SYMBOL(cdev_del); EXPORT_SYMBOL(cdev_add); +EXPORT_SYMBOL(device_add_cdev); EXPORT_SYMBOL(__register_chrdev); EXPORT_SYMBOL(__unregister_chrdev); diff --git a/include/linux/cdev.h b/include/linux/cdev.h index f876361..9edbc37 100644 --- a/include/linux/cdev.h +++ b/include/linux/cdev.h @@ -25,6 +25,7 @@ struct cdev *cdev_alloc(void); void cdev_put(struct cdev *p); int cdev_add(struct cdev *, dev_t, unsigned); +int device_add_cdev(struct device *parent, struct cdev *cdev); void cdev_del(struct cdev *); -- 2.1.4 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ale.deltatee.com (ale.deltatee.com [207.54.116.67]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9667C82223 for ; Mon, 20 Feb 2017 21:03:10 -0800 (PST) From: Logan Gunthorpe Date: Mon, 20 Feb 2017 22:00:40 -0700 Message-Id: <1487653253-11497-2-git-send-email-logang@deltatee.com> In-Reply-To: <1487653253-11497-1-git-send-email-logang@deltatee.com> References: <1487653253-11497-1-git-send-email-logang@deltatee.com> Subject: [PATCH 01/14] chardev: add helper function to register char devs with a struct device List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: linux-nvdimm-bounces@lists.01.org Sender: "Linux-nvdimm" To: Greg Kroah-Hartman , Dan Williams , Alexander Viro , Johannes Thumshirn , Jan Kara , Arnd Bergmann , Sajjan Vikas C , Dmitry Torokhov , Linus Walleij , Alexandre Courbot , Peter Huewe , Marcel Selhorst , Jarkko Sakkinen , Jason Gunthorpe , Olof Johansson , Doug Ledford , Sean Hefty , Hal Rosenstock , Dmitry Vyukov , Haggai Eran , Parav Pandit , Leon Romanovsky , Jonathan Cameron , Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald-Stadler , Hans Verkuil , Mauro Carvalho Chehab , Artem Bityutskiy , Richard Weinberger , David Woodhouse , Brian Norris , Boris Brezillon , Marek Vasut , Cyrille Pitchen , Matt Porter , Alexandre Bounine , Andrew Morton , Joe Perches , Lorenzo Stoakes , Vladimir Zapolskiy , Alessandro Zummo , Alexandre Belloni , Boaz Harrosh , Benny Halevy , "James E.J. Bottomley" , "Martin K. Petersen" , Stephen Bates , Bjorn Helgaas Cc: linux-scsi@vger.kernel.org, rtc-linux@googlegroups.com, linux-gpio@vger.kernel.org, linux-iio@vger.kernel.org, linux-pci@vger.kernel.org, linux-nvdimm@lists.01.org, linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org, tpmdd-devel@lists.sourceforge.net, linux-mtd@lists.infradead.org, osd-dev@open-osd.org, linux-input@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-media@vger.kernel.org List-ID: Credit for this patch goes entirely to Dan Williams [1]. I've just fleshed out the comments and created the patch, but the premise remains exactly the same. 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 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. In [1], Dan notes he took inspiration for the form of the API device_add_disk. [1] https://lkml.org/lkml/2017/2/13/700 [2] https://lkml.org/lkml/2017/2/10/370 Signed-off-by: Logan Gunthorpe --- fs/char_dev.c | 24 ++++++++++++++++++++++++ include/linux/cdev.h | 1 + 2 files changed, 25 insertions(+) diff --git a/fs/char_dev.c b/fs/char_dev.c index 44a240c..1f9246c 100644 --- a/fs/char_dev.c +++ b/fs/char_dev.c @@ -471,6 +471,29 @@ int cdev_add(struct cdev *p, dev_t dev, unsigned count) return 0; } +/** + * device_add_cdev() - add a char device to the system with a parent + * struct device + * @parent: the device structure of the parent + * @cdev: the cdev structure for the device + * @count: the number of consecutive minor numbers corresponding to this + * + * device_add_cdev() adds the char device represented by @p to the system, + * just as cdev_add does. 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. + * (Thus, cdev_del should be called before device_unregister.) 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 device_add_cdev(struct device *parent, struct cdev *cdev) +{ + cdev->kobj.parent = &parent->kobj; + return cdev_add(cdev, parent->devt, 1); +} + static void cdev_unmap(dev_t dev, unsigned count) { kobj_unmap(cdev_map, dev, count); @@ -570,5 +593,6 @@ EXPORT_SYMBOL(cdev_init); EXPORT_SYMBOL(cdev_alloc); EXPORT_SYMBOL(cdev_del); EXPORT_SYMBOL(cdev_add); +EXPORT_SYMBOL(device_add_cdev); EXPORT_SYMBOL(__register_chrdev); EXPORT_SYMBOL(__unregister_chrdev); diff --git a/include/linux/cdev.h b/include/linux/cdev.h index f876361..9edbc37 100644 --- a/include/linux/cdev.h +++ b/include/linux/cdev.h @@ -25,6 +25,7 @@ struct cdev *cdev_alloc(void); void cdev_put(struct cdev *p); int cdev_add(struct cdev *, dev_t, unsigned); +int device_add_cdev(struct device *parent, struct cdev *cdev); void cdev_del(struct cdev *); -- 2.1.4 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ale.deltatee.com ([207.54.116.67]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1cg2bi-0002pj-Mb for linux-mtd@lists.infradead.org; Tue, 21 Feb 2017 05:03:28 +0000 From: Logan Gunthorpe To: Greg Kroah-Hartman , Dan Williams , Alexander Viro , Johannes Thumshirn , Jan Kara , Arnd Bergmann , Sajjan Vikas C , Dmitry Torokhov , Linus Walleij , Alexandre Courbot , Peter Huewe , Marcel Selhorst , Jarkko Sakkinen , Jason Gunthorpe , Olof Johansson , Doug Ledford , Sean Hefty , Hal Rosenstock , Dmitry Vyukov , Haggai Eran , Parav Pandit , Leon Romanovsky , Jonathan Cameron , Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald-Stadler , Hans Verkuil , Mauro Carvalho Chehab , Artem Bityutskiy , Richard Weinberger , David Woodhouse , Brian Norris , Boris Brezillon , Marek Vasut , Cyrille Pitchen , Matt Porter , Alexandre Bounine , Andrew Morton , Joe Perches , Lorenzo Stoakes , Vladimir Zapolskiy , Alessandro Zummo , Alexandre Belloni , Boaz Harrosh , Benny Halevy , "James E.J. Bottomley" , "Martin K. Petersen" , Stephen Bates , Bjorn Helgaas Cc: linux-pci@vger.kernel.org, osd-dev@open-osd.org, linux-scsi@vger.kernel.org, rtc-linux@googlegroups.com, linux-mtd@lists.infradead.org, linux-media@vger.kernel.org, linux-iio@vger.kernel.org, linux-rdma@vger.kernel.org, tpmdd-devel@lists.sourceforge.net, linux-gpio@vger.kernel.org, linux-input@vger.kernel.org, linux-nvdimm@lists.01.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Logan Gunthorpe Date: Mon, 20 Feb 2017 22:00:40 -0700 Message-Id: <1487653253-11497-2-git-send-email-logang@deltatee.com> In-Reply-To: <1487653253-11497-1-git-send-email-logang@deltatee.com> References: <1487653253-11497-1-git-send-email-logang@deltatee.com> Subject: [PATCH 01/14] chardev: add helper function to register char devs with a struct device List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Credit for this patch goes entirely to Dan Williams [1]. I've just fleshed out the comments and created the patch, but the premise remains exactly the same. 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 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. In [1], Dan notes he took inspiration for the form of the API device_add_disk. [1] https://lkml.org/lkml/2017/2/13/700 [2] https://lkml.org/lkml/2017/2/10/370 Signed-off-by: Logan Gunthorpe --- fs/char_dev.c | 24 ++++++++++++++++++++++++ include/linux/cdev.h | 1 + 2 files changed, 25 insertions(+) diff --git a/fs/char_dev.c b/fs/char_dev.c index 44a240c..1f9246c 100644 --- a/fs/char_dev.c +++ b/fs/char_dev.c @@ -471,6 +471,29 @@ int cdev_add(struct cdev *p, dev_t dev, unsigned count) return 0; } +/** + * device_add_cdev() - add a char device to the system with a parent + * struct device + * @parent: the device structure of the parent + * @cdev: the cdev structure for the device + * @count: the number of consecutive minor numbers corresponding to this + * + * device_add_cdev() adds the char device represented by @p to the system, + * just as cdev_add does. 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. + * (Thus, cdev_del should be called before device_unregister.) 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 device_add_cdev(struct device *parent, struct cdev *cdev) +{ + cdev->kobj.parent = &parent->kobj; + return cdev_add(cdev, parent->devt, 1); +} + static void cdev_unmap(dev_t dev, unsigned count) { kobj_unmap(cdev_map, dev, count); @@ -570,5 +593,6 @@ EXPORT_SYMBOL(cdev_init); EXPORT_SYMBOL(cdev_alloc); EXPORT_SYMBOL(cdev_del); EXPORT_SYMBOL(cdev_add); +EXPORT_SYMBOL(device_add_cdev); EXPORT_SYMBOL(__register_chrdev); EXPORT_SYMBOL(__unregister_chrdev); diff --git a/include/linux/cdev.h b/include/linux/cdev.h index f876361..9edbc37 100644 --- a/include/linux/cdev.h +++ b/include/linux/cdev.h @@ -25,6 +25,7 @@ struct cdev *cdev_alloc(void); void cdev_put(struct cdev *p); int cdev_add(struct cdev *, dev_t, unsigned); +int device_add_cdev(struct device *parent, struct cdev *cdev); void cdev_del(struct cdev *); -- 2.1.4 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Sender: rtc-linux@googlegroups.com MIME-Version: 1.0 Received: from ale.deltatee.com (ale.deltatee.com. [207.54.116.67]) by gmr-mx.google.com with ESMTPS id 23si2940905pfw.4.2017.02.20.21.03.03 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 20 Feb 2017 21:03:04 -0800 (PST) From: Logan Gunthorpe To: Greg Kroah-Hartman , Dan Williams , Alexander Viro , Johannes Thumshirn , Jan Kara , Arnd Bergmann , Sajjan Vikas C , Dmitry Torokhov , Linus Walleij , Alexandre Courbot , Peter Huewe , Marcel Selhorst , Jarkko Sakkinen , Jason Gunthorpe , Olof Johansson , Doug Ledford , Sean Hefty , Hal Rosenstock , Dmitry Vyukov , Haggai Eran , Parav Pandit , Leon Romanovsky , Jonathan Cameron , Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald-Stadler , Hans Verkuil , Mauro Carvalho Chehab , Artem Bityutskiy , Richard Weinberger , David Woodhouse , Brian Norris , Boris Brezillon , Marek Vasut , Cyrille Pitchen , Matt Porter , Alexandre Bounine , Andrew Morton , Joe Perches , Lorenzo Stoakes , Vladimir Zapolskiy , Alessandro Zummo , Alexandre Belloni , Boaz Harrosh , Benny Halevy , "James E.J. Bottomley" , "Martin K. Petersen" , Stephen Bates , Bjorn Helgaas Cc: linux-pci@vger.kernel.org, osd-dev@open-osd.org, linux-scsi@vger.kernel.org, rtc-linux@googlegroups.com, linux-mtd@lists.infradead.org, linux-media@vger.kernel.org, linux-iio@vger.kernel.org, linux-rdma@vger.kernel.org, tpmdd-devel@lists.sourceforge.net, linux-gpio@vger.kernel.org, linux-input@vger.kernel.org, linux-nvdimm@lists.01.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Logan Gunthorpe Date: Mon, 20 Feb 2017 22:00:40 -0700 Message-Id: <1487653253-11497-2-git-send-email-logang@deltatee.com> In-Reply-To: <1487653253-11497-1-git-send-email-logang@deltatee.com> References: <1487653253-11497-1-git-send-email-logang@deltatee.com> Subject: [rtc-linux] [PATCH 01/14] chardev: add helper function to register char devs with a struct device Reply-To: rtc-linux@googlegroups.com Content-Type: text/plain; charset=UTF-8 List-ID: List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , Credit for this patch goes entirely to Dan Williams [1]. I've just fleshed out the comments and created the patch, but the premise remains exactly the same. 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 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. In [1], Dan notes he took inspiration for the form of the API device_add_disk. [1] https://lkml.org/lkml/2017/2/13/700 [2] https://lkml.org/lkml/2017/2/10/370 Signed-off-by: Logan Gunthorpe --- fs/char_dev.c | 24 ++++++++++++++++++++++++ include/linux/cdev.h | 1 + 2 files changed, 25 insertions(+) diff --git a/fs/char_dev.c b/fs/char_dev.c index 44a240c..1f9246c 100644 --- a/fs/char_dev.c +++ b/fs/char_dev.c @@ -471,6 +471,29 @@ int cdev_add(struct cdev *p, dev_t dev, unsigned count) return 0; } +/** + * device_add_cdev() - add a char device to the system with a parent + * struct device + * @parent: the device structure of the parent + * @cdev: the cdev structure for the device + * @count: the number of consecutive minor numbers corresponding to this + * + * device_add_cdev() adds the char device represented by @p to the system, + * just as cdev_add does. 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. + * (Thus, cdev_del should be called before device_unregister.) 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 device_add_cdev(struct device *parent, struct cdev *cdev) +{ + cdev->kobj.parent = &parent->kobj; + return cdev_add(cdev, parent->devt, 1); +} + static void cdev_unmap(dev_t dev, unsigned count) { kobj_unmap(cdev_map, dev, count); @@ -570,5 +593,6 @@ EXPORT_SYMBOL(cdev_init); EXPORT_SYMBOL(cdev_alloc); EXPORT_SYMBOL(cdev_del); EXPORT_SYMBOL(cdev_add); +EXPORT_SYMBOL(device_add_cdev); EXPORT_SYMBOL(__register_chrdev); EXPORT_SYMBOL(__unregister_chrdev); diff --git a/include/linux/cdev.h b/include/linux/cdev.h index f876361..9edbc37 100644 --- a/include/linux/cdev.h +++ b/include/linux/cdev.h @@ -25,6 +25,7 @@ struct cdev *cdev_alloc(void); void cdev_put(struct cdev *p); int cdev_add(struct cdev *, dev_t, unsigned); +int device_add_cdev(struct device *parent, struct cdev *cdev); void cdev_del(struct cdev *); -- 2.1.4 -- You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. --- You received this message because you are subscribed to the Google Groups "rtc-linux" group. To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Logan Gunthorpe Subject: [PATCH 01/14] chardev: add helper function to register char devs with a struct device Date: Mon, 20 Feb 2017 22:00:40 -0700 Message-ID: <1487653253-11497-2-git-send-email-logang@deltatee.com> References: <1487653253-11497-1-git-send-email-logang@deltatee.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, rtc-linux-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, osd-dev-yNzVSZO3znNg9hUCZPvPmw@public.gmane.org, linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Greg Kroah-Hartman , Dan Williams , Alexander Viro , Johannes Thumshirn , Jan Kara , Arnd Bergmann , Sajjan Vikas C , Dmitry Torokhov , Linus Walleij , Alexandre Courbot , Peter Huewe , Marcel Selhorst , Jarkko Sakkinen , Jason Gunthorpe , Olof Johansson , Doug Ledford , Sean Hefty , Hal Rosenstock , Dmitry Vyukov , Haggai Eran , Parav Pandit , Leon Romanovsky , Jonathan Cameron Return-path: In-Reply-To: <1487653253-11497-1-git-send-email-logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linux-nvdimm-bounces-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org Sender: "Linux-nvdimm" List-Id: linux-fsdevel.vger.kernel.org Credit for this patch goes entirely to Dan Williams [1]. I've just fleshed out the comments and created the patch, but the premise remains exactly the same. 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 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. In [1], Dan notes he took inspiration for the form of the API device_add_disk. [1] https://lkml.org/lkml/2017/2/13/700 [2] https://lkml.org/lkml/2017/2/10/370 Signed-off-by: Logan Gunthorpe --- fs/char_dev.c | 24 ++++++++++++++++++++++++ include/linux/cdev.h | 1 + 2 files changed, 25 insertions(+) diff --git a/fs/char_dev.c b/fs/char_dev.c index 44a240c..1f9246c 100644 --- a/fs/char_dev.c +++ b/fs/char_dev.c @@ -471,6 +471,29 @@ int cdev_add(struct cdev *p, dev_t dev, unsigned count) return 0; } +/** + * device_add_cdev() - add a char device to the system with a parent + * struct device + * @parent: the device structure of the parent + * @cdev: the cdev structure for the device + * @count: the number of consecutive minor numbers corresponding to this + * + * device_add_cdev() adds the char device represented by @p to the system, + * just as cdev_add does. 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. + * (Thus, cdev_del should be called before device_unregister.) 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 device_add_cdev(struct device *parent, struct cdev *cdev) +{ + cdev->kobj.parent = &parent->kobj; + return cdev_add(cdev, parent->devt, 1); +} + static void cdev_unmap(dev_t dev, unsigned count) { kobj_unmap(cdev_map, dev, count); @@ -570,5 +593,6 @@ EXPORT_SYMBOL(cdev_init); EXPORT_SYMBOL(cdev_alloc); EXPORT_SYMBOL(cdev_del); EXPORT_SYMBOL(cdev_add); +EXPORT_SYMBOL(device_add_cdev); EXPORT_SYMBOL(__register_chrdev); EXPORT_SYMBOL(__unregister_chrdev); diff --git a/include/linux/cdev.h b/include/linux/cdev.h index f876361..9edbc37 100644 --- a/include/linux/cdev.h +++ b/include/linux/cdev.h @@ -25,6 +25,7 @@ struct cdev *cdev_alloc(void); void cdev_put(struct cdev *p); int cdev_add(struct cdev *, dev_t, unsigned); +int device_add_cdev(struct device *parent, struct cdev *cdev); void cdev_del(struct cdev *); -- 2.1.4