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.
