From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id ABF38C43331 for ; Fri, 6 Sep 2019 02:00:25 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id B2467206DE for ; Fri, 6 Sep 2019 02:00:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2404309AbfIFCAY (ORCPT ); Thu, 5 Sep 2019 22:00:24 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:46100 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2404307AbfIFCAY (ORCPT ); Thu, 5 Sep 2019 22:00:24 -0400 Received: from viro by ZenIV.linux.org.uk with local (Exim 4.92.1 #3 (Red Hat Linux)) id 1i63Xt-0005wH-AW; Fri, 06 Sep 2019 02:00:17 +0000 Date: Fri, 6 Sep 2019 03:00:17 +0100 From: Al Viro To: Jun Li Cc: "zhengbin (A)" , "jack@suse.cz" , "akpm@linux-foundation.org" , "linux-fsdevel@vger.kernel.org" , "zhangyi (F)" , "renxudong1@huawei.com" Subject: Re: Possible FS race condition between iterate_dir and d_alloc_parallel Message-ID: <20190906020017.GV1131@ZenIV.linux.org.uk> References: <20190903154007.GJ1131@ZenIV.linux.org.uk> <20190903154114.GK1131@ZenIV.linux.org.uk> <20190905174744.GP1131@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.12.0 (2019-05-25) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org 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.