* [PATCH] fs/char_dev: fix cdev_put() vs f_op->release() use-after-free
@ 2016-08-11 4:49 Dan Williams
2016-08-11 5:16 ` Al Viro
0 siblings, 1 reply; 3+ messages in thread
From: Dan Williams @ 2016-08-11 4:49 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel, linux-kernel, linux-nvdimm
drivers/dax/dax.c implements a character device that supports mmap().
While trying to convert it to use the cdev api a unit test started
failing. The test effectively does the following to test that the driver
revokes active mappings when the device is unregistered:
fd = open("/dev/dax0.0", ...);
mmap(..., fd, ...);
<twiddle sysfs to trigger device_unregister()>;
exit();
...results in this crash:
dax dax0.0: dax_dev_vm_close
dax dax0.0: dax_dev_release: ffff880338afd298
dax_dev_free: ffff880338afd298
general protection fault: 0000 [#1] SMP
[..]
Call Trace:
[<ffffffff8128e2c0>] cdev_put+0x20/0x30
[<ffffffff8128b4db>] __fput+0x1ab/0x1f0
[<ffffffff8128b55e>] ____fput+0xe/0x10
[<ffffffff810d371e>] task_work_run+0x7e/0xa0
[<ffffffff810b41b2>] do_exit+0x302/0xc10
Where dax_dev_release() is the f_op->release() method, and is
implemented to simply drop the final references on our driver objects:
struct dax_dev *dax_dev = filp->private_data;
struct device *dev = dax_dev->dev;
dev_dbg(dax_dev->dev, "%s\n", __func__);
put_device(dev);
dax_dev_put(dax_dev);
The dax_dev object embeds a 'struct cdev' which means f_op->release()
may free cdev, so __fput() needs to drop the cdev reference before
calling f_op->release().
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
fs/file_table.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/file_table.c b/fs/file_table.c
index ad17e05ebf95..8856b8008248 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -204,13 +204,13 @@ static void __fput(struct file *file)
file->f_op->fasync(-1, file, 0);
}
ima_file_free(file);
- if (file->f_op->release)
- file->f_op->release(inode, file);
- security_file_free(file);
if (unlikely(S_ISCHR(inode->i_mode) && inode->i_cdev != NULL &&
!(file->f_mode & FMODE_PATH))) {
cdev_put(inode->i_cdev);
}
+ if (file->f_op->release)
+ file->f_op->release(inode, file);
+ security_file_free(file);
fops_put(file->f_op);
put_pid(file->f_owner.pid);
if ((file->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] fs/char_dev: fix cdev_put() vs f_op->release() use-after-free
2016-08-11 4:49 [PATCH] fs/char_dev: fix cdev_put() vs f_op->release() use-after-free Dan Williams
@ 2016-08-11 5:16 ` Al Viro
2016-08-11 7:24 ` Dan Williams
0 siblings, 1 reply; 3+ messages in thread
From: Al Viro @ 2016-08-11 5:16 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-fsdevel, linux-kernel, linux-nvdimm
On Wed, Aug 10, 2016 at 09:49:22PM -0700, Dan Williams wrote:
> Where dax_dev_release() is the f_op->release() method, and is
> implemented to simply drop the final references on our driver objects:
>
> struct dax_dev *dax_dev = filp->private_data;
> struct device *dev = dax_dev->dev;
>
> dev_dbg(dax_dev->dev, "%s\n", __func__);
> put_device(dev);
> dax_dev_put(dax_dev);
>
> The dax_dev object embeds a 'struct cdev' which means f_op->release()
> may free cdev, so __fput() needs to drop the cdev reference before
> calling f_op->release().
NAK. You *can't* free a structure that contains kobj with currently
positive refcount. Ever. If you embed a struct kobj into something,
you must use the refcount of that kobj (or one of its ancestors) to
control the lifetime of containing object. If your dax_dev_put() can
trigger freeing of dax_dev despite the still-positive refcount of
embedded cdev.kobj, it is fundamentally broken.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] fs/char_dev: fix cdev_put() vs f_op->release() use-after-free
2016-08-11 5:16 ` Al Viro
@ 2016-08-11 7:24 ` Dan Williams
0 siblings, 0 replies; 3+ messages in thread
From: Dan Williams @ 2016-08-11 7:24 UTC (permalink / raw)
To: Al Viro; +Cc: linux-fsdevel, linux-kernel, linux-nvdimm
On Wed, Aug 10, 2016 at 10:16 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Aug 10, 2016 at 09:49:22PM -0700, Dan Williams wrote:
>
>> Where dax_dev_release() is the f_op->release() method, and is
>> implemented to simply drop the final references on our driver objects:
>>
>> struct dax_dev *dax_dev = filp->private_data;
>> struct device *dev = dax_dev->dev;
>>
>> dev_dbg(dax_dev->dev, "%s\n", __func__);
>> put_device(dev);
>> dax_dev_put(dax_dev);
>>
>> The dax_dev object embeds a 'struct cdev' which means f_op->release()
>> may free cdev, so __fput() needs to drop the cdev reference before
>> calling f_op->release().
>
> NAK. You *can't* free a structure that contains kobj with currently
> positive refcount. Ever. If you embed a struct kobj into something,
> you must use the refcount of that kobj (or one of its ancestors) to
> control the lifetime of containing object. If your dax_dev_put() can
> trigger freeing of dax_dev despite the still-positive refcount of
> embedded cdev.kobj, it is fundamentally broken.
Ah, ok. I missed that cdev_put() drops a parent kobj ref, NULL in my
case. So that "put_device(dev)" above can just be delegated to
cdev_put() and I can remove the kref behind dax_dev_put(). Thank you
for straightening me out!
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-08-11 7:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-11 4:49 [PATCH] fs/char_dev: fix cdev_put() vs f_op->release() use-after-free Dan Williams
2016-08-11 5:16 ` Al Viro
2016-08-11 7:24 ` Dan Williams
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).