[RFC PATCH 0/3] debugfs: make __debugfs_remove wait for dentry release

[RFC PATCH 0/3] debugfs: make __debugfs_remove wait for dentry release

[Roman Pen]

Hello.

Recently I observed that all over the sources people expect that debugfs_remove()
should behave as a barrier, hence file operations won't be invoked after it is
called and it is safe to free remain memory.

But by default debugfs_remove() does not guarantee that dentry will be released,
and after this call fops->open/read/write can still be invoked if someone holds
the reference on dentry because of successfull lookup.

For example here is the grep output:

   *** drivers/block/pktcdvd.c:
       pkt_debugfs_dev_remove[489]    debugfs_remove(pd->dfs_f_info);
       pkt_debugfs_dev_remove[490]    debugfs_remove(pd->dfs_d_root);

   *** drivers/char/virtio_console.c:
       unplug_port[1595]              debugfs_remove(port->debugfs_file);

   *** drivers/crypto/qat/qat_common/adf_cfg.c:
       adf_cfg_dev_remove[187]        debugfs_remove(dev_cfg_data->debug);

   *** drivers/gpu/drm/drm_debugfs.c:
       drm_debugfs_remove_files[203]  debugfs_remove(tmp->dent);

  .... and more and more and more ...

where people do the following sequence:

      debugfs_remove(dev->debugfs_dentry);
      kfree(dev);

and 'dev' pointer was passed to 'debugfs_create_file("my_dev", , dev, dev_fops)',
so any access to 'dev' in file operations can lead to usage-after-free.

In this patch __debugfs_remove() waits for last dentry release callback.

BUT! I am not sure that nobody tries to remove the dentry from it's own file
operation (dentry suicide).  And if so - deadlock will happen.

Probably, dentry_remove_self() should be implemented for such cases, which is
similar to sysfs_remove_file_self().  But for now I do not want to add new
function which can be useless in the nearest future.

Also I did a fix for automount inodes and increased i_nlink references because
of WARNING at fs/inode.c:273 drop_nlink.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org

Roman Pen (3):
  debugfs: fix automount inode i_nlink references
  debugfs: put private data to i_private for automount inode
  debugfs: make __debugfs_remove wait for dentry release

 fs/debugfs/inode.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 7 deletions(-)

-- 
2.6.2

[RFC PATCH 1/3] debugfs: fix automount inode i_nlink references

[Roman Pen]

Directory inodes should start off with i_nlink == 2 (for "." entry).
Of course the same rule should be applied to automount dentries for
child and parent inodes as well.

Also now automount dentry does fsnotify_mkdir.

Without this patch kernel complains when sees i_nlink == 0:

  [   86.288070] WARNING: CPU: 1 PID: 3616 at fs/inode.c:273 drop_nlink+0x3e/0x50()
  [   86.288461] Modules linked in: debugfs_example2(O-)
  [   86.288745] CPU: 1 PID: 3616 Comm: rmmod Tainted: G           O    4.4.0-rc3-next-20151207+ #135
  [   86.289197] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.2-20150617_082717-anatol 04/01/2014
  [   86.289696]  ffffffff81be05c9 ffff8800b9e6fda0 ffffffff81352e2c 0000000000000000
  [   86.290110]  ffff8800b9e6fdd8 ffffffff81065142 ffff8801399175e8 ffff8800bb78b240
  [   86.290507]  ffff8801399175e8 ffff8800b73d7898 ffff8800b73d7840 ffff8800b9e6fde8
  [   86.290933] Call Trace:
  [   86.291080]  [<ffffffff81352e2c>] dump_stack+0x4e/0x82
  [   86.291340]  [<ffffffff81065142>] warn_slowpath_common+0x82/0xc0
  [   86.291640]  [<ffffffff8106523a>] warn_slowpath_null+0x1a/0x20
  [   86.291932]  [<ffffffff811ae62e>] drop_nlink+0x3e/0x50
  [   86.292208]  [<ffffffff811ba35b>] simple_unlink+0x4b/0x60
  [   86.292481]  [<ffffffff811ba3a7>] simple_rmdir+0x37/0x50
  [   86.292748]  [<ffffffff812d9808>] __debugfs_remove.part.16+0xa8/0xd0
  [   86.293082]  [<ffffffff812d9a0b>] debugfs_remove_recursive+0xdb/0x1c0
  [   86.293406]  [<ffffffffa00004dd>] cleanup_module+0x2d/0x3b [debugfs_example2]
  [   86.293762]  [<ffffffff810d959b>] SyS_delete_module+0x16b/0x220
  [   86.294077]  [<ffffffff818ef857>] entry_SYSCALL_64_fastpath+0x12/0x6a
  [   86.294405] ---[ end trace c9fc53353fe14a36 ]---
  [   86.294639] ------------[ cut here ]------------
  [   86.294871] WARNING: CPU: 1 PID: 3616 at fs/inode.c:273 drop_nlink+0x3e/0x50()
  [   86.295249] Modules linked in: debugfs_example2(O-)
  [   86.295510] CPU: 1 PID: 3616 Comm: rmmod Tainted: G        W  O    4.4.0-rc3-next-20151207+ #135
  [   86.295943] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.2-20150617_082717-anatol 04/01/2014
  [   86.296450]  ffffffff81be05c9 ffff8800b9e6fda0 ffffffff81352e2c 0000000000000000
  [   86.296846]  ffff8800b9e6fdd8 ffffffff81065142 ffff880139917810 ffff8800b73d7840
  [   86.297253]  ffff880139917810 ffff8800b73d7898 ffff8800b73d7840 ffff8800b9e6fde8
  [   86.297650] Call Trace:
  [   86.297777]  [<ffffffff81352e2c>] dump_stack+0x4e/0x82
  [   86.298043]  [<ffffffff81065142>] warn_slowpath_common+0x82/0xc0
  [   86.298344]  [<ffffffff8106523a>] warn_slowpath_null+0x1a/0x20
  [   86.298636]  [<ffffffff811ae62e>] drop_nlink+0x3e/0x50
  [   86.298893]  [<ffffffff811ba35b>] simple_unlink+0x4b/0x60
  [   86.299174]  [<ffffffff811ba3a7>] simple_rmdir+0x37/0x50
  [   86.299442]  [<ffffffff812d9808>] __debugfs_remove.part.16+0xa8/0xd0
  [   86.299760]  [<ffffffff812d9aaf>] debugfs_remove_recursive+0x17f/0x1c0
  [   86.300095]  [<ffffffffa00004dd>] cleanup_module+0x2d/0x3b [debugfs_example2]
  [   86.300453]  [<ffffffff810d959b>] SyS_delete_module+0x16b/0x220
  [   86.300750]  [<ffffffff818ef857>] entry_SYSCALL_64_fastpath+0x12/0x6a
  [   86.301198] ---[ end trace c9fc53353fe14a37 ]---

Signed-off-by: Roman Pen <r.peniaev@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org
---
 fs/debugfs/inode.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index b7fcc0d..7bd7e19 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -461,7 +461,11 @@ struct dentry *debugfs_create_automount(const char *name,
 	inode->i_flags |= S_AUTOMOUNT;
 	inode->i_private = data;
 	dentry->d_fsdata = (void *)f;
+	/* directory inodes start off with i_nlink == 2 (for "." entry) */
+	inc_nlink(inode);
 	d_instantiate(dentry, inode);
+	inc_nlink(d_inode(dentry->d_parent));
+	fsnotify_mkdir(d_inode(dentry->d_parent), dentry);
 	return end_creating(dentry);
 }
 EXPORT_SYMBOL(debugfs_create_automount);
-- 
2.6.2

Re: [RFC PATCH 1/3] debugfs: fix automount inode i_nlink references

[Greg Kroah-Hartman]

On Thu, Dec 10, 2015 at 01:47:12PM +0100, Roman Pen wrote:
> Directory inodes should start off with i_nlink == 2 (for "." entry).
> Of course the same rule should be applied to automount dentries for
> child and parent inodes as well.
> 
> Also now automount dentry does fsnotify_mkdir.
> 
> Without this patch kernel complains when sees i_nlink == 0:

How can the kernel see this?  What did you do to trigger this?

thanks,

greg k-h

Re: [RFC PATCH 1/3] debugfs: fix automount inode i_nlink references

[Roman Peniaev]

On Mon, Feb 8, 2016 at 7:38 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Dec 10, 2015 at 01:47:12PM +0100, Roman Pen wrote:
>> Directory inodes should start off with i_nlink == 2 (for "." entry).
>> Of course the same rule should be applied to automount dentries for
>> child and parent inodes as well.
>>
>> Also now automount dentry does fsnotify_mkdir.
>>
>> Without this patch kernel complains when sees i_nlink == 0:
>
> How can the kernel see this?  What did you do to trigger this?

Yes, sorry, I had to be more precise on this.
That happens on unlinking of automount dentry.

Easily can be reproduced:

    autom = debugfs_create_automount("automount", parentd, vfsmount_cb, data);
    BUG_ON(IS_ERR_OR_NULL(autom));
    debugfs_remove(autom);

You will immediately see one warning on attempt to drop_nlink() (which is zero)
for automount dentry.

The second warning happens when you unlink 'parentd', because
debugfs_create_automount() did not increase the nlink for parent
inode.

Do I need to resend this patch with more precise description?

--
Roman

>
> thanks,
>
> greg k-h

Re: [RFC PATCH 1/3] debugfs: fix automount inode i_nlink references

[Greg Kroah-Hartman]

On Mon, Feb 08, 2016 at 11:28:52AM +0100, Roman Peniaev wrote:
> On Mon, Feb 8, 2016 at 7:38 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, Dec 10, 2015 at 01:47:12PM +0100, Roman Pen wrote:
> >> Directory inodes should start off with i_nlink == 2 (for "." entry).
> >> Of course the same rule should be applied to automount dentries for
> >> child and parent inodes as well.
> >>
> >> Also now automount dentry does fsnotify_mkdir.
> >>
> >> Without this patch kernel complains when sees i_nlink == 0:
> >
> > How can the kernel see this?  What did you do to trigger this?
> 
> Yes, sorry, I had to be more precise on this.
> That happens on unlinking of automount dentry.
> 
> Easily can be reproduced:
> 
>     autom = debugfs_create_automount("automount", parentd, vfsmount_cb, data);
>     BUG_ON(IS_ERR_OR_NULL(autom));
>     debugfs_remove(autom);
> 
> You will immediately see one warning on attempt to drop_nlink() (which is zero)
> for automount dentry.

Why don't we see this "in the wild" today with the one user of this
function?

> The second warning happens when you unlink 'parentd', because
> debugfs_create_automount() did not increase the nlink for parent
> inode.
> 
> Do I need to resend this patch with more precise description?

Yes, please fix up and resend as a stand-alone patch, as it is
independant of your other proposal.

And take off the "RFC" marking, I can never apply a patch with that type
of marking as you obviously don't think it is good enough to be merged,
so why would I?  :)

thanks,

greg k-h

Re: [RFC PATCH 1/3] debugfs: fix automount inode i_nlink references

[Roman Peniaev]

On Mon, Feb 8, 2016 at 5:35 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Feb 08, 2016 at 11:28:52AM +0100, Roman Peniaev wrote:
>> On Mon, Feb 8, 2016 at 7:38 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Thu, Dec 10, 2015 at 01:47:12PM +0100, Roman Pen wrote:
>> >> Directory inodes should start off with i_nlink == 2 (for "." entry).
>> >> Of course the same rule should be applied to automount dentries for
>> >> child and parent inodes as well.
>> >>
>> >> Also now automount dentry does fsnotify_mkdir.
>> >>
>> >> Without this patch kernel complains when sees i_nlink == 0:
>> >
>> > How can the kernel see this?  What did you do to trigger this?
>>
>> Yes, sorry, I had to be more precise on this.
>> That happens on unlinking of automount dentry.
>>
>> Easily can be reproduced:
>>
>>     autom = debugfs_create_automount("automount", parentd, vfsmount_cb, data);
>>     BUG_ON(IS_ERR_OR_NULL(autom));
>>     debugfs_remove(autom);
>>
>> You will immediately see one warning on attempt to drop_nlink() (which is zero)
>> for automount dentry.
>
> Why don't we see this "in the wild" today with the one user of this
> function?

I see only one current user: tracefs.
tracefs does not remove the automount root dentry.
That's the reason.

>
>> The second warning happens when you unlink 'parentd', because
>> debugfs_create_automount() did not increase the nlink for parent
>> inode.
>>
>> Do I need to resend this patch with more precise description?
>
> Yes, please fix up and resend as a stand-alone patch, as it is
> independant of your other proposal.
>
> And take off the "RFC" marking, I can never apply a patch with that type
> of marking as you obviously don't think it is good enough to be merged,
> so why would I?  :)

I've resent a patch.
Please, take a look.

--
Roman

[RFC PATCH 2/3] debugfs: put private data to i_private for automount inode

[Roman Pen]

I will need dentry->d_fsdata in the next patch.
Keep 'd_fsdata' for debugfs needs.

Signed-off-by: Roman Pen <r.peniaev@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org
---
 fs/debugfs/inode.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 7bd7e19..a1d077a 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -170,6 +170,8 @@ static void debugfs_evict_inode(struct inode *inode)
 	clear_inode(inode);
 	if (S_ISLNK(inode->i_mode))
 		kfree(inode->i_link);
+	else if (S_ISDIR(inode->i_mode) && IS_AUTOMOUNT(inode))
+		kfree(inode->i_private);
 }
 
 static const struct super_operations debugfs_super_operations = {
@@ -179,11 +181,16 @@ static const struct super_operations debugfs_super_operations = {
 	.evict_inode	= debugfs_evict_inode,
 };
 
+struct automount_priv {
+	struct vfsmount *(*func)(void *);
+	void *data;
+};
+
 static struct vfsmount *debugfs_automount(struct path *path)
 {
-	struct vfsmount *(*f)(void *);
-	f = (struct vfsmount *(*)(void *))path->dentry->d_fsdata;
-	return f(d_inode(path->dentry)->i_private);
+	struct automount_priv *p = d_inode(path->dentry)->i_private;
+
+	return p->func(p->data);
 }
 
 static const struct dentry_operations debugfs_dops = {
@@ -448,19 +455,28 @@ struct dentry *debugfs_create_automount(const char *name,
 					void *data)
 {
 	struct dentry *dentry = start_creating(name, parent);
+	struct automount_priv *p;
 	struct inode *inode;
 
 	if (IS_ERR(dentry))
 		return NULL;
 
+	p = kmalloc(sizeof(*p), GFP_KERNEL);
+	if (unlikely(!p))
+		return failed_creating(dentry);
+
+	p->func = f;
+	p->data = data;
+
 	inode = debugfs_get_inode(dentry->d_sb);
-	if (unlikely(!inode))
+	if (unlikely(!inode)) {
+		kfree(p);
 		return failed_creating(dentry);
+	}
 
 	inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
 	inode->i_flags |= S_AUTOMOUNT;
-	inode->i_private = data;
-	dentry->d_fsdata = (void *)f;
+	inode->i_private = p;
 	/* directory inodes start off with i_nlink == 2 (for "." entry) */
 	inc_nlink(inode);
 	d_instantiate(dentry, inode);
-- 
2.6.2

[RFC PATCH 3/3] debugfs: make __debugfs_remove wait for dentry release

[Roman Pen]

__debugfs_remove does not wait for dentry release, thus dentry can still be
alive and file operations can still be invoked after the function returns.

>From debugfs point of view this behaviour is definitely ok, but that can be
critical for users of debugfs and lead to usage-after-free: file operations
can be called after dentry is considered as removed.

Simple grep over the sources shows that dynamic debugfs file creation and
removal is exactly the case, and common usage is the following:

  create_dev():
      dev = kmalloc();
      dev->debugfs_dentry = debugfs_create_file("my_dev", , dev, dev_fops);
                                                            ^^^
                                            !! pointer is passed to file
                                            !! operations as private data

  remove_dev(dev):
      debugfs_remove(dev->debugfs_dentry);
      kfree(dev);
            ^^^
      !! memory is freed, but fops->open/read/write
      !! can still be called and lead to usage-after-free

Here is quick grep output of the case described above:

   *** drivers/block/pktcdvd.c:
       pkt_debugfs_dev_remove[489]    debugfs_remove(pd->dfs_f_info);
       pkt_debugfs_dev_remove[490]    debugfs_remove(pd->dfs_d_root);

   *** drivers/char/virtio_console.c:
       unplug_port[1595]              debugfs_remove(port->debugfs_file);

   *** drivers/crypto/qat/qat_common/adf_cfg.c:
       adf_cfg_dev_remove[187]        debugfs_remove(dev_cfg_data->debug);

   *** drivers/gpu/drm/drm_debugfs.c:
       drm_debugfs_remove_files[203]  debugfs_remove(tmp->dent);

  .... and more and more and more ...

In my grep output each third line is exactly this case: people expect that
debugfs_remove() is a barrier and file operations won't be invoked after it
(same behaviour as kobject_del(),kobject_put() tuple).

So in this patch debugfs_remove() waits for completion of final dentry release
callback.

BUT! I am not sure that nobody tries to remove the dentry from it's own file
operation (dentry suicide).  And if so - deadlock will happen.

Probably, dentry_remove_self() should be implemented for such cases, which is
similar to sysfs_remove_file_self().  But for now I do not want to add new
function which can be useless in the nearest future.

Signed-off-by: Roman Pen <r.peniaev@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org
---
 fs/debugfs/inode.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index a1d077a..2525158 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -193,9 +193,23 @@ static struct vfsmount *debugfs_automount(struct path *path)
 	return p->func(p->data);
 }
 
+static void debugfs_release(struct dentry *dentry)
+{
+	struct completion *comp;
+
+	/* Paired with __debugfs_remove */
+	smp_rmb();
+	comp = dentry->d_fsdata;
+	if (likely(comp)) {
+		dentry->d_fsdata = NULL;
+		complete(comp);
+	}
+}
+
 static const struct dentry_operations debugfs_dops = {
 	.d_delete = always_delete_dentry,
 	.d_automount = debugfs_automount,
+	.d_release = debugfs_release,
 };
 
 static int debug_fill_super(struct super_block *sb, void *data, int silent)
@@ -542,14 +556,24 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent)
 	int ret = 0;
 
 	if (simple_positive(dentry)) {
+		struct completion comp;
+
+		init_completion(&comp);
 		dget(dentry);
 		if (d_is_dir(dentry))
 			ret = simple_rmdir(d_inode(parent), dentry);
 		else
 			simple_unlink(d_inode(parent), dentry);
-		if (!ret)
+		if (likely(!ret)) {
 			d_delete(dentry);
+			dentry->d_fsdata = &comp;
+			/* Paired with debugfs_release callback */
+			smp_wmb();
+		}
 		dput(dentry);
+
+		if (likely(!ret))
+			wait_for_completion(&comp);
 	}
 	return ret;
 }
-- 
2.6.2

Re: [RFC PATCH 3/3] debugfs: make __debugfs_remove wait for dentry release

[Greg Kroah-Hartman]

On Thu, Dec 10, 2015 at 01:47:14PM +0100, Roman Pen wrote:
> __debugfs_remove does not wait for dentry release, thus dentry can still be
> alive and file operations can still be invoked after the function returns.
> 
> >From debugfs point of view this behaviour is definitely ok, but that can be
> critical for users of debugfs and lead to usage-after-free: file operations
> can be called after dentry is considered as removed.
> 
> Simple grep over the sources shows that dynamic debugfs file creation and
> removal is exactly the case, and common usage is the following:
> 
>   create_dev():
>       dev = kmalloc();
>       dev->debugfs_dentry = debugfs_create_file("my_dev", , dev, dev_fops);
>                                                             ^^^
>                                             !! pointer is passed to file
>                                             !! operations as private data
> 
>   remove_dev(dev):
>       debugfs_remove(dev->debugfs_dentry);
>       kfree(dev);
>             ^^^
>       !! memory is freed, but fops->open/read/write
>       !! can still be called and lead to usage-after-free
> 
> Here is quick grep output of the case described above:
> 
>    *** drivers/block/pktcdvd.c:
>        pkt_debugfs_dev_remove[489]    debugfs_remove(pd->dfs_f_info);
>        pkt_debugfs_dev_remove[490]    debugfs_remove(pd->dfs_d_root);
> 
>    *** drivers/char/virtio_console.c:
>        unplug_port[1595]              debugfs_remove(port->debugfs_file);
> 
>    *** drivers/crypto/qat/qat_common/adf_cfg.c:
>        adf_cfg_dev_remove[187]        debugfs_remove(dev_cfg_data->debug);
> 
>    *** drivers/gpu/drm/drm_debugfs.c:
>        drm_debugfs_remove_files[203]  debugfs_remove(tmp->dent);
> 
>   .... and more and more and more ...
> 
> In my grep output each third line is exactly this case: people expect that
> debugfs_remove() is a barrier and file operations won't be invoked after it
> (same behaviour as kobject_del(),kobject_put() tuple).
> 
> So in this patch debugfs_remove() waits for completion of final dentry release
> callback.
> 
> BUT! I am not sure that nobody tries to remove the dentry from it's own file
> operation (dentry suicide).  And if so - deadlock will happen.
> 
> Probably, dentry_remove_self() should be implemented for such cases, which is
> similar to sysfs_remove_file_self().  But for now I do not want to add new
> function which can be useless in the nearest future.
> 
> Signed-off-by: Roman Pen <r.peniaev@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-kernel@vger.kernel.org
> ---
>  fs/debugfs/inode.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)

Without a real user, I don't want to take this, as it's "odd".  Also
this is debugfs, you shouldn't be using this for any "real" code, it's
only for debugging.  If you want this for a real api, use configfs.

thanks,

greg k-h