Re: [PATCH v7 8/8] block: create the request_queue debugfs_dir on registration

[Bart Van Assche <bvanassche@acm.org>]

On 2020-06-22 05:42, Luis Chamberlain wrote:
> On Sat, Jun 20, 2020 at 11:07:43AM -0700, Bart Van Assche wrote:
>> On 2020-06-19 13:47, Luis Chamberlain wrote:
>>> 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.

>> 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.
> 
>> 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.