| Message ID | 147089096242.9037.17705831938180576705.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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! -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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(-) -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html