* [PATCH 1/2] char_dev: allow setting up and pinning parent devices [not found] <20121019175046.GP2616@ZenIV.linux.org.uk> @ 2012-10-21 7:24 ` Dmitry Torokhov 2012-10-21 7:24 ` [PATCH 2/2] Input: fix use-after-free introduced with dynamic minor changes Dmitry Torokhov 2012-10-21 7:39 ` [PATCH 1/2] char_dev: allow setting up and pinning parent devices Al Viro 0 siblings, 2 replies; 8+ messages in thread From: Dmitry Torokhov @ 2012-10-21 7:24 UTC (permalink / raw) To: Al Viro, Dave Jones; +Cc: Linus Torvalds, linux-kernel, linux-input In certain cases (for example when a cdev structure is embedded into another object whose lifetime is controlled by a separate device object) it is beneficial to tie lifetime of another struct device to the lifetime of character device so that related object is not freed until after char_dev object is freed. To achieve this allow setting a "parent" device for character devices and pin them when doing cdev_add() and unpin when last reference to cdev structure is being released. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- Sorry, messed up the first attempt to send... So, how about these 2? I enabled kmemleak and added some debug printks and it looks like we dropping references and freeing object at right times and in proper order. Thanks! fs/char_dev.c | 18 +++++++++++++++++- include/linux/cdev.h | 1 + 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/fs/char_dev.c b/fs/char_dev.c index 3f152b9..f8c44cc 100644 --- a/fs/char_dev.c +++ b/fs/char_dev.c @@ -471,9 +471,19 @@ static int exact_lock(dev_t dev, void *data) */ int cdev_add(struct cdev *p, dev_t dev, unsigned count) { + int error; + p->dev = dev; p->count = count; - return kobj_map(cdev_map, dev, count, NULL, exact_match, exact_lock, p); + + error = kobj_map(cdev_map, dev, count, NULL, + exact_match, exact_lock, p); + if (error) + return error; + + get_device(p->parent); + + return 0; } static void cdev_unmap(dev_t dev, unsigned count) @@ -498,14 +508,20 @@ void cdev_del(struct cdev *p) static void cdev_default_release(struct kobject *kobj) { struct cdev *p = container_of(kobj, struct cdev, kobj); + struct device *parent = p->parent; + cdev_purge(p); + put_device(parent); } static void cdev_dynamic_release(struct kobject *kobj) { struct cdev *p = container_of(kobj, struct cdev, kobj); + struct device *parent = p->parent; + cdev_purge(p); kfree(p); + put_device(parent); } static struct kobj_type ktype_cdev_default = { diff --git a/include/linux/cdev.h b/include/linux/cdev.h index fb45919..eba8251 100644 --- a/include/linux/cdev.h +++ b/include/linux/cdev.h @@ -12,6 +12,7 @@ struct module; struct cdev { struct kobject kobj; struct module *owner; + struct device *parent; const struct file_operations *ops; struct list_head list; dev_t dev; -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] Input: fix use-after-free introduced with dynamic minor changes 2012-10-21 7:24 ` [PATCH 1/2] char_dev: allow setting up and pinning parent devices Dmitry Torokhov @ 2012-10-21 7:24 ` Dmitry Torokhov 2012-10-21 7:39 ` [PATCH 1/2] char_dev: allow setting up and pinning parent devices Al Viro 1 sibling, 0 replies; 8+ messages in thread From: Dmitry Torokhov @ 2012-10-21 7:24 UTC (permalink / raw) To: Al Viro, Dave Jones; +Cc: Linus Torvalds, linux-kernel, linux-input Commit 7f8d4cad1e4e11a45d02bd6e024cc2812963c38a made evdev, joydev and mousedev to embed struct cdev into their respective structures representing input devices. Unfortunately character device structure may outlive the parent structure unless we do not set it up as parent of character device so that it will stay pinned until character device is freed. Also, now that parent structure is pinned while character device exists we do not need to pin and unpin it every time user opens or closes it. Reported-by: Dave Jones <davej@redhat.com> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- 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 6ae2ac4..49d9e34 100644 --- a/drivers/input/evdev.c +++ b/drivers/input/evdev.c @@ -292,7 +292,6 @@ static int evdev_release(struct inode *inode, struct file *file) kfree(client); evdev_close_device(evdev); - put_device(&evdev->dev); return 0; } @@ -331,7 +330,6 @@ static int evdev_open(struct inode *inode, struct file *file) file->private_data = client; nonseekable_open(inode, file); - get_device(&evdev->dev); return 0; err_free_client: @@ -1001,6 +999,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.parent = &evdev->dev; error = cdev_add(&evdev->cdev, evdev->dev.devt, 1); if (error) goto err_unregister_handle; diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c index 63e5916..00c3537 100644 --- a/drivers/input/joydev.c +++ b/drivers/input/joydev.c @@ -262,7 +262,6 @@ static int joydev_release(struct inode *inode, struct file *file) kfree(client); joydev_close_device(joydev); - put_device(&joydev->dev); return 0; } @@ -289,7 +288,6 @@ static int joydev_open(struct inode *inode, struct file *file) file->private_data = client; nonseekable_open(inode, file); - get_device(&joydev->dev); return 0; err_free_client: @@ -877,6 +875,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.parent = &joydev->dev; error = cdev_add(&joydev->cdev, joydev->dev.devt, 1); if (error) goto err_unregister_handle; diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c index a1b4c37..b65d476 100644 --- a/drivers/input/mousedev.c +++ b/drivers/input/mousedev.c @@ -523,7 +523,6 @@ static int mousedev_release(struct inode *inode, struct file *file) kfree(client); mousedev_close_device(mousedev); - put_device(&mousedev->dev); return 0; } @@ -558,7 +557,6 @@ static int mousedev_open(struct inode *inode, struct file *file) file->private_data = client; nonseekable_open(inode, file); - get_device(&mousedev->dev); return 0; err_free_client: @@ -892,6 +890,7 @@ static struct mousedev *mousedev_create(struct input_dev *dev, } cdev_init(&mousedev->cdev, &mousedev_fops); + mousedev->cdev.parent = &mousedev->dev; error = cdev_add(&mousedev->cdev, mousedev->dev.devt, 1); if (error) goto err_unregister_handle; -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] char_dev: allow setting up and pinning parent devices 2012-10-21 7:24 ` [PATCH 1/2] char_dev: allow setting up and pinning parent devices Dmitry Torokhov 2012-10-21 7:24 ` [PATCH 2/2] Input: fix use-after-free introduced with dynamic minor changes Dmitry Torokhov @ 2012-10-21 7:39 ` Al Viro 2012-10-21 8:13 ` Dmitry Torokhov 2012-10-22 0:57 ` [PATCH 1/2] char_dev: pin parent kobject Dmitry Torokhov 1 sibling, 2 replies; 8+ messages in thread From: Al Viro @ 2012-10-21 7:39 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Dave Jones, Linus Torvalds, linux-kernel, linux-input On Sun, Oct 21, 2012 at 12:24:30AM -0700, Dmitry Torokhov wrote: > In certain cases (for example when a cdev structure is embedded into > another object whose lifetime is controlled by a separate device object) > it is beneficial to tie lifetime of another struct device to the lifetime > of character device so that related object is not freed until after > char_dev object is freed. To achieve this allow setting a "parent" device > for character devices and pin them when doing cdev_add() and unpin when > last reference to cdev structure is being released. Why struct device and not simply struct kobject? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] char_dev: allow setting up and pinning parent devices 2012-10-21 7:39 ` [PATCH 1/2] char_dev: allow setting up and pinning parent devices Al Viro @ 2012-10-21 8:13 ` Dmitry Torokhov 2012-10-22 0:57 ` [PATCH 1/2] char_dev: pin parent kobject Dmitry Torokhov 1 sibling, 0 replies; 8+ messages in thread From: Dmitry Torokhov @ 2012-10-21 8:13 UTC (permalink / raw) To: Al Viro; +Cc: Dave Jones, Linus Torvalds, linux-kernel, linux-input On Sun, Oct 21, 2012 at 08:39:28AM +0100, Al Viro wrote: > On Sun, Oct 21, 2012 at 12:24:30AM -0700, Dmitry Torokhov wrote: > > In certain cases (for example when a cdev structure is embedded into > > another object whose lifetime is controlled by a separate device object) > > it is beneficial to tie lifetime of another struct device to the lifetime > > of character device so that related object is not freed until after > > char_dev object is freed. To achieve this allow setting a "parent" device > > for character devices and pin them when doing cdev_add() and unpin when > > last reference to cdev structure is being released. > > Why struct device and not simply struct kobject? It was more convenient for my uses and I also think that parents of character devices will be struct devices... I however will not insist and if you prefer using more generic kobject I can change it, Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] char_dev: pin parent kobject 2012-10-21 7:39 ` [PATCH 1/2] char_dev: allow setting up and pinning parent devices Al Viro 2012-10-21 8:13 ` Dmitry Torokhov @ 2012-10-22 0:57 ` Dmitry Torokhov 2012-10-22 0:57 ` [PATCH 2/2] Input: fix use-after-free introduced with dynamic minor changes Dmitry Torokhov 2012-10-22 5:02 ` [PATCH 1/2] char_dev: pin parent kobject Linus Torvalds 1 sibling, 2 replies; 8+ messages in thread From: Dmitry Torokhov @ 2012-10-22 0:57 UTC (permalink / raw) To: Al Viro, Dave Jones; +Cc: Linus Torvalds, linux-kernel, linux-input In certain cases (for example when a cdev structure is embedded into another object whose lifetime is controlled by a separate kobject) it is beneficial to tie lifetime of another object to the lifetime of character device so that related object is not freed until after char_dev object is freed. To achieve this let's pin kobject's parent when doing cdev_add() and unpin when last reference to cdev structure is being released. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- OK, so it looks like cdev kobject's parent is unused so we can just have users set themselves as parents of character device and have cdev_add() pin it. fs/char_dev.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/fs/char_dev.c b/fs/char_dev.c index 3f152b9..afc2bb6 100644 --- a/fs/char_dev.c +++ b/fs/char_dev.c @@ -471,9 +471,19 @@ static int exact_lock(dev_t dev, void *data) */ int cdev_add(struct cdev *p, dev_t dev, unsigned count) { + int error; + p->dev = dev; p->count = count; - return kobj_map(cdev_map, dev, count, NULL, exact_match, exact_lock, p); + + error = kobj_map(cdev_map, dev, count, NULL, + exact_match, exact_lock, p); + if (error) + return error; + + kobject_get(p->kobj.parent); + + return 0; } static void cdev_unmap(dev_t dev, unsigned count) @@ -498,14 +508,20 @@ void cdev_del(struct cdev *p) static void cdev_default_release(struct kobject *kobj) { struct cdev *p = container_of(kobj, struct cdev, kobj); + struct kobject *parent = kobj->parent; + cdev_purge(p); + kobject_put(parent); } static void cdev_dynamic_release(struct kobject *kobj) { struct cdev *p = container_of(kobj, struct cdev, kobj); + struct kobject *parent = kobj->parent; + cdev_purge(p); kfree(p); + kobject_put(parent); } static struct kobj_type ktype_cdev_default = { -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] Input: fix use-after-free introduced with dynamic minor changes 2012-10-22 0:57 ` [PATCH 1/2] char_dev: pin parent kobject Dmitry Torokhov @ 2012-10-22 0:57 ` Dmitry Torokhov 2012-10-22 5:02 ` [PATCH 1/2] char_dev: pin parent kobject Linus Torvalds 1 sibling, 0 replies; 8+ messages in thread From: Dmitry Torokhov @ 2012-10-22 0:57 UTC (permalink / raw) To: Al Viro, Dave Jones; +Cc: Linus Torvalds, linux-kernel, linux-input Commit 7f8d4cad1e4e11a45d02bd6e024cc2812963c38a made evdev, joydev and mousedev to embed struct cdev into their respective structures representing input devices. Unfortunately character device structure may outlive the parent structure unless we do not set it up as parent of character device so that it will stay pinned until character device is freed. Also, now that parent structure is pinned while character device exists we do not need to pin and unpin it every time user opens or closes it. Reported-by: Dave Jones <davej@redhat.com> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- 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 6ae2ac4..f0f8928 100644 --- a/drivers/input/evdev.c +++ b/drivers/input/evdev.c @@ -292,7 +292,6 @@ static int evdev_release(struct inode *inode, struct file *file) kfree(client); evdev_close_device(evdev); - put_device(&evdev->dev); return 0; } @@ -331,7 +330,6 @@ static int evdev_open(struct inode *inode, struct file *file) file->private_data = client; nonseekable_open(inode, file); - get_device(&evdev->dev); return 0; err_free_client: @@ -1001,6 +999,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); if (error) goto err_unregister_handle; diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c index 63e5916..9c7526d 100644 --- a/drivers/input/joydev.c +++ b/drivers/input/joydev.c @@ -262,7 +262,6 @@ static int joydev_release(struct inode *inode, struct file *file) kfree(client); joydev_close_device(joydev); - put_device(&joydev->dev); return 0; } @@ -289,7 +288,6 @@ static int joydev_open(struct inode *inode, struct file *file) file->private_data = client; nonseekable_open(inode, file); - get_device(&joydev->dev); return 0; err_free_client: @@ -877,6 +875,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); if (error) goto err_unregister_handle; diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c index a1b4c37..8f02e3d 100644 --- a/drivers/input/mousedev.c +++ b/drivers/input/mousedev.c @@ -523,7 +523,6 @@ static int mousedev_release(struct inode *inode, struct file *file) kfree(client); mousedev_close_device(mousedev); - put_device(&mousedev->dev); return 0; } @@ -558,7 +557,6 @@ static int mousedev_open(struct inode *inode, struct file *file) file->private_data = client; nonseekable_open(inode, file); - get_device(&mousedev->dev); return 0; err_free_client: @@ -892,6 +890,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); if (error) goto err_unregister_handle; -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] char_dev: pin parent kobject 2012-10-22 0:57 ` [PATCH 1/2] char_dev: pin parent kobject Dmitry Torokhov 2012-10-22 0:57 ` [PATCH 2/2] Input: fix use-after-free introduced with dynamic minor changes Dmitry Torokhov @ 2012-10-22 5:02 ` Linus Torvalds 2012-10-22 5:42 ` Al Viro 1 sibling, 1 reply; 8+ messages in thread From: Linus Torvalds @ 2012-10-22 5:02 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Al Viro, Dave Jones, linux-kernel, linux-input This series looks good to me, and looks to fix the issue about as cleanly as it can. Al, holler if you have any concerns, but I'm planning on applying it ASAP, Linus On Mon, Oct 22, 2012 at 3:57 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > In certain cases (for example when a cdev structure is embedded into > another object whose lifetime is controlled by a separate kobject) it is > beneficial to tie lifetime of another object to the lifetime of character > device so that related object is not freed until after char_dev object is > freed. To achieve this let's pin kobject's parent when doing cdev_add() > and unpin when last reference to cdev structure is being released. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] char_dev: pin parent kobject 2012-10-22 5:02 ` [PATCH 1/2] char_dev: pin parent kobject Linus Torvalds @ 2012-10-22 5:42 ` Al Viro 0 siblings, 0 replies; 8+ messages in thread From: Al Viro @ 2012-10-22 5:42 UTC (permalink / raw) To: Linus Torvalds; +Cc: Dmitry Torokhov, Dave Jones, linux-kernel, linux-input On Mon, Oct 22, 2012 at 08:02:12AM +0300, Linus Torvalds wrote: > This series looks good to me, and looks to fix the issue about as > cleanly as it can. Al, holler if you have any concerns, but I'm > planning on applying it ASAP, Add my ACKed-by. And from the look of it, we'll need something similar at least in drivers/mtd/ubi in addition to evdev patch. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-10-22 5:42 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20121019175046.GP2616@ZenIV.linux.org.uk>
2012-10-21 7:24 ` [PATCH 1/2] char_dev: allow setting up and pinning parent devices Dmitry Torokhov
2012-10-21 7:24 ` [PATCH 2/2] Input: fix use-after-free introduced with dynamic minor changes Dmitry Torokhov
2012-10-21 7:39 ` [PATCH 1/2] char_dev: allow setting up and pinning parent devices Al Viro
2012-10-21 8:13 ` Dmitry Torokhov
2012-10-22 0:57 ` [PATCH 1/2] char_dev: pin parent kobject Dmitry Torokhov
2012-10-22 0:57 ` [PATCH 2/2] Input: fix use-after-free introduced with dynamic minor changes Dmitry Torokhov
2012-10-22 5:02 ` [PATCH 1/2] char_dev: pin parent kobject Linus Torvalds
2012-10-22 5:42 ` Al Viro
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).