On 2020-06-22 05:42, Luis Chamberlain wrote:

On Sat, Jun 20, 2020 at 11:07:43AM -0700, Bart Van Assche wrote:

quoted

On 2020-06-19 13:47, Luis Chamberlain wrote:

quoted

We were only creating the request_queue debugfs_dir only
for make_request block drivers (multiqueue), but never for
request-based block drivers. We did this as we were only
creating non-blktrace additional debugfs files on that directory
for make_request drivers. However, since blktrace *always* creates
that directory anyway, we special-case the use of that directory
on blktrace. Other than this being an eye-sore, this exposes
request-based block drivers to the same debugfs fragile
race that used to exist with make_request block drivers
where if we start adding files onto that directory we can later
run a race with a double removal of dentries on the directory
if we don't deal with this carefully on blktrace.

Instead, just simplify things by always creating the request_queue
debugfs_dir on request_queue registration. Rename the mutex also to
reflect the fact that this is used outside of the blktrace context.

There are two changes in this patch: a bug fix and a rename of a mutex.
I don't like it to see two changes in a single patch.

I thought about doing the split first, and I did it at first, but
then I could hear Christoph yelling at me for it. So I merged the
two together. Although it makes it more difficult for review,
the changes do go together.

During the past weeks I have been more busy than usual. I will try to
make sure that in the future I have the time to read all comments on the
previous versions of a patch series before replying to the latest
version of a patch series.

quoted

Additionally, is the new mutex name really better than the old name? The
proper way to use mutexes is to use mutexes to protect data instead of
code. Where is the documentation that mentions which member variable(s)
of which data structures are protected by the mutex formerly called
blk_trace_mutex?

It does not exist, and that is the point. The debugfs_dir use after
free showed us *when* that UAF can happen, and so care must be taken
if we are to use the mutex to protect the debugfs_dir but also re-use
the same directory for other block core shenanigans.

quoted

Since the new name makes it even less clear which data
is protected by this mutex, is the new name really better than the old name?

I thought the new name makes it crystal clear what is being protected. I
can however add a comment to explain that the q->debugfs_mutex protects
the q->debugfs_dir if it is created, otherwise it protects the ephemeral
debugfs_dir directory which would otherwise be created in lieue of
q->debugfs_dir, however the patch still lies under <debugfs_root>/block/.

Let me know if you think that will help.

My concern is that q->debugfs_mutex would evolve the same way as
q->sysfs_lock: at the time of introduction the role of a mutex is very
clear but over time the number of use cases grows to a point where it is
no longer possible to recognize the original purpose. I think there are
two possible approaches: either a comment is added now that explains the
role of q->debugfs_mutex or someone who has followed this conversation
yells when someone tries to use q->debugfs_mutex for another purpose
than what it was intended for.

Thanks,

Bart.