Re: [PATCH v6 6/6] blktrace: fix debugfs use after free

From: Christoph Hellwig <hch@infradead.org>
To: Luis Chamberlain <mcgrof@kernel.org>
Cc: Christoph Hellwig <hch@infradead.org>, Jan Kara <jack@suse.cz>,
	axboe@kernel.dk, viro@zeniv.linux.org.uk, bvanassche@acm.org,
	gregkh@linuxfoundation.org, rostedt@goodmis.org,
	mingo@redhat.com, ming.lei@redhat.com, nstange@suse.de,
	akpm@linux-foundation.org, mhocko@suse.com, yukuai3@huawei.com,
	martin.petersen@oracle.com, jejb@linux.ibm.com,
	linux-block@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Omar Sandoval <osandov@fb.com>, Hannes Reinecke <hare@suse.com>,
	Michal Hocko <mhocko@kernel.org>,
	syzbot+603294af2d01acfdd6da@syzkaller.appspotmail.com
Subject: Re: [PATCH v6 6/6] blktrace: fix debugfs use after free
Date: Tue, 9 Jun 2020 10:32:18 -0700	[thread overview]
Message-ID: <20200609173218.GA7968@infradead.org> (raw)
In-Reply-To: <20200609172922.GP11244@42.do-not-panic.com>

On Tue, Jun 09, 2020 at 05:29:22PM +0000, Luis Chamberlain wrote:
> Is scsi-generic is the only unwanted ugly child blktrace has to deal
> with? For some reason I thought drivers/md/md.c was one but it seems
> like it is not. Do we have an easy way to search for these? I think
> this would just affect how we express the comment only.

grep for blk_trace_setup.  For all blk devices that setup comes in
through the block device ioctl path, and that relies on having a
struct block_device and queue.  sg on the other hand calls
blk_trace_setup directly with a NULL bdev argument.

> >  		 */
> > -		dir = q->sg_debugfs_dir;
> > +		dir = debugfs_create_dir(buts->name, blk_debugfs_root);
> > +		bt->dir = dir;
> 
> The other chicken and egg problem to consider at least in the comments
> is that the debugfs directory for these types of devices *have* an
> exposed path, but the data structure is rather opaque to the device and
> even blktrace.  Fortunately given the recent set of changes around the
> q->blk_trace and clarifications around its use we have made it clear now
> that so long as hold the q->blk_trace_mutex *and* check q->blk_trace we
> *should* not race against two separate creations of debugfs directories,
> so I think this is safe, so long as these indpendent drivers don't end
> up re-using the same path for some other things later in the future, and
> since we have control over what goes under debugfsroot block / I think
> we should be good.
> 
> But I think that the concern for race on names may still be worth
> explaining a bit here.

Feel free to add more comments, but please try to keep them short
and crisp.  At the some point long comments really distract from what
is going on.