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

[Jeff Moyer]

Jeff Mahoney <jeffm@xxxxxxxx> writes:

> On 9/27/13 2:43 PM, Jeff Moyer wrote:
>> Jeff Mahoney <jeffm@xxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/