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.5 required=3.0 tests=MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,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 70FFAC5518A for ; Wed, 22 Apr 2020 07:48:07 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 31F5F20767 for ; Wed, 22 Apr 2020 07:48:07 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 31F5F20767 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id C271F8E0005; Wed, 22 Apr 2020 03:48:06 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id BD7088E0003; Wed, 22 Apr 2020 03:48:06 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id AEBA58E0005; Wed, 22 Apr 2020 03:48:06 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0043.hostedemail.com [216.40.44.43]) by kanga.kvack.org (Postfix) with ESMTP id 95DDB8E0003 for ; Wed, 22 Apr 2020 03:48:06 -0400 (EDT) Received: from smtpin07.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 4F3F58248047 for ; Wed, 22 Apr 2020 07:48:06 +0000 (UTC) X-FDA: 76734712572.07.skirt55_63d2b8fdc8d4e X-HE-Tag: skirt55_63d2b8fdc8d4e X-Filterd-Recvd-Size: 7724 Received: from mail-pg1-f195.google.com (mail-pg1-f195.google.com [209.85.215.195]) by imf15.hostedemail.com (Postfix) with ESMTP for ; Wed, 22 Apr 2020 07:48:05 +0000 (UTC) Received: by mail-pg1-f195.google.com with SMTP id q18so670445pgm.11 for ; Wed, 22 Apr 2020 00:48:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=+SMYq/4vJ74M0LfJtwun4BRKxfhWvtLo+wkfl9OYurc=; b=ECKmCrVG4W6fRBIc4eVqqgHB5BQMR/N+gpoSt0DcsJqncgIockKpeqQz6ZXgfCoDnK kfHBlUYCmfpjUXPuSaYEZ1C5SNkIItqFmaOfpR9DAGxKfdtgANU3W7/ii191onTZSU+C PUUR6Qv7JgIGIOK4VkuiNZB7ofiNCEkguQ6+kM7ggy/2i6/+HgI/l18jinhWU1TVzKXg ZMN9dkm6FOCS/+aVO9oM4SxiHQpYHlaSZj3Qw7bbcxVr0SnaPNHg4rU5EWySpzlCNSxm bwYw7BKGRAQqxrYSDLkgnKQJf3osjrUvp3ixmpGvb6TnwyIpuRKi/pBUWrufmmU9XaVH z3mw== X-Gm-Message-State: AGi0PuZ/cMxyu/qteeDRUbaFGPsgHqGa1AuZJ37HG9XTNQTtgEPABmCd MTuuYbL5BZojDdGR4Aov4RY= X-Google-Smtp-Source: APiQypLRQPdTJrBV6I/vZtyxwxFN1GzQ0y7RzxhSMbXI2z5hooPeifS431qso2u8rbsgVBHwKEY4Bw== X-Received: by 2002:a63:6f07:: with SMTP id k7mr26600891pgc.274.1587541684638; Wed, 22 Apr 2020 00:48:04 -0700 (PDT) Received: from 42.do-not-panic.com (42.do-not-panic.com. [157.230.128.187]) by smtp.gmail.com with ESMTPSA id fh18sm16882435pjb.0.2020.04.22.00.48.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 22 Apr 2020 00:48:03 -0700 (PDT) Received: by 42.do-not-panic.com (Postfix, from userid 1000) id D50C1402A1; Wed, 22 Apr 2020 07:48:02 +0000 (UTC) Date: Wed, 22 Apr 2020 07:48:02 +0000 From: Luis Chamberlain To: Christoph Hellwig Cc: axboe@kernel.dk, viro@zeniv.linux.org.uk, bvanassche@acm.org, gregkh@linuxfoundation.org, rostedt@goodmis.org, mingo@redhat.com, jack@suse.cz, ming.lei@redhat.com, nstange@suse.de, akpm@linux-foundation.org, mhocko@suse.com, yukuai3@huawei.com, linux-block@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Omar Sandoval , Hannes Reinecke , Michal Hocko , syzbot+603294af2d01acfdd6da@syzkaller.appspotmail.com Subject: Re: [PATCH v2 03/10] blktrace: fix debugfs use after free Message-ID: <20200422074802.GS11244@42.do-not-panic.com> References: <20200419194529.4872-1-mcgrof@kernel.org> <20200419194529.4872-4-mcgrof@kernel.org> <20200422072715.GC19116@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200422072715.GC19116@infradead.org> User-Agent: Mutt/1.10.1 (2018-07-13) X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, Apr 22, 2020 at 12:27:15AM -0700, Christoph Hellwig wrote: > On Sun, Apr 19, 2020 at 07:45:22PM +0000, Luis Chamberlain wrote: > > +{ > > + struct dentry *dir = NULL; > > + > > + /* This can happen if we have a bug in the lower layers */ > > + dir = debugfs_lookup(kobject_name(q->kobj.parent), blk_debugfs_root); > > + if (dir) { > > + pr_warn("%s: registering request_queue debugfs directory twice is not allowed\n", > > + kobject_name(q->kobj.parent)); > > + dput(dir); > > + return -EALREADY; > > + } > > I don't see why we need this check. If it is valueable enough we > should have a debugfs_create_dir_exclusive or so that retunrns an error > for an exsting directory, instead of reimplementing it in the caller in > a racy way. But I'm not really sure we need it to start with. In short races, and even with synchronous request_queue removal I'm seeing the race is still possible, but that's due to some other races I'm going to chase down now. The easier solution really is to just have a debugfs dir created for each partition if debugfs is enabled, this way the directory will always be there, and the lookups are gone. > > + > > + q->debugfs_dir = debugfs_create_dir(kobject_name(q->kobj.parent), > > + blk_debugfs_root); > > + if (!q->debugfs_dir) > > + return -ENOMEM; > > + > > + return 0; > > +} > > + > > +void blk_queue_debugfs_unregister(struct request_queue *q) > > +{ > > + debugfs_remove_recursive(q->debugfs_dir); > > + q->debugfs_dir = NULL; > > +} > > Which to me suggests we can just fold these two into the callers, > with an IS_ENABLED for the creation case given that we check for errors > and the stub will always return an error. Sorry not sure I follow this. > > debugfs_create_files(q->debugfs_dir, q, blk_mq_debugfs_queue_attrs); > > > > /* > > @@ -856,9 +853,7 @@ void blk_mq_debugfs_register(struct request_queue *q) > > > > void blk_mq_debugfs_unregister(struct request_queue *q) > > { > > - debugfs_remove_recursive(q->debugfs_dir); > > q->sched_debugfs_dir = NULL; > > - q->debugfs_dir = NULL; > > } > > This function is weird - the sched dir gets removed by the > debugfs_remove_recursive, so just leaving a function that clears > a pointer is rather odd. In fact I don't think we need to clear > either sched_debugfs_dir or debugfs_dir anywhere. Indeed. Will clean it up. > > @@ -975,6 +976,14 @@ int blk_register_queue(struct gendisk *disk) > > goto unlock; > > } > > > > + ret = blk_queue_debugfs_register(q); > > + if (ret) { > > + blk_trace_remove_sysfs(dev); > > + kobject_del(&q->kobj); > > + kobject_put(&dev->kobj); > > + goto unlock; > > + } > > + > > Please use a goto label to consolidate the common cleanup code. Sure. > Also I think these generic debugfs changes probably should be separate > to the blktrace changes. I'll try to do that. > > static struct dentry *blk_trace_debugfs_dir(struct blk_user_trace_setup *buts, > > + struct request_queue *q, > > struct blk_trace *bt) > > { > > struct dentry *dir = NULL; > > > > + /* This can only happen if we have a bug on our lower layers */ > > + if (!q->kobj.parent) { > > + pr_warn("%s: request_queue parent is gone\n", buts->name); > > + return NULL; > > + } > > Why is this not simply a WARN_ON_ONCE()? I'll actually remove it and instead fix the race where it happens. > > + if (blk_trace_target_disk(buts->name, kobject_name(q->kobj.parent))) { > > + if (!q->debugfs_dir) { > > + pr_warn("%s: expected request_queue debugfs_dir is not set\n", > > + buts->name); > > + return NULL; > > + } > > + /* > > + * debugfs_lookup() is used to ensure the directory is not > > + * taken from underneath us. We must dput() it later once > > + * done with it within blktrace. > > + */ > > + dir = debugfs_lookup(buts->name, blk_debugfs_root); > > + if (!dir) { > > + pr_warn("%s: expected request_queue debugfs_dir dentry is gone\n", > > + buts->name); > > + return NULL; > > + } > > + /* > > + * This is a reaffirmation that debugfs_lookup() shall always > > + * return the same dentry if it was already set. > > + */ > > + if (dir != q->debugfs_dir) { > > + dput(dir); > > + pr_warn("%s: expected dentry dir != q->debugfs_dir\n", > > + buts->name); > > + return NULL; > > + } > > + bt->backing_dir = q->debugfs_dir; > > + return bt->backing_dir; > > + } > > Even with the gigantic commit log I don't get the point of this > code. It looks rather sketchy and I can't find a rationale for it. Yeah I think this is going to be much easier on the eyes with the revert to synchronous request_queue removal first. Luis