Re: [PATCH v5 5/7] blktrace: fix debugfs use after free

2020-06-05 Thread Luis Chamberlain
On Thu, Jun 04, 2020 at 09:48:43PM -0700, Bart Van Assche wrote: > On 2020-06-01 10:05, Luis Chamberlain wrote: > > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c > > index a55cbfd060f5..5b0310f38e11 100644 > > --- a/kernel/trace/blktrace.c > > +++ b/kernel/trace/blktrace.c > > @@

Re: [PATCH v5 5/7] blktrace: fix debugfs use after free

2020-06-04 Thread Bart Van Assche
On 2020-06-01 10:05, Luis Chamberlain wrote: > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c > index a55cbfd060f5..5b0310f38e11 100644 > --- a/kernel/trace/blktrace.c > +++ b/kernel/trace/blktrace.c > @@ -511,6 +511,11 @@ static int do_blk_trace_setup(struct request_queue *q, >

Re: [PATCH v5 5/7] blktrace: fix debugfs use after free

2020-06-01 Thread Luis Chamberlain
On Wed, May 27, 2020 at 03:12:02AM +, Luis Chamberlain wrote: > You forgot to deal with partitions. Putting similar lipstick on the pig, > this is what I end up with, let me know if this seems agreeable: So even with the partition stuff in place, this approach still don't allow multiple uses

Re: [PATCH v5 5/7] blktrace: fix debugfs use after free

2020-05-29 Thread Bart Van Assche
On 2020-05-29 00:56, Luis Chamberlain wrote: > On Wed, May 27, 2020 at 06:15:10PM -0700, Bart Van Assche wrote: >> How about adding a lockdep_assert_held(>blk_trace_mutex) statement in >> do_blk_trace_setup()? > > Sure, however that doesn't seem part of the fix. How about adding that > as a

Re: [PATCH v5 5/7] blktrace: fix debugfs use after free

2020-05-29 Thread Luis Chamberlain
On Wed, May 27, 2020 at 06:15:10PM -0700, Bart Van Assche wrote: > On 2020-05-26 20:12, Luis Chamberlain wrote: > > + /* > > +* Blktrace needs a debugsfs name even for queues that don't register > > +* a gendisk, so it lazily registers the debugfs directory. But that > > +* can get

Re: [PATCH v5 5/7] blktrace: fix debugfs use after free

2020-05-27 Thread Bart Van Assche
On 2020-05-26 20:12, Luis Chamberlain wrote: > + /* > + * Blktrace needs a debugsfs name even for queues that don't register > + * a gendisk, so it lazily registers the debugfs directory. But that > + * can get us into a situation where a SCSI device is found, with no > +

Re: [PATCH v5 5/7] blktrace: fix debugfs use after free

2020-05-26 Thread Luis Chamberlain
On Tue, May 19, 2020 at 09:37:13AM -0700, Christoph Hellwig wrote: > I don't think we need any of that symlink stuff. Even if we want it > (which I don't), it should not be in a bug fix patch. > > In fact to fix the blktrace race I think we only need something like > this fairly trivial patch

Re: [PATCH v5 5/7] blktrace: fix debugfs use after free

2020-05-19 Thread Greg KH
On Tue, May 19, 2020 at 03:52:10PM +, Luis Chamberlain wrote: > On Tue, May 19, 2020 at 04:44:08PM +0200, Greg KH wrote: > > On Sat, May 16, 2020 at 03:19:54AM +, Luis Chamberlain wrote: > > > struct dentry *blk_debugfs_root; > > > +struct dentry *blk_debugfs_bsg = NULL; > > > >

Re: [PATCH v5 5/7] blktrace: fix debugfs use after free

2020-05-19 Thread Greg KH
On Tue, May 19, 2020 at 09:37:13AM -0700, Christoph Hellwig wrote: > I don't think we need any of that symlink stuff. Even if we want it > (which I don't), it should not be in a bug fix patch. I agree, why are the symlinks even needed? This is debugfs, the files/contents there can change

Re: [PATCH v5 5/7] blktrace: fix debugfs use after free

2020-05-19 Thread Christoph Hellwig
I don't think we need any of that symlink stuff. Even if we want it (which I don't), it should not be in a bug fix patch. In fact to fix the blktrace race I think we only need something like this fairly trivial patch (completely untested so far) below. (and with that we can also drop the

Re: [PATCH v5 5/7] blktrace: fix debugfs use after free

2020-05-19 Thread Luis Chamberlain
On Tue, May 19, 2020 at 04:44:08PM +0200, Greg KH wrote: > On Sat, May 16, 2020 at 03:19:54AM +, Luis Chamberlain wrote: > > struct dentry *blk_debugfs_root; > > +struct dentry *blk_debugfs_bsg = NULL; > > checkpatch didn't complain about "= NULL;"? Will remove. > > +static void

Re: [PATCH v5 5/7] blktrace: fix debugfs use after free

2020-05-19 Thread Greg KH
On Sat, May 16, 2020 at 03:19:54AM +, Luis Chamberlain wrote: > struct dentry *blk_debugfs_root; > +struct dentry *blk_debugfs_bsg = NULL; checkpatch didn't complain about "= NULL;"? > + > +/** > + * enum blk_debugfs_dir_type - block device debugfs directory type > + * @BLK_DBG_DIR_BASE:

[PATCH v5 5/7] blktrace: fix debugfs use after free

2020-05-15 Thread Luis Chamberlain
On commit 6ac93117ab00 ("blktrace: use existing disk debugfs directory") merged on v4.12 Omar fixed the original blktrace code for request-based drivers (multiqueue). This however left in place a possible crash, if you happen to abuse blktrace while racing to remove / add a device. We used to use