[PATCH] blktrace: fix race with open trace files and directory removal

[PATCH] blktrace: fix race with open trace files and directory removal

[Jeff Mahoney]

There's a bug in the blktrace client where it will stop and tear down
all of the tracing instances for devices it's opened whether it
successfully completed the setup or not.

By starting multiple blktrace processes on the same device, it's possible
to permanently disable blktrace on that device. The cause is that when
the first blktrace process to exit tears down the directory structure,
the trace files are still held open. Debugfs removes the dentries for the
open files just fine but the relay implementation doesn't remove the
dentries until all of the references to the file are dropped. This means
that if there are open files when debugfs_remove is called for the device
directory, the directory is not empty and can't be removed. Since the
shutdown of the blktrace structure xchg's the structure out, there's no
way to clean up the directory and any new blktrace processes will fail
to start because it can't create the directory.

This patch adds a kref to blk_trace so that we can release it after the
initial reference as well as all of the references accumulated by the
relay files are dropped.

I have a fix for the blktrace client to avoid the situation on older
kernels as well.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 include/linux/blktrace_api.h |    2 ++
 kernel/trace/blktrace.c      |   37 +++++++++++++++++++++++++++++++++----
 2 files changed, 35 insertions(+), 4 deletions(-)

--- a/include/linux/blktrace_api.h	2013-09-24 22:39:53.342710250 -0400
+++ b/include/linux/blktrace_api.h	2013-09-24 22:55:11.376936518 -0400
@@ -4,6 +4,7 @@
 #include <linux/blkdev.h>
 #include <linux/relay.h>
 #include <linux/compat.h>
+#include <linux/kref.h>
 #include <uapi/linux/blktrace_api.h>
 
 #if defined(CONFIG_BLK_DEV_IO_TRACE)
@@ -24,6 +25,7 @@ struct blk_trace {
 	struct dentry *dropped_file;
 	struct dentry *msg_file;
 	atomic_t dropped;
+	struct kref kref;
 };
 
 extern int blk_trace_ioctl(struct block_device *, unsigned, char __user *);
--- a/kernel/trace/blktrace.c	2013-09-24 22:39:53.342710250 -0400
+++ b/kernel/trace/blktrace.c	2013-09-24 22:55:39.004572055 -0400
@@ -278,17 +278,33 @@ record_it:
 static struct dentry *blk_tree_root;
 static DEFINE_MUTEX(blk_tree_mutex);
 
-static void blk_trace_free(struct blk_trace *bt)
+static void blk_trace_release(struct kref *kref)
 {
+	struct blk_trace *bt = container_of(kref, struct blk_trace, kref);
 	debugfs_remove(bt->msg_file);
 	debugfs_remove(bt->dropped_file);
-	relay_close(bt->rchan);
 	debugfs_remove(bt->dir);
 	free_percpu(bt->sequence);
 	free_percpu(bt->msg_data);
 	kfree(bt);
 }
 
+static void blk_trace_free(struct blk_trace *bt)
+{
+	/*
+	 * The directory can't be removed until it's empty.
+	 * The debugfs files created directly can be removed while
+	 * they're open. The debugfs files created by relay won't
+	 * be removed until they've been released. This drops our
+	 * references to the files but the directory (and the rest
+	 * of the blk_trace structure) won't be cleaned up until
+	 * all of the relay files are closed by the user.
+	 */
+	relay_close(bt->rchan);
+	bt->rchan = NULL;
+	kref_put(&bt->kref, blk_trace_release);
+}
+
 static void blk_trace_cleanup(struct blk_trace *bt)
 {
 	blk_trace_free(bt);
@@ -381,8 +397,10 @@ static int blk_subbuf_start_callback(str
 
 static int blk_remove_buf_file_callback(struct dentry *dentry)
 {
+	struct blk_trace *bt = dentry->d_parent->d_inode->i_private;
 	debugfs_remove(dentry);
 
+	kref_put(&bt->kref, blk_trace_release);
 	return 0;
 }
 
@@ -392,8 +410,15 @@ static struct dentry *blk_create_buf_fil
 						   struct rchan_buf *buf,
 						   int *is_global)
 {
-	return debugfs_create_file(filename, mode, parent, buf,
-					&relay_file_operations);
+	struct blk_trace *bt = parent->d_inode->i_private;
+	struct dentry *dentry;
+
+	dentry = debugfs_create_file(filename, mode, parent, buf,
+				     &relay_file_operations);
+	if (dentry)
+		kref_get(&bt->kref);
+
+	return dentry;
 }
 
 static struct rchan_callbacks blk_relay_callbacks = {
@@ -448,6 +473,8 @@ int do_blk_trace_setup(struct request_qu
 	if (!bt)
 		return -ENOMEM;
 
+	kref_init(&bt->kref);
+
 	ret = -ENOMEM;
 	bt->sequence = alloc_percpu(unsigned long);
 	if (!bt->sequence)
@@ -474,6 +501,8 @@ int do_blk_trace_setup(struct request_qu
 	if (!dir)
 		goto err;
 
+	dir->d_inode->i_private = bt;
+
 	bt->dir = dir;
 	bt->dev = dev;
 	atomic_set(&bt->dropped, 0);

Re: [PATCH] blktrace: fix race with open trace files and directory removal

[Jeff Moyer]

Jeff Mahoney <jeffm@suse.com> writes:

> There's a bug in the blktrace client where it will stop and tear down
> all of the tracing instances for devices it's opened whether it
> successfully completed the setup or not.
>
> By starting multiple blktrace processes on the same device, it's possible
> to permanently disable blktrace on that device. The cause is that when
> the first blktrace process to exit tears down the directory structure,
> the trace files are still held open. Debugfs removes the dentries for the
> open files just fine but the relay implementation doesn't remove the
> dentries until all of the references to the file are dropped. This means
> that if there are open files when debugfs_remove is called for the device
> directory, the directory is not empty and can't be removed. Since the
> shutdown of the blktrace structure xchg's the structure out, there's no
> way to clean up the directory and any new blktrace processes will fail
> to start because it can't create the directory.
>
> This patch adds a kref to blk_trace so that we can release it after the
> initial reference as well as all of the references accumulated by the
> relay files are dropped.

Can't we just do proper unwinding of errors in the do_blktrace_setup
function?  In other words, don't just blindly call blk_trace_free, but
instead just undo anything we've done.

Cheers,
Jeff

Re: [PATCH] blktrace: fix race with open trace files and directory removal

[Jeff Mahoney]

[-- Attachment #1: Type: text/plain, Size: 1651 bytes --]

On 9/27/13 2:43 PM, Jeff Moyer wrote:
> Jeff Mahoney <jeffm@suse.com> writes:
> 
>> There's a bug in the blktrace client where it will stop and tear down
>> all of the tracing instances for devices it's opened whether it
>> successfully completed the setup or not.
>>
>> By starting multiple blktrace processes on the same device, it's possible
>> to permanently disable blktrace on that device. The cause is that when
>> the first blktrace process to exit tears down the directory structure,
>> the trace files are still held open. Debugfs removes the dentries for the
>> open files just fine but the relay implementation doesn't remove the
>> dentries until all of the references to the file are dropped. This means
>> that if there are open files when debugfs_remove is called for the device
>> directory, the directory is not empty and can't be removed. Since the
>> shutdown of the blktrace structure xchg's the structure out, there's no
>> way to clean up the directory and any new blktrace processes will fail
>> to start because it can't create the directory.
>>
>> This patch adds a kref to blk_trace so that we can release it after the
>> initial reference as well as all of the references accumulated by the
>> relay files are dropped.
> 
> Can't we just do proper unwinding of errors in the do_blktrace_setup
> function?  In other words, don't just blindly call blk_trace_free, but
> instead just undo anything we've done.

No. It's not the setup that's causing the problem. It's one process
holding the trace files open while another process calls BLKTRACETEARDOWN.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH] blktrace: fix race with open trace files and directory removal

[Jeff Moyer]

Jeff Mahoney <jeffm@suse.com> writes:

> On 9/27/13 2:43 PM, Jeff Moyer wrote:
>> Jeff Mahoney <jeffm@suse.com> writes:
>> 
>>> There's a bug in the blktrace client where it will stop and tear down
>>> all of the tracing instances for devices it's opened whether it
>>> successfully completed the setup or not.
>>>
>>> By starting multiple blktrace processes on the same device, it's possible
>>> to permanently disable blktrace on that device. The cause is that when
>>> the first blktrace process to exit tears down the directory structure,
>>> the trace files are still held open. Debugfs removes the dentries for the
>>> open files just fine but the relay implementation doesn't remove the
>>> dentries until all of the references to the file are dropped. This means
>>> that if there are open files when debugfs_remove is called for the device
>>> directory, the directory is not empty and can't be removed. Since the
>>> shutdown of the blktrace structure xchg's the structure out, there's no
>>> way to clean up the directory and any new blktrace processes will fail
>>> to start because it can't create the directory.
>>>
>>> This patch adds a kref to blk_trace so that we can release it after the
>>> initial reference as well as all of the references accumulated by the
>>> relay files are dropped.
>> 
>> Can't we just do proper unwinding of errors in the do_blktrace_setup
>> function?  In other words, don't just blindly call blk_trace_free, but
>> instead just undo anything we've done.
>
> No. It's not the setup that's causing the problem. It's one process
> holding the trace files open while another process calls BLKTRACETEARDOWN.

Ah, right.  So, in that case I'd rather restrict the ioctl to just the
process that setup the trace.  Jens, Tejun, any opinions?

Cheers,
Jeff

Re: [PATCH] blktrace: fix race with open trace files and directory removal

[Jeff Mahoney]

[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]

On 9/27/13 2:56 PM, Jeff Moyer wrote:
> Jeff Mahoney <jeffm@suse.com> writes:
> 
>> On 9/27/13 2:43 PM, Jeff Moyer wrote:
>>> Jeff Mahoney <jeffm@suse.com> writes:
>>>
>>>> There's a bug in the blktrace client where it will stop and tear down
>>>> all of the tracing instances for devices it's opened whether it
>>>> successfully completed the setup or not.
>>>>
>>>> By starting multiple blktrace processes on the same device, it's possible
>>>> to permanently disable blktrace on that device. The cause is that when
>>>> the first blktrace process to exit tears down the directory structure,
>>>> the trace files are still held open. Debugfs removes the dentries for the
>>>> open files just fine but the relay implementation doesn't remove the
>>>> dentries until all of the references to the file are dropped. This means
>>>> that if there are open files when debugfs_remove is called for the device
>>>> directory, the directory is not empty and can't be removed. Since the
>>>> shutdown of the blktrace structure xchg's the structure out, there's no
>>>> way to clean up the directory and any new blktrace processes will fail
>>>> to start because it can't create the directory.
>>>>
>>>> This patch adds a kref to blk_trace so that we can release it after the
>>>> initial reference as well as all of the references accumulated by the
>>>> relay files are dropped.
>>>
>>> Can't we just do proper unwinding of errors in the do_blktrace_setup
>>> function?  In other words, don't just blindly call blk_trace_free, but
>>> instead just undo anything we've done.
>>
>> No. It's not the setup that's causing the problem. It's one process
>> holding the trace files open while another process calls BLKTRACETEARDOWN.
> 
> Ah, right.  So, in that case I'd rather restrict the ioctl to just the
> process that setup the trace.  Jens, Tejun, any opinions?

We'd also need to check to see if the task that started the trace is
still around.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 841 bytes --]