[PATCH 00/14] Cleanup chardev instances with helper function

[PATCH 00/14] Cleanup chardev instances with helper function

[Logan Gunthorpe]

Hello,

Our story for this patch-set begins with a new driver I wrote and am in
the process of submitting upstream. That driver creates a fairly
standard char device and the code for it was copied from a similar
instance in device-dax. However, upon review, Greg Kroah-Hartman
noticed [1] a suspicious line that assigned to the parent field of
the underlying kobject for the char device.

I removed that from my code and endeavoured to remove it from the
code I copied as well. However, Dan Williams pointed out [2] that this
code is necessary for correct reference counting of the underlying
structures. This prompted me to do a fair bit more research and
investigation into whats going on and I found it to be a common pattern.
(Although misleading and though required to be correct, frequently
forgotten.) This pattern is used in at least 15 places in the kernel
and probably should have been used in at least 5 more.

This patch-set aims to correct this and hopefully prevent future
developers from wasting their time on it. The first patch introduces
a new helper API as originally proposed by Dan Williams [3]. Please
see the commit message for that patch for a longer description of the
problem and history.

The subsequent patches either update correct instances to use the
new API or fix incorrect usages to ensure the cdev correctly takes
a reference count using the new API (this is noted in those patches).

This moves all except four of the cdev.kobj.parent usages into the one
cdev function, the remaining four are in the infiniband subsystem and
I've left alone because that subsystem seems to make use of kobjects
directly (instead of struct devices). These are noted in patch 7 of
this series.

This series is based on v4.10 with the exception of the last patch
which is for my new driver which, as yet, has not been accepted
upstream.

@Dan the first patch in this series will need your sign-off.

Thanks,

Logan

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

Logan Gunthorpe (14):
  chardev: add helper function to register char devs with a struct
    device
  device-dax: utilize new device_add_cdev helper function
  input: utilize new device_add_cdev helper function
  gpiolib: utilize new device_add_cdev helper function
  tpm-chip: utilize new device_add_cdev helper function
  platform/chrome: utilize new device_add_cdev helper function
  infiniband: utilize new device_add_cdev helper function
  iio:core: utilize new device_add_cdev helper function
  media: utilize new device_add_cdev helper function
  mtd: utilize new device_add_cdev helper function
  rapidio: utilize new device_add_cdev helper function
  rtc: utilize new device_add_cdev helper function
  scsi: utilize new device_add_cdev helper function
  switchtec: utilize new device_add_cdev helper function

 drivers/char/tpm/tpm-chip.c              |  3 +--
 drivers/dax/dax.c                        |  5 ++---
 drivers/gpio/gpiolib.c                   |  3 +--
 drivers/iio/industrialio-core.c          |  3 +--
 drivers/infiniband/core/ucm.c            |  8 +++++---
 drivers/input/evdev.c                    |  3 +--
 drivers/input/joydev.c                   |  3 +--
 drivers/input/mousedev.c                 |  3 +--
 drivers/media/cec/cec-core.c             |  3 +--
 drivers/media/media-devnode.c            |  3 +--
 drivers/mtd/ubi/build.c                  |  8 +++++---
 drivers/mtd/ubi/vmt.c                    | 10 +++++-----
 drivers/pci/switch/switchtec.c           |  3 +--
 drivers/platform/chrome/cros_ec_dev.c    |  6 ++----
 drivers/rapidio/devices/rio_mport_cdev.c |  9 ++++++---
 drivers/rtc/rtc-dev.c                    |  3 +--
 drivers/scsi/osd/osd_uld.c               |  9 +++++----
 fs/char_dev.c                            | 24 ++++++++++++++++++++++++
 include/linux/cdev.h                     |  1 +
 19 files changed, 65 insertions(+), 45 deletions(-)

--
2.1.4

[PATCH 01/14] chardev: add helper function to register char devs with a struct device

[Logan Gunthorpe]

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 <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
---
 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

[PATCH 02/14] device-dax: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
---
 drivers/dax/dax.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/dax/dax.c b/drivers/dax/dax.c
index ed758b7..0d24822 100644
--- a/drivers/dax/dax.c
+++ b/drivers/dax/dax.c
@@ -701,12 +701,12 @@ struct dax_dev *devm_create_dax_dev(struct dax_region *dax_region,
 
 	/* device_initialize() so cdev can reference kobj parent */
 	device_initialize(dev);
+	dev->devt = dev_t;
 
 	cdev = &dax_dev->cdev;
 	cdev_init(cdev, &dax_fops);
 	cdev->owner = parent->driver->owner;
-	cdev->kobj.parent = &dev->kobj;
-	rc = cdev_add(&dax_dev->cdev, dev_t, 1);
+	rc = device_add_cdev(dev, cdev);
 	if (rc)
 		goto err_cdev;
 
@@ -716,7 +716,6 @@ struct dax_dev *devm_create_dax_dev(struct dax_region *dax_region,
 	dax_dev->region = dax_region;
 	kref_get(&dax_region->kref);
 
-	dev->devt = dev_t;
 	dev->class = dax_class;
 	dev->parent = parent;
 	dev->groups = dax_attribute_groups;
-- 
2.1.4

[PATCH 03/14] input: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
---
 drivers/input/evdev.c    | 3 +--
 drivers/input/joydev.c   | 3 +--
 drivers/input/mousedev.c | 3 +--
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..a3407ea 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -1416,8 +1416,7 @@ static int evdev_connect(struct input_handler *handler, struct input_dev *dev,
 		goto err_free_evdev;
 
 	cdev_init(&evdev->cdev, &evdev_fops);
-	evdev->cdev.kobj.parent = &evdev->dev.kobj;
-	error = cdev_add(&evdev->cdev, evdev->dev.devt, 1);
+	error = device_add_cdev(&evdev->dev, &evdev->cdev);
 	if (error)
 		goto err_unregister_handle;
 
diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index abd18f3..012e06f 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -905,8 +905,7 @@ static int joydev_connect(struct input_handler *handler, struct input_dev *dev,
 		goto err_free_joydev;
 
 	cdev_init(&joydev->cdev, &joydev_fops);
-	joydev->cdev.kobj.parent = &joydev->dev.kobj;
-	error = cdev_add(&joydev->cdev, joydev->dev.devt, 1);
+	error = device_add_cdev(&joydev->dev, &joydev->cdev);
 	if (error)
 		goto err_unregister_handle;
 
diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c
index b604564..efd8666 100644
--- a/drivers/input/mousedev.c
+++ b/drivers/input/mousedev.c
@@ -901,8 +901,7 @@ static struct mousedev *mousedev_create(struct input_dev *dev,
 	}
 
 	cdev_init(&mousedev->cdev, &mousedev_fops);
-	mousedev->cdev.kobj.parent = &mousedev->dev.kobj;
-	error = cdev_add(&mousedev->cdev, mousedev->dev.devt, 1);
+	error = device_add_cdev(&mousedev->dev, &mousedev->cdev);
 	if (error)
 		goto err_unregister_handle;
 
-- 
2.1.4

[PATCH 04/14] gpiolib: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
---
 drivers/gpio/gpiolib.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a07ae9e..04dbc4a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1036,9 +1036,8 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
 
 	cdev_init(&gdev->chrdev, &gpio_fileops);
 	gdev->chrdev.owner = THIS_MODULE;
-	gdev->chrdev.kobj.parent = &gdev->dev.kobj;
 	gdev->dev.devt = MKDEV(MAJOR(gpio_devt), gdev->id);
-	status = cdev_add(&gdev->chrdev, gdev->dev.devt, 1);
+	status = device_add_cdev(&gdev->dev, &gdev->chrdev);
 	if (status < 0)
 		chip_warn(gdev->chip, "failed to add char device %d:%d\n",
 			  MAJOR(gpio_devt), gdev->id);
-- 
2.1.4

[PATCH 05/14] tpm-chip: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
---
 drivers/char/tpm/tpm-chip.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index a77262d..dc90b45 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -187,7 +187,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
 
 	cdev_init(&chip->cdev, &tpm_fops);
 	chip->cdev.owner = THIS_MODULE;
-	chip->cdev.kobj.parent = &chip->dev.kobj;
 
 	return chip;
 
@@ -230,7 +229,7 @@ static int tpm_add_char_device(struct tpm_chip *chip)
 {
 	int rc;
 
-	rc = cdev_add(&chip->cdev, chip->dev.devt, 1);
+	rc = device_add_cdev(&chip->dev, &chip->cdev);
 	if (rc) {
 		dev_err(&chip->dev,
 			"unable to cdev_add() %s, major %d, minor %d, err=%d\n",
-- 
2.1.4

[PATCH 06/14] platform/chrome: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
---
 drivers/platform/chrome/cros_ec_dev.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
index 47268ec..658fb99 100644
--- a/drivers/platform/chrome/cros_ec_dev.c
+++ b/drivers/platform/chrome/cros_ec_dev.c
@@ -388,7 +388,6 @@ static int ec_device_probe(struct platform_device *pdev)
 	int retval = -ENOMEM;
 	struct device *dev = &pdev->dev;
 	struct cros_ec_platform *ec_platform = dev_get_platdata(dev);
-	dev_t devno = MKDEV(ec_major, pdev->id);
 	struct cros_ec_dev *ec = kzalloc(sizeof(*ec), GFP_KERNEL);
 
 	if (!ec)
@@ -401,6 +400,7 @@ static int ec_device_probe(struct platform_device *pdev)
 	ec->features[0] = -1U; /* Not cached yet */
 	ec->features[1] = -1U; /* Not cached yet */
 	device_initialize(&ec->class_dev);
+	ec->class_dev.devt = MKDEV(ec_major, pdev->id);
 	cdev_init(&ec->cdev, &fops);
 
 	/*
@@ -408,8 +408,7 @@ static int ec_device_probe(struct platform_device *pdev)
 	 * Link cdev to the class device to be sure device is not used
 	 * before unbinding it.
 	 */
-	ec->cdev.kobj.parent = &ec->class_dev.kobj;
-	retval = cdev_add(&ec->cdev, devno, 1);
+	retval = device_add_cdev(&ec->class_dev, &ec->cdev);
 	if (retval) {
 		dev_err(dev, ": failed to add character device\n");
 		goto cdev_add_failed;
@@ -420,7 +419,6 @@ static int ec_device_probe(struct platform_device *pdev)
 	 * Link to the character device for creating the /dev entry
 	 * in devtmpfs.
 	 */
-	ec->class_dev.devt = ec->cdev.dev;
 	ec->class_dev.class = &cros_class;
 	ec->class_dev.parent = dev;
 	ec->class_dev.release = __remove;
-- 
2.1.4

[PATCH 07/14] infiniband: utilize new device_add_cdev helper function

[Logan Gunthorpe]

This patch updates core/ucm.c which didn't originally use the
cdev.kobj.parent with it's parent device. I did not look heavily into
whether this was a bug or not, but it seems likely to me there would
be a use before free.

I also took a look at core/uverbs_main.c, core/user_mad.c and
hw/hfi1/device.c which utilize cdev.kobj.parent but because the
infiniband core seems to use kobjs internally instead of struct devices
they could not be converted to use the new helper API and still
directly manipulate the internals of the kobj.

Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/ucm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/core/ucm.c b/drivers/infiniband/core/ucm.c
index e0a995b..38ea316 100644
--- a/drivers/infiniband/core/ucm.c
+++ b/drivers/infiniband/core/ucm.c
@@ -1283,18 +1283,20 @@ static void ib_ucm_add_one(struct ib_device *device)
 		set_bit(devnum, dev_map);
 	}
 
+	device_initialize(&ucm_dev->dev);
+	ucm_dev->dev.devt = base;
+
 	cdev_init(&ucm_dev->cdev, &ucm_fops);
 	ucm_dev->cdev.owner = THIS_MODULE;
 	kobject_set_name(&ucm_dev->cdev.kobj, "ucm%d", ucm_dev->devnum);
-	if (cdev_add(&ucm_dev->cdev, base, 1))
+	if (device_add_cdev(&ucm_dev->dev, &ucm_dev->cdev))
 		goto err;
 
 	ucm_dev->dev.class = &cm_class;
 	ucm_dev->dev.parent = device->dma_device;
-	ucm_dev->dev.devt = ucm_dev->cdev.dev;
 	ucm_dev->dev.release = ib_ucm_release_dev;
 	dev_set_name(&ucm_dev->dev, "ucm%d", ucm_dev->devnum);
-	if (device_register(&ucm_dev->dev))
+	if (device_add(&ucm_dev->dev))
 		goto err_cdev;
 
 	if (device_create_file(&ucm_dev->dev, &dev_attr_ibdev))
-- 
2.1.4

[PATCH 08/14] iio:core: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
---
 drivers/iio/industrialio-core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index aaca428..b0ef245 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1718,8 +1718,7 @@ int iio_device_register(struct iio_dev *indio_dev)
 
 	cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
 	indio_dev->chrdev.owner = indio_dev->info->driver_module;
-	indio_dev->chrdev.kobj.parent = &indio_dev->dev.kobj;
-	ret = cdev_add(&indio_dev->chrdev, indio_dev->dev.devt, 1);
+	ret = device_add_cdev(&indio_dev->dev, &indio_dev->chrdev);
 	if (ret < 0)
 		goto error_unreg_eventset;
 
-- 
2.1.4

[PATCH 09/14] media: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
---
 drivers/media/cec/cec-core.c  | 3 +--
 drivers/media/media-devnode.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
index aca3ab8..a475aa5 100644
--- a/drivers/media/cec/cec-core.c
+++ b/drivers/media/cec/cec-core.c
@@ -137,10 +137,9 @@ static int __must_check cec_devnode_register(struct cec_devnode *devnode,
 
 	/* Part 2: Initialize and register the character device */
 	cdev_init(&devnode->cdev, &cec_devnode_fops);
-	devnode->cdev.kobj.parent = &devnode->dev.kobj;
 	devnode->cdev.owner = owner;
 
-	ret = cdev_add(&devnode->cdev, devnode->dev.devt, 1);
+	ret = device_add_cdev(&devnode->dev, &devnode->cdev);
 	if (ret < 0) {
 		pr_err("%s: cdev_add failed\n", __func__);
 		goto clr_bit;
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index f2772ba..8d4718c 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -255,9 +255,8 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	/* Part 2: Initialize and register the character device */
 	cdev_init(&devnode->cdev, &media_devnode_fops);
 	devnode->cdev.owner = owner;
-	devnode->cdev.kobj.parent = &devnode->dev.kobj;
 
-	ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t), devnode->minor), 1);
+	ret = device_add_cdev(&devnode->dev, &devnode->cdev);
 	if (ret < 0) {
 		pr_err("%s: cdev_add failed\n", __func__);
 		goto cdev_add_error;
-- 
2.1.4

[PATCH 10/14] mtd: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Note: neither of the cdev instances in the mtd tree originally
set the kobject parent. Thus, I'm reasonably confident that
both these instances would have suffered from a minor use after
free bug if the cdevs were open when the backing device was released.

Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
---
 drivers/mtd/ubi/build.c |  8 +++++---
 drivers/mtd/ubi/vmt.c   | 10 +++++-----
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 85d54f3..a509f15 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -434,11 +434,10 @@ static int ubi_sysfs_init(struct ubi_device *ubi, int *ref)
 	int err;
 
 	ubi->dev.release = dev_release;
-	ubi->dev.devt = ubi->cdev.dev;
 	ubi->dev.class = &ubi_class;
 	ubi->dev.groups = ubi_dev_groups;
 	dev_set_name(&ubi->dev, UBI_NAME_STR"%d", ubi->ubi_num);
-	err = device_register(&ubi->dev);
+	err = device_add(&ubi->dev);
 	if (err)
 		return err;
 
@@ -508,12 +507,15 @@ static int uif_init(struct ubi_device *ubi, int *ref)
 		return err;
 	}
 
+	device_initialize(&ubi->dev);
+	ubi->dev.devt = dev;
+
 	ubi_assert(MINOR(dev) == 0);
 	cdev_init(&ubi->cdev, &ubi_cdev_operations);
 	dbg_gen("%s major is %u", ubi->ubi_name, MAJOR(dev));
 	ubi->cdev.owner = THIS_MODULE;
 
-	err = cdev_add(&ubi->cdev, dev, 1);
+	err = device_add_cdev(&ubi->dev, &ubi->cdev);
 	if (err) {
 		ubi_err(ubi, "cannot add character device");
 		goto out_unreg;
diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
index 7ac78c1..df84ba7 100644
--- a/drivers/mtd/ubi/vmt.c
+++ b/drivers/mtd/ubi/vmt.c
@@ -159,7 +159,6 @@ int ubi_create_volume(struct ubi_device *ubi, struct ubi_mkvol_req *req)
 	struct ubi_volume *vol;
 	struct ubi_vtbl_record vtbl_rec;
 	struct ubi_eba_table *eba_tbl = NULL;
-	dev_t dev;
 
 	if (ubi->ro_mode)
 		return -EROFS;
@@ -265,11 +264,13 @@ int ubi_create_volume(struct ubi_device *ubi, struct ubi_mkvol_req *req)
 			vol->last_eb_bytes = vol->usable_leb_size;
 	}
 
+	device_initialize(&vol->dev);
+	vol->dev.devt = MKDEV(MAJOR(ubi->cdev.dev), vol_id + 1);
+
 	/* Register character device for the volume */
 	cdev_init(&vol->cdev, &ubi_vol_cdev_operations);
 	vol->cdev.owner = THIS_MODULE;
-	dev = MKDEV(MAJOR(ubi->cdev.dev), vol_id + 1);
-	err = cdev_add(&vol->cdev, dev, 1);
+	err = device_add_cdev(&vol->dev, &vol->cdev);
 	if (err) {
 		ubi_err(ubi, "cannot add character device");
 		goto out_mapping;
@@ -277,12 +278,11 @@ int ubi_create_volume(struct ubi_device *ubi, struct ubi_mkvol_req *req)
 
 	vol->dev.release = vol_release;
 	vol->dev.parent = &ubi->dev;
-	vol->dev.devt = dev;
 	vol->dev.class = &ubi_class;
 	vol->dev.groups = volume_dev_groups;
 
 	dev_set_name(&vol->dev, "%s_%d", ubi->ubi_name, vol->vol_id);
-	err = device_register(&vol->dev);
+	err = device_add(&vol->dev);
 	if (err) {
 		ubi_err(ubi, "cannot register device");
 		goto out_cdev;
-- 
2.1.4

[PATCH 11/14] rapidio: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Note: the chardev instance in rio_mport_cdev originally did not
set the kobject parent. Thus, I'm reasonably confident that because
of this, this code  would have suffered from a minor use after free
bug if the cdev was open when the backing device was released.

Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
---
 drivers/rapidio/devices/rio_mport_cdev.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/rapidio/devices/rio_mport_cdev.c b/drivers/rapidio/devices/rio_mport_cdev.c
index 9013a58..10a6b54 100644
--- a/drivers/rapidio/devices/rio_mport_cdev.c
+++ b/drivers/rapidio/devices/rio_mport_cdev.c
@@ -2445,23 +2445,26 @@ static struct mport_dev *mport_cdev_add(struct rio_mport *mport)
 	mutex_init(&md->buf_mutex);
 	mutex_init(&md->file_mutex);
 	INIT_LIST_HEAD(&md->file_list);
+
+	device_initialize(&md->dev);
+	md->dev.devt = MKDEV(MAJOR(dev_number), mport->id);
+
 	cdev_init(&md->cdev, &mport_fops);
 	md->cdev.owner = THIS_MODULE;
-	ret = cdev_add(&md->cdev, MKDEV(MAJOR(dev_number), mport->id), 1);
+	ret = device_add_cdev(&md->dev, &md->cdev);
 	if (ret < 0) {
 		kfree(md);
 		rmcd_error("Unable to register a device, err=%d", ret);
 		return NULL;
 	}
 
-	md->dev.devt = md->cdev.dev;
 	md->dev.class = dev_class;
 	md->dev.parent = &mport->dev;
 	md->dev.release = mport_device_release;
 	dev_set_name(&md->dev, DEV_NAME "%d", mport->id);
 	atomic_set(&md->active, 1);
 
-	ret = device_register(&md->dev);
+	ret = device_add(&md->dev);
 	if (ret) {
 		rmcd_error("Failed to register mport %d (err=%d)",
 		       mport->id, ret);
-- 
2.1.4

[PATCH 12/14] rtc: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
---
 drivers/rtc/rtc-dev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c
index a6d9434..e4012bb 100644
--- a/drivers/rtc/rtc-dev.c
+++ b/drivers/rtc/rtc-dev.c
@@ -477,12 +477,11 @@ void rtc_dev_prepare(struct rtc_device *rtc)
 
 	cdev_init(&rtc->char_dev, &rtc_dev_fops);
 	rtc->char_dev.owner = rtc->owner;
-	rtc->char_dev.kobj.parent = &rtc->dev.kobj;
 }
 
 void rtc_dev_add_device(struct rtc_device *rtc)
 {
-	if (cdev_add(&rtc->char_dev, rtc->dev.devt, 1))
+	if (device_add_cdev(&rtc->dev, &rtc->char_dev))
 		dev_warn(&rtc->dev, "%s: failed to add char device %d:%d\n",
 			rtc->name, MAJOR(rtc_devt), rtc->id);
 	else
-- 
2.1.4

[PATCH 13/14] scsi: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Note: the chardev instance in osd_uld.c originally did not
set the kobject parent. Thus, I'm reasonably confident that because
of this, this code  would have suffered from a minor use after free
bug if the cdev was open when the backing device was released.

Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
---
 drivers/scsi/osd/osd_uld.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
index 243eab3..519be56 100644
--- a/drivers/scsi/osd/osd_uld.c
+++ b/drivers/scsi/osd/osd_uld.c
@@ -473,18 +473,19 @@ static int osd_probe(struct device *dev)
 		goto err_put_disk;
 	}
 
+	device_initialize(&oud->class_dev);
+	oud->class_dev.devt = MKDEV(SCSI_OSD_MAJOR, oud->minor);
+
 	/* init the char-device for communication with user-mode */
 	cdev_init(&oud->cdev, &osd_fops);
 	oud->cdev.owner = THIS_MODULE;
-	error = cdev_add(&oud->cdev,
-			 MKDEV(SCSI_OSD_MAJOR, oud->minor), 1);
+	error = device_add_cdev(&oud->class_dev, &oud->cdev);
 	if (error) {
 		OSD_ERR("cdev_add failed\n");
 		goto err_put_disk;
 	}
 
 	/* class device member */
-	oud->class_dev.devt = oud->cdev.dev;
 	oud->class_dev.class = &osd_uld_class;
 	oud->class_dev.parent = dev;
 	oud->class_dev.release = __remove;
@@ -494,7 +495,7 @@ static int osd_probe(struct device *dev)
 		goto err_put_cdev;
 	}
 
-	error = device_register(&oud->class_dev);
+	error = device_add(&oud->class_dev);
 	if (error) {
 		OSD_ERR("device_register failed => %d\n", error);
 		goto err_put_cdev;
-- 
2.1.4

[PATCH 14/14] switchtec: utilize new device_add_cdev helper function

[Logan Gunthorpe]

Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
---
 drivers/pci/switch/switchtec.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
index 82bfd18..95aabd0 100644
--- a/drivers/pci/switch/switchtec.c
+++ b/drivers/pci/switch/switchtec.c
@@ -1233,9 +1233,8 @@ static struct switchtec_dev *stdev_create(struct pci_dev *pdev)
 	cdev = &stdev->cdev;
 	cdev_init(cdev, &switchtec_fops);
 	cdev->owner = THIS_MODULE;
-	cdev->kobj.parent = &dev->kobj;
 
-	rc = cdev_add(&stdev->cdev, dev->devt, 1);
+	rc = device_add_cdev(dev, cdev);
 	if (rc)
 		goto err_cdev;
 
-- 
2.1.4

Re: [PATCH 01/14] chardev: add helper function to register char devs with a struct device

[Dmitry Torokhov]

On Mon, Feb 20, 2017 at 10:00:40PM -0700, Logan Gunthorpe wrote:
> 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 <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
> ---
>  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

My only objection is to this statement. There is absolutely nothing that
prevents from calling device_unregister() first and cdev_del() later.
Refcounting will sort it all out.

> + * 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 *);
>  

Thanks.

-- 
Dmitry

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 01/14] chardev: add helper function to register char devs with a struct device

[Logan Gunthorpe]

On 20/02/17 10:35 PM, Dmitry Torokhov wrote:
> My only objection is to this statement. There is absolutely nothing that
> prevents from calling device_unregister() first and cdev_del() later.
> Refcounting will sort it all out.

Yeah, I was really trying to warn people against calling cdev_del within
the release function of the device. If you do that, then the cdev
reference will block the device from ever getting released.

Certainly, you could call device_unregister followed by cdev_del. I
could reword this if you feel it necessary.

Logan

Re: [PATCH 00/14] Cleanup chardev instances with helper function

[Richard Weinberger]

Logan,

Am 21.02.2017 um 06:00 schrieb Logan Gunthorpe:
> Hello,
> 
> Our story for this patch-set begins with a new driver I wrote and am in
> the process of submitting upstream. That driver creates a fairly
> standard char device and the code for it was copied from a similar
> instance in device-dax. However, upon review, Greg Kroah-Hartman
> noticed [1] a suspicious line that assigned to the parent field of
> the underlying kobject for the char device.
> 
> I removed that from my code and endeavoured to remove it from the
> code I copied as well. However, Dan Williams pointed out [2] that this
> code is necessary for correct reference counting of the underlying
> structures. This prompted me to do a fair bit more research and
> investigation into whats going on and I found it to be a common pattern.
> (Although misleading and though required to be correct, frequently
> forgotten.) This pattern is used in at least 15 places in the kernel
> and probably should have been used in at least 5 more.
> 
> This patch-set aims to correct this and hopefully prevent future
> developers from wasting their time on it. The first patch introduces
> a new helper API as originally proposed by Dan Williams [3]. Please
> see the commit message for that patch for a longer description of the
> problem and history.
> 
> The subsequent patches either update correct instances to use the
> new API or fix incorrect usages to ensure the cdev correctly takes
> a reference count using the new API (this is noted in those patches).
> 
> This moves all except four of the cdev.kobj.parent usages into the one
> cdev function, the remaining four are in the infiniband subsystem and
> I've left alone because that subsystem seems to make use of kobjects
> directly (instead of struct devices). These are noted in patch 7 of
> this series.
> 
> This series is based on v4.10 with the exception of the last patch
> which is for my new driver which, as yet, has not been accepted
> upstream.
> 
> @Dan the first patch in this series will need your sign-off.
> 
> Thanks,
> 
> Logan
> 
> [1] https://lkml.org/lkml/2017/2/10/370
> [2] https://lkml.org/lkml/2017/2/10/607
> [3] https://lkml.org/lkml/2017/2/13/700
> 
> Logan Gunthorpe (14):
>   chardev: add helper function to register char devs with a struct
>     device
>   device-dax: utilize new device_add_cdev helper function
>   input: utilize new device_add_cdev helper function
>   gpiolib: utilize new device_add_cdev helper function
>   tpm-chip: utilize new device_add_cdev helper function
>   platform/chrome: utilize new device_add_cdev helper function
>   infiniband: utilize new device_add_cdev helper function
>   iio:core: utilize new device_add_cdev helper function
>   media: utilize new device_add_cdev helper function
>   mtd: utilize new device_add_cdev helper function
>   rapidio: utilize new device_add_cdev helper function
>   rtc: utilize new device_add_cdev helper function
>   scsi: utilize new device_add_cdev helper function
>   switchtec: utilize new device_add_cdev helper function
> 
>  drivers/char/tpm/tpm-chip.c              |  3 +--
>  drivers/dax/dax.c                        |  5 ++---
>  drivers/gpio/gpiolib.c                   |  3 +--
>  drivers/iio/industrialio-core.c          |  3 +--
>  drivers/infiniband/core/ucm.c            |  8 +++++---
>  drivers/input/evdev.c                    |  3 +--
>  drivers/input/joydev.c                   |  3 +--
>  drivers/input/mousedev.c                 |  3 +--
>  drivers/media/cec/cec-core.c             |  3 +--
>  drivers/media/media-devnode.c            |  3 +--
>  drivers/mtd/ubi/build.c                  |  8 +++++---
>  drivers/mtd/ubi/vmt.c                    | 10 +++++-----
>  drivers/pci/switch/switchtec.c           |  3 +--
>  drivers/platform/chrome/cros_ec_dev.c    |  6 ++----
>  drivers/rapidio/devices/rio_mport_cdev.c |  9 ++++++---
>  drivers/rtc/rtc-dev.c                    |  3 +--
>  drivers/scsi/osd/osd_uld.c               |  9 +++++----
>  fs/char_dev.c                            | 24 ++++++++++++++++++++++++
>  include/linux/cdev.h                     |  1 +
>  19 files changed, 65 insertions(+), 45 deletions(-)

Do you have a git tree where I can pull from?

Thanks,
//richard

Re: [PATCH 02/14] device-dax: utilize new device_add_cdev helper function

[Alexandre Belloni]

Hi,

A small comment, you must always have a commit message. Even if it is
small.

On 20/02/2017 at 22:00:41 -0700, Logan Gunthorpe wrote:
> Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/dax/dax.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dax/dax.c b/drivers/dax/dax.c
> index ed758b7..0d24822 100644
> --- a/drivers/dax/dax.c
> +++ b/drivers/dax/dax.c
> @@ -701,12 +701,12 @@ struct dax_dev *devm_create_dax_dev(struct dax_region *dax_region,
>  
>  	/* device_initialize() so cdev can reference kobj parent */
>  	device_initialize(dev);
> +	dev->devt = dev_t;
>  
>  	cdev = &dax_dev->cdev;
>  	cdev_init(cdev, &dax_fops);
>  	cdev->owner = parent->driver->owner;
> -	cdev->kobj.parent = &dev->kobj;
> -	rc = cdev_add(&dax_dev->cdev, dev_t, 1);
> +	rc = device_add_cdev(dev, cdev);
>  	if (rc)
>  		goto err_cdev;
>  
> @@ -716,7 +716,6 @@ struct dax_dev *devm_create_dax_dev(struct dax_region *dax_region,
>  	dax_dev->region = dax_region;
>  	kref_get(&dax_region->kref);
>  
> -	dev->devt = dev_t;
>  	dev->class = dax_class;
>  	dev->parent = parent;
>  	dev->groups = dax_attribute_groups;
> -- 
> 2.1.4
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 13/14] scsi: utilize new device_add_cdev helper function

[Boaz Harrosh]

On 02/21/2017 07:00 AM, Logan Gunthorpe wrote:
> Note: the chardev instance in osd_uld.c originally did not
> set the kobject parent. Thus, I'm reasonably confident that because
> of this, this code  would have suffered from a minor use after free
> bug if the cdev was open when the backing device was released.
> 
> Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>

Cool thanks. And even a bug fix
ACK-by: Boaz Harrosh <ooo-rh7Tgz9RNieUD9Wbbkgo/g@public.gmane.org>

> ---
>  drivers/scsi/osd/osd_uld.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> index 243eab3..519be56 100644
> --- a/drivers/scsi/osd/osd_uld.c
> +++ b/drivers/scsi/osd/osd_uld.c
> @@ -473,18 +473,19 @@ static int osd_probe(struct device *dev)
>  		goto err_put_disk;
>  	}
>  
> +	device_initialize(&oud->class_dev);
> +	oud->class_dev.devt = MKDEV(SCSI_OSD_MAJOR, oud->minor);
> +
>  	/* init the char-device for communication with user-mode */
>  	cdev_init(&oud->cdev, &osd_fops);
>  	oud->cdev.owner = THIS_MODULE;
> -	error = cdev_add(&oud->cdev,
> -			 MKDEV(SCSI_OSD_MAJOR, oud->minor), 1);
> +	error = device_add_cdev(&oud->class_dev, &oud->cdev);
>  	if (error) {
>  		OSD_ERR("cdev_add failed\n");
>  		goto err_put_disk;
>  	}
>  
>  	/* class device member */
> -	oud->class_dev.devt = oud->cdev.dev;
>  	oud->class_dev.class = &osd_uld_class;
>  	oud->class_dev.parent = dev;
>  	oud->class_dev.release = __remove;
> @@ -494,7 +495,7 @@ static int osd_probe(struct device *dev)
>  		goto err_put_cdev;
>  	}
>  
> -	error = device_register(&oud->class_dev);
> +	error = device_add(&oud->class_dev);
>  	if (error) {
>  		OSD_ERR("device_register failed => %d\n", error);
>  		goto err_put_cdev;
> 

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 01/14] chardev: add helper function to register char devs with a struct device

[Dan Williams]

On Mon, Feb 20, 2017 at 10:35 PM, Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org> wrote:
>
>
> On 20/02/17 10:35 PM, Dmitry Torokhov wrote:
>> My only objection is to this statement. There is absolutely nothing that
>> prevents from calling device_unregister() first and cdev_del() later.
>> Refcounting will sort it all out.
>
> Yeah, I was really trying to warn people against calling cdev_del within
> the release function of the device. If you do that, then the cdev
> reference will block the device from ever getting released.
>
> Certainly, you could call device_unregister followed by cdev_del. I
> could reword this if you feel it necessary.

I agree with Dmitry, just delete the statement in parenthesis and the
rest is fine. If you're modifying this patch it might be good to take
the opportunity to add a WARN_ON(!parent->kobj.state_initialized) to
catch attempts to call device_add_cdev() with an uninitialized device.

You can also add:

Signed-off-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Re: [PATCH 05/14] tpm-chip: utilize new device_add_cdev helper function

[Jason Gunthorpe]

On Mon, Feb 20, 2017 at 10:00:44PM -0700, Logan Gunthorpe wrote:
> Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
>  drivers/char/tpm/tpm-chip.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

Jason

Re: [PATCH 07/14] infiniband: utilize new device_add_cdev helper function

[Jason Gunthorpe]

On Mon, Feb 20, 2017 at 10:00:46PM -0700, Logan Gunthorpe wrote:
> This patch updates core/ucm.c which didn't originally use the
> cdev.kobj.parent with it's parent device. I did not look heavily into
> whether this was a bug or not, but it seems likely to me there would
> be a use before free.

Hum, is probably safe - ib_ucm_remove_one can only happen on module
unload and the cdev holds the module lock while open.

Even so device_add_cdev should be used anyhow.

> I also took a look at core/uverbs_main.c, core/user_mad.c and
> hw/hfi1/device.c which utilize cdev.kobj.parent but because the
> infiniband core seems to use kobjs internally instead of struct devices
> they could not be converted to use the new helper API and still
> directly manipulate the internals of the kobj.

Yes, and hfi1 had the same use-afte-free bug when it was first
submitted as well. IHMO cdev should have a helper entry for this style
of use case as well.

> diff --git a/drivers/infiniband/core/ucm.c b/drivers/infiniband/core/ucm.c
> index e0a995b..38ea316 100644
> +++ b/drivers/infiniband/core/ucm.c
> @@ -1283,18 +1283,20 @@ static void ib_ucm_add_one(struct ib_device *device)
>  		set_bit(devnum, dev_map);
>  	}
>  
> +	device_initialize(&ucm_dev->dev);

Ah, this needs more help. Once device_initialize is called put_device
should be the error-unwind, can you use something more like this?

>From ac7a35ea98066c9a9e3f3e54557c0ccda44b0ffc Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Date: Tue, 21 Feb 2017 12:07:55 -0700
Subject: [PATCH] infiniband: utilize new device_add_cdev helper

The use after free is not triggerable here because the cdev holds
the module lock and the only device_unregister is only triggered by
module ouload, however make the change for consistency.

To make this work the cdev_del needs to move out of the struct device
release function.

Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 drivers/infiniband/core/ucm.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/core/ucm.c b/drivers/infiniband/core/ucm.c
index 7713ef089c3ccc..acda8d941381f3 100644
--- a/drivers/infiniband/core/ucm.c
+++ b/drivers/infiniband/core/ucm.c
@@ -1202,12 +1202,16 @@ static void ib_ucm_release_dev(struct device *dev)
 	struct ib_ucm_device *ucm_dev;
 
 	ucm_dev = container_of(dev, struct ib_ucm_device, dev);
+	kfree(ucm_dev);
+}
+
+static void ib_ucm_cdev_del(struct ib_ucm_device *ucm_dev)
+{
 	cdev_del(&ucm_dev->cdev);
 	if (ucm_dev->devnum < IB_UCM_MAX_DEVICES)
 		clear_bit(ucm_dev->devnum, dev_map);
 	else
 		clear_bit(ucm_dev->devnum - IB_UCM_MAX_DEVICES, overflow_map);
-	kfree(ucm_dev);
 }
 
 static const struct file_operations ucm_fops = {
@@ -1263,6 +1267,7 @@ static void ib_ucm_add_one(struct ib_device *device)
 	if (!ucm_dev)
 		return;
 
+	device_initialize(&ucm_dev->dev);
 	ucm_dev->ib_dev = device;
 
 	devnum = find_first_zero_bit(dev_map, IB_UCM_MAX_DEVICES);
@@ -1280,18 +1285,19 @@ static void ib_ucm_add_one(struct ib_device *device)
 		set_bit(devnum, dev_map);
 	}
 
+	ucm_dev->dev.devt = base;
+	ucm_dev->dev.release = ib_ucm_release_dev;
+
 	cdev_init(&ucm_dev->cdev, &ucm_fops);
 	ucm_dev->cdev.owner = THIS_MODULE;
 	kobject_set_name(&ucm_dev->cdev.kobj, "ucm%d", ucm_dev->devnum);
-	if (cdev_add(&ucm_dev->cdev, base, 1))
+	if (device_add_cdev(&ucm_dev->dev, &ucm_dev->cdev))
 		goto err;
 
 	ucm_dev->dev.class = &cm_class;
 	ucm_dev->dev.parent = device->dma_device;
-	ucm_dev->dev.devt = ucm_dev->cdev.dev;
-	ucm_dev->dev.release = ib_ucm_release_dev;
 	dev_set_name(&ucm_dev->dev, "ucm%d", ucm_dev->devnum);
-	if (device_register(&ucm_dev->dev))
+	if (device_add(&ucm_dev->dev))
 		goto err_cdev;
 
 	if (device_create_file(&ucm_dev->dev, &dev_attr_ibdev))
@@ -1303,13 +1309,9 @@ static void ib_ucm_add_one(struct ib_device *device)
 err_dev:
 	device_unregister(&ucm_dev->dev);
 err_cdev:
-	cdev_del(&ucm_dev->cdev);
-	if (ucm_dev->devnum < IB_UCM_MAX_DEVICES)
-		clear_bit(devnum, dev_map);
-	else
-		clear_bit(devnum, overflow_map);
+	ib_ucm_cdev_del(ucm_dev);
 err:
-	kfree(ucm_dev);
+	put_device(&ucm_dev->dev);
 	return;
 }
 
@@ -1320,6 +1322,7 @@ static void ib_ucm_remove_one(struct ib_device *device, void *client_data)
 	if (!ucm_dev)
 		return;
 
+	ib_ucm_cdev_del(ucm_dev);
 	device_unregister(&ucm_dev->dev);
 }
 
-- 
2.7.4

Re: [PATCH 02/14] device-dax: utilize new device_add_cdev helper function

[Dan Williams]

On Tue, Feb 21, 2017 at 3:37 AM, Alexandre Belloni
<alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi,
>
> A small comment, you must always have a commit message. Even if it is
> small.

Yes, something like: "Replace the open coded initialization of the
cdev with the new device_add_cdev() helper. The helper takes the
proper reference against the parent device while the cdev object is
alive."

With that you can also add:

Acked-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 01/14] chardev: add helper function to register char devs with a struct device

[Logan Gunthorpe]

Hey,

Ok, I'll do a v2 later this week with some of the feedback and tags?

Logan

On 21/02/17 11:18 AM, Dan Williams wrote:
> On Mon, Feb 20, 2017 at 10:35 PM, Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org> wrote:
>>
>>
>> On 20/02/17 10:35 PM, Dmitry Torokhov wrote:
>>> My only objection is to this statement. There is absolutely nothing that
>>> prevents from calling device_unregister() first and cdev_del() later.
>>> Refcounting will sort it all out.
>>
>> Yeah, I was really trying to warn people against calling cdev_del within
>> the release function of the device. If you do that, then the cdev
>> reference will block the device from ever getting released.
>>
>> Certainly, you could call device_unregister followed by cdev_del. I
>> could reword this if you feel it necessary.
> 
> I agree with Dmitry, just delete the statement in parenthesis and the
> rest is fine. If you're modifying this patch it might be good to take
> the opportunity to add a WARN_ON(!parent->kobj.state_initialized) to
> catch attempts to call device_add_cdev() with an uninitialized device.
> 
> You can also add:
> 
> Signed-off-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 

Re: [PATCH 07/14] infiniband: utilize new device_add_cdev helper function

[Logan Gunthorpe]

On 21/02/17 12:09 PM, Jason Gunthorpe wrote:
> On Mon, Feb 20, 2017 at 10:00:46PM -0700, Logan Gunthorpe wrote:
>> This patch updates core/ucm.c which didn't originally use the
>> cdev.kobj.parent with it's parent device. I did not look heavily into
>> whether this was a bug or not, but it seems likely to me there would
>> be a use before free.
> 
> Hum, is probably safe - ib_ucm_remove_one can only happen on module
> unload and the cdev holds the module lock while open.

Ah, yes looks like you are correct.

> Even so device_add_cdev should be used anyhow.

Agreed.

>> I also took a look at core/uverbs_main.c, core/user_mad.c and
>> hw/hfi1/device.c which utilize cdev.kobj.parent but because the
>> infiniband core seems to use kobjs internally instead of struct devices
>> they could not be converted to use the new helper API and still
>> directly manipulate the internals of the kobj.
> 
> Yes, and hfi1 had the same use-afte-free bug when it was first
> submitted as well. IHMO cdev should have a helper entry for this style
> of use case as well.

I agree, but I'm not sure what that api should look like. Same thing but
kobject_add_cdev???

>> diff --git a/drivers/infiniband/core/ucm.c b/drivers/infiniband/core/ucm.c
>> index e0a995b..38ea316 100644
>> +++ b/drivers/infiniband/core/ucm.c
>> @@ -1283,18 +1283,20 @@ static void ib_ucm_add_one(struct ib_device *device)
>>  		set_bit(devnum, dev_map);
>>  	}
>>  
>> +	device_initialize(&ucm_dev->dev);
> 
> Ah, this needs more help. Once device_initialize is called put_device
> should be the error-unwind, can you use something more like this?

Is that true? Once device_register or device_add is called then you need
to use put_device. But I didn't think that's true for device_initialize.
In fact device_add is what does the first get_device so this doesn't add
up to me. device_initialize only inits some structures it doesn't do
anything that needs to be torn down -- at least that I'm aware of.

I know the DAX code only uses put_device after device_add. [1]

Logan

[1] http://lxr.free-electrons.com/source/drivers/dax/dax.c#L713

Re: [PATCH 07/14] infiniband: utilize new device_add_cdev helper function

[Jason Gunthorpe]

On Tue, Feb 21, 2017 at 04:54:05PM -0700, Logan Gunthorpe wrote:

> Is that true? Once device_register or device_add is called then you need
> to use put_device.

General rule is once kref_init has been called then you should use
kref_put and not kfree.

device_initialize ultimately calls kref_init..

Reasoning that you don't 'need' to use put_device until device_add is
just too hard.

For instance, there is still another bug in ib_ucm_add_one:

	if (device_add(&ucm_dev->dev))
		goto err_cdev;

	if (device_create_file(&ucm_dev->dev, &dev_attr_ibdev))
		goto err_dev;
[....]
err_dev:
	device_unregister(&ucm_dev->dev);
err_cdev:
	ib_ucm_cdev_del(ucm_dev);
err:
	kfree(ucm_dev);
	return;
}

If we go down the err_dev path then device_unregister will probably
kfree ucm_dev - the argument is not guarenteed valid after
device_unregister returns - this is what makes it different from
device_del.

The simplest fix is to change the unwind into:

err_dev:
	device_del(&ucm_dev->dev);
err_cdev:
	ib_ucm_cdev_del(ucm_dev);
err:
	put_device(&ucm_dev->dev);
	return;
}

And the only way to keep our idiomatic goto unwind working is if all
'goto errs' can call put_device - which requires the device_initialize
be done before any errors are possible.

> In fact device_add is what does the first get_device so this doesn't
> add up to me.

Not quite, kref_init creates a kref with a count of 1 - eg the caller
owns the ref, and that ref must be put to trigger kref release.

Thus good kref usage should always pair a kref_put with the kref_init.

The get_device at the start of device_add pairs with the put_device at
the end of device_del and the kref_init pairs with the put_device at
the end of device_unregister. (notice that device_unregister ends up
calling put_device twice in a row..)

I'll send you a clean patch for your v2.

> I know the DAX code only uses put_device after device_add.

Looks to me like that code fails to call cdev_del if device_add fails?

This approach is problematic because it is trying do the ida removals
in the release function. That is not necessary and has the side effect
of making the release function uncallable until too late in the
process.

Jason

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 07/14] infiniband: utilize new device_add_cdev helper function

[Lars-Peter Clausen]

On 02/22/2017 12:54 AM, Logan Gunthorpe wrote:
[...]
>>
>> Ah, this needs more help. Once device_initialize is called put_device
>> should be the error-unwind, can you use something more like this?
> 
> Is that true? Once device_register or device_add is called then you need
> to use put_device. [...]

device_initialize() documentation:

	NOTE: Use put_device() to give up your reference instead of freeing
	@dev directly once you have called this function.


-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 03/14] input: utilize new device_add_cdev helper function

[Dmitry Torokhov]

On Mon, Feb 20, 2017 at 10:00:42PM -0700, Logan Gunthorpe wrote:
> Signed-off-by: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>
> ---

Please add some more verbage, otherwise

Acked-by: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>


>  drivers/input/evdev.c    | 3 +--
>  drivers/input/joydev.c   | 3 +--
>  drivers/input/mousedev.c | 3 +--
>  3 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index e9ae3d5..a3407ea 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -1416,8 +1416,7 @@ static int evdev_connect(struct input_handler *handler, struct input_dev *dev,
>  		goto err_free_evdev;
>  
>  	cdev_init(&evdev->cdev, &evdev_fops);
> -	evdev->cdev.kobj.parent = &evdev->dev.kobj;
> -	error = cdev_add(&evdev->cdev, evdev->dev.devt, 1);
> +	error = device_add_cdev(&evdev->dev, &evdev->cdev);
>  	if (error)
>  		goto err_unregister_handle;
>  
> diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
> index abd18f3..012e06f 100644
> --- a/drivers/input/joydev.c
> +++ b/drivers/input/joydev.c
> @@ -905,8 +905,7 @@ static int joydev_connect(struct input_handler *handler, struct input_dev *dev,
>  		goto err_free_joydev;
>  
>  	cdev_init(&joydev->cdev, &joydev_fops);
> -	joydev->cdev.kobj.parent = &joydev->dev.kobj;
> -	error = cdev_add(&joydev->cdev, joydev->dev.devt, 1);
> +	error = device_add_cdev(&joydev->dev, &joydev->cdev);
>  	if (error)
>  		goto err_unregister_handle;
>  
> diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c
> index b604564..efd8666 100644
> --- a/drivers/input/mousedev.c
> +++ b/drivers/input/mousedev.c
> @@ -901,8 +901,7 @@ static struct mousedev *mousedev_create(struct input_dev *dev,
>  	}
>  
>  	cdev_init(&mousedev->cdev, &mousedev_fops);
> -	mousedev->cdev.kobj.parent = &mousedev->dev.kobj;
> -	error = cdev_add(&mousedev->cdev, mousedev->dev.devt, 1);
> +	error = device_add_cdev(&mousedev->dev, &mousedev->cdev);
>  	if (error)
>  		goto err_unregister_handle;
>  
> -- 
> 2.1.4
> 

-- 
Dmitry

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.