Re: Possible FS race condition between iterate_dir and d_alloc_parallel

[Al Viro <viro@zeniv.linux.org.uk>]

On Fri, Sep 06, 2019 at 12:55:22AM +0000, Jun Li wrote:
> > Huh?
> > In drivers/usb/typec/tcpm/tcpm.c:
> > static void tcpm_debugfs_exit(struct tcpm_port *port) {
> >         int i;
> > 
> >         mutex_lock(&port->logbuffer_lock);
> >         for (i = 0; i < LOG_BUFFER_ENTRIES; i++) {
> >                 kfree(port->logbuffer[i]);
> >                 port->logbuffer[i] = NULL;
> >         }
> >         mutex_unlock(&port->logbuffer_lock);
> > 
> >         debugfs_remove(port->dentry);
> >         if (list_empty(&rootdir->d_subdirs)) {
> >                 debugfs_remove(rootdir);
> >                 rootdir = NULL;
> >         }
> > }
> > 
> > Unrelated, but obviously broken.  Not only the locking is deeply suspect, but it's trivially
> > confused by open() on the damn directory.  It will definitely have ->d_subdirs non-empty.
> > 
> > Came in "usb: typec: tcpm: remove tcpm dir if no children", author Cc'd...  Why not
> > remove the directory on rmmod?
> 
> That's because tcpm is a utility driver and there may be multiple instances
> created under the directory, each instance/user removal will call to tcpm_debugfs_exit()
> but only the last one should remove the directory.

Er...  So why not have the directory present for as long as the module is in,
removing it on rmmod?

> Below patch changed this by using dedicated dir for each instance:
> 
> https://www.spinics.net/lists/linux-usb/msg183965.html

*shrug*

Up to you; the variant in mainline is obviously broken (open the debugfs
directory and you'll confuse the hell out of that check).  My preference
in fixing those would've been to make mkdir and rmdir of the parent
unconditional, happening on module_init() and module_exit() resp., not
bothering with "is that the last one" checks, but I'm (a) not a user of that
code and (b) currently not quite sober, so I'll just leave that to you guys.