Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-13 Thread t...@kernel.org
Hello, Bart.

On Thu, Apr 12, 2018 at 10:40:52PM +, Bart Van Assche wrote:
> Is something like the patch below perhaps what you had in mind?

Yeah, exactly.  It'd be really great to have the lockdep asserts add
to the right places.

Thanks.

-- 
tejun


Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-12 Thread Bart Van Assche
On Thu, 2018-04-12 at 12:09 -0700, t...@kernel.org wrote:
> On Thu, Apr 12, 2018 at 06:56:26PM +, Bart Van Assche wrote:
> > On Thu, 2018-04-12 at 11:11 -0700, t...@kernel.org wrote:
> > > * Right now, blk_queue_enter/exit() doesn't have any annotations.
> > >   IOW, there's no way for paths which need enter locked to actually
> > >   assert that.  If we're gonna protect more things with queue
> > >   enter/exit, it gotta be annotated.
> > 
> > Hello Tejun,
> > 
> > One possibility is to check the count for the local CPU of 
> > q->q_usage_counter
> > at runtime, e.g. using WARN_ON_ONCE(). However, that might have a runtime
> > overhead. Another possibility is to follow the example of kernfs and to use
> > a lockdep map and lockdep_assert_held(). There might be other alternatives I
> > have not thought of.
> 
> Oh, I'd just do straight-forward lockdep annotation on
> queue_enter/exit functions and provide the necessary assert helpers.

Hello Tejun,

Is something like the patch below perhaps what you had in mind?

Thanks,

Bart.

Subject: [PATCH] block: Use lockdep to verify whether blk_queue_enter() is
 used when necessary

Suggested-by: Tejun Heo 
Signed-off-by: Bart Van Assche 
Cc: Tejun Heo 
---
 block/blk-cgroup.c |  2 ++
 block/blk-core.c   | 13 -
 block/blk.h|  1 +
 include/linux/blk-cgroup.h |  2 ++
 include/linux/blkdev.h |  1 +
 5 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 1c16694ae145..c34e13e76f90 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -145,6 +145,8 @@ struct blkcg_gq *blkg_lookup_slowpath(struct blkcg *blkcg,
 {
struct blkcg_gq *blkg;
 
+   lock_is_held(>q_enter_map);
+
/*
 * Hint didn't match.  Look up from the radix tree.  Note that the
 * hint can only be updated under queue_lock as otherwise @blkg
diff --git a/block/blk-core.c b/block/blk-core.c
index 39308e874ffa..a61dbe7f24a5 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -931,8 +931,13 @@ int blk_queue_enter(struct request_queue *q, 
blk_mq_req_flags_t flags)
}
rcu_read_unlock();
 
-   if (success)
+   if (success) {
+   mutex_acquire_nest(>q_enter_map, 0, 0,
+  lock_is_held(>q_enter_map) ?
+  >q_enter_map : NULL,
+  _RET_IP_);
return 0;
+   }
 
if (flags & BLK_MQ_REQ_NOWAIT)
return -EBUSY;
@@ -959,6 +964,7 @@ int blk_queue_enter(struct request_queue *q, 
blk_mq_req_flags_t flags)
 
 void blk_queue_exit(struct request_queue *q)
 {
+   mutex_release(>q_enter_map, 0, _RET_IP_);
percpu_ref_put(>q_usage_counter);
 }
 
@@ -994,12 +1000,15 @@ struct request_queue *blk_alloc_queue_node(gfp_t 
gfp_mask, int node_id,
   spinlock_t *lock)
 {
struct request_queue *q;
+   static struct lock_class_key __key;
 
q = kmem_cache_alloc_node(blk_requestq_cachep,
gfp_mask | __GFP_ZERO, node_id);
if (!q)
return NULL;
 
+   lockdep_init_map(>q_enter_map, "q_enter_map", &__key, 0);
+
q->id = ida_simple_get(_queue_ida, 0, 0, gfp_mask);
if (q->id < 0)
goto fail_q;
@@ -2264,6 +2273,8 @@ generic_make_request_checks(struct bio *bio)
goto end_io;
}
 
+   lock_is_held(>q_enter_map);
+
/*
 * For a REQ_NOWAIT based request, return -EOPNOTSUPP
 * if queue is not a request based queue.
diff --git a/block/blk.h b/block/blk.h
index 7cd64f533a46..26f313331b13 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -145,6 +145,7 @@ static inline void blk_queue_enter_live(struct 
request_queue *q)
 * been established, further references under that same context
 * need not check that the queue has been frozen (marked dead).
 */
+   mutex_acquire_nest(>q_enter_map, 0, 0, >q_enter_map, _RET_IP_);
percpu_ref_get(>q_usage_counter);
 }
 
diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
index 6c666fd7de3c..52e2e2d9971e 100644
--- a/include/linux/blk-cgroup.h
+++ b/include/linux/blk-cgroup.h
@@ -266,6 +266,8 @@ static inline struct blkcg_gq *__blkg_lookup(struct blkcg 
*blkcg,
 {
struct blkcg_gq *blkg;
 
+   lock_is_held(>q_enter_map);
+
if (blkcg == _root)
return q->root_blkg;
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 0d22f351a74b..a0b1adbd4406 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -631,6 +631,7 @@ struct request_queue {
struct rcu_head rcu_head;
wait_queue_head_t   mq_freeze_wq;
struct 

Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-12 Thread t...@kernel.org
Hello,

On Thu, Apr 12, 2018 at 06:56:26PM +, Bart Van Assche wrote:
> On Thu, 2018-04-12 at 11:11 -0700, t...@kernel.org wrote:
> > * Right now, blk_queue_enter/exit() doesn't have any annotations.
> >   IOW, there's no way for paths which need enter locked to actually
> >   assert that.  If we're gonna protect more things with queue
> >   enter/exit, it gotta be annotated.
> 
> Hello Tejun,
> 
> One possibility is to check the count for the local CPU of q->q_usage_counter
> at runtime, e.g. using WARN_ON_ONCE(). However, that might have a runtime
> overhead. Another possibility is to follow the example of kernfs and to use
> a lockdep map and lockdep_assert_held(). There might be other alternatives I
> have not thought of.

Oh, I'd just do straight-forward lockdep annotation on
queue_enter/exit functions and provide the necessary assert helpers.

Thanks.

-- 
tejun


Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-12 Thread Bart Van Assche
On Thu, 2018-04-12 at 11:11 -0700, t...@kernel.org wrote:
> * Right now, blk_queue_enter/exit() doesn't have any annotations.
>   IOW, there's no way for paths which need enter locked to actually
>   assert that.  If we're gonna protect more things with queue
>   enter/exit, it gotta be annotated.

Hello Tejun,

One possibility is to check the count for the local CPU of q->q_usage_counter
at runtime, e.g. using WARN_ON_ONCE(). However, that might have a runtime
overhead. Another possibility is to follow the example of kernfs and to use
a lockdep map and lockdep_assert_held(). There might be other alternatives I
have not thought of.

Bart.





Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-12 Thread t...@kernel.org
Hello,

On Thu, Apr 12, 2018 at 04:29:09PM +, Bart Van Assche wrote:
> Any code that submits a bio or request needs blk_queue_enter() /
> blk_queue_exit() anyway. Please have a look at the following commit - you will
> see that that commit reduces the number of blk_queue_enter() / 
> blk_queue_exit()
> calls in the hot path:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/commit/?h=for-linus=37f9579f4c31a6d698dbf3016d7bf132f9288d30

So, this can work, but it's still pretty fragile.

* Lock switching is fragile and we really should get rid of it.  This
  is very likely to come back and bite us.

* Right now, blk_queue_enter/exit() doesn't have any annotations.
  IOW, there's no way for paths which need enter locked to actually
  assert that.  If we're gonna protect more things with queue
  enter/exit, it gotta be annotated.

Thanks.

-- 
tejun


Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-12 Thread Bart Van Assche
On Thu, 2018-04-12 at 09:12 -0700, t...@kernel.org wrote:
> > Did you perhaps mean blkg_lookup_create()? That function has one caller,
> > namely blkcg_bio_issue_check(). The only caller of that function is
> > generic_make_request_checks(). A patch was posted on the linux-block mailing
> > list recently that surrounds that call with blk_queue_enter() / 
> > blk_queue_exit().
> > I think that prevents that blkcg_exit_queue() is called concurrently with
> > blkg_lookup_create().
> 
> Yeah, that'd solve the problem for that particular path, but what
> that's doing is adding another layer of refcnting which is used to
> implement the revoke (or sever) semantics.  This is a fragile approach
> tho because it isn't clear who's protecting what and there's nothing
> in blkg's usage which suggests it'd be protected that way and we're
> basically working around a different underlying problem (lock
> switching) by expanding the coverage of a different lock.

Hello Tejun,

Any code that submits a bio or request needs blk_queue_enter() /
blk_queue_exit() anyway. Please have a look at the following commit - you will
see that that commit reduces the number of blk_queue_enter() / blk_queue_exit()
calls in the hot path:

https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/commit/?h=for-linus=37f9579f4c31a6d698dbf3016d7bf132f9288d30

Thanks,

Bart.




Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-12 Thread t...@kernel.org
Hello,

On Thu, Apr 12, 2018 at 04:03:52PM +, Bart Van Assche wrote:
> On Thu, 2018-04-12 at 08:37 -0700, Tejun Heo wrote:
> > On Thu, Apr 12, 2018 at 08:09:17AM -0600, Bart Van Assche wrote:
> > > I have retested hotunplugging by rerunning the srp-test software. It
> > > seems like you overlooked that this patch does not remove the
> > > blkcg_exit_queue() call from blk_cleanup_queue()? If a device is
> > > hotunplugged it is up to the block driver to call
> > > blk_cleanup_queue(). And blk_cleanup_queue() will call
> > > blkcg_exit_queue().
> > 
> > Hmm... what'd prevent blg_lookup_and_create() racing against that?
> 
> Hello Tejun,
> 
> Did you perhaps mean blkg_lookup_create()? That function has one caller,

Ah yeah, sorry about the sloppiness.

> namely blkcg_bio_issue_check(). The only caller of that function is
> generic_make_request_checks(). A patch was posted on the linux-block mailing
> list recently that surrounds that call with blk_queue_enter() / 
> blk_queue_exit().
> I think that prevents that blkcg_exit_queue() is called concurrently with
> blkg_lookup_create().

Yeah, that'd solve the problem for that particular path, but what
that's doing is adding another layer of refcnting which is used to
implement the revoke (or sever) semantics.  This is a fragile approach
tho because it isn't clear who's protecting what and there's nothing
in blkg's usage which suggests it'd be protected that way and we're
basically working around a different underlying problem (lock
switching) by expanding the coverage of a different lock.

I'd much prefer fixing the lock switching problem properly than
working around that shortcoming this way.

Thans.

-- 
tejun


Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-12 Thread Bart Van Assche
On Thu, 2018-04-12 at 08:37 -0700, Tejun Heo wrote:
> On Thu, Apr 12, 2018 at 08:09:17AM -0600, Bart Van Assche wrote:
> > I have retested hotunplugging by rerunning the srp-test software. It
> > seems like you overlooked that this patch does not remove the
> > blkcg_exit_queue() call from blk_cleanup_queue()? If a device is
> > hotunplugged it is up to the block driver to call
> > blk_cleanup_queue(). And blk_cleanup_queue() will call
> > blkcg_exit_queue().
> 
> Hmm... what'd prevent blg_lookup_and_create() racing against that?

Hello Tejun,

Did you perhaps mean blkg_lookup_create()? That function has one caller,
namely blkcg_bio_issue_check(). The only caller of that function is
generic_make_request_checks(). A patch was posted on the linux-block mailing
list recently that surrounds that call with blk_queue_enter() / 
blk_queue_exit().
I think that prevents that blkcg_exit_queue() is called concurrently with
blkg_lookup_create().

Bart.





Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-12 Thread Tejun Heo
Hello, Bart.

On Thu, Apr 12, 2018 at 08:09:17AM -0600, Bart Van Assche wrote:
> I have retested hotunplugging by rerunning the srp-test software. It
> seems like you overlooked that this patch does not remove the
> blkcg_exit_queue() call from blk_cleanup_queue()? If a device is
> hotunplugged it is up to the block driver to call
> blk_cleanup_queue(). And blk_cleanup_queue() will call
> blkcg_exit_queue().

Hmm... what'd prevent blg_lookup_and_create() racing against that?

Thanks.

-- 
tejun


Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-12 Thread Bart Van Assche

On 04/12/18 07:51, Tejun Heo wrote:

On Wed, Apr 11, 2018 at 07:58:52PM -0600, Bart Van Assche wrote:

Several block drivers call alloc_disk() followed by put_disk() if
something fails before device_add_disk() is called without calling
blk_cleanup_queue(). Make sure that also for this scenario a request
queue is dissociated from the cgroup controller. This patch avoids
that loading the parport_pc, paride and pf drivers triggers the
following kernel crash:

BUG: KASAN: null-ptr-deref in pi_init+0x42e/0x580 [paride]
Read of size 4 at addr 0008 by task modprobe/744
Call Trace:
dump_stack+0x9a/0xeb
kasan_report+0x139/0x350
pi_init+0x42e/0x580 [paride]
pf_init+0x2bb/0x1000 [pf]
do_one_initcall+0x8e/0x405
do_init_module+0xd9/0x2f2
load_module+0x3ab4/0x4700
SYSC_finit_module+0x176/0x1a0
do_syscall_64+0xee/0x2b0
entry_SYSCALL_64_after_hwframe+0x42/0xb7

Reported-by: Alexandru Moise <00moses.alexande...@gmail.com>
Fixes: a063057d7c73 ("block: Fix a race between request queue removal and the block 
cgroup controller")
Signed-off-by: Bart Van Assche 
Cc: Alexandru Moise <00moses.alexande...@gmail.com>
Cc: Tejun Heo 


So, this might be correct for this reported case but I don't think
it's correct in general.  There's no synchronization between blkcg
q->lock usages and blk_cleanup_queue().  e.g. hotunplugging and
shutting down a device while file operations are still in progress can
easily blow this up.


Hello Tejun,

I have retested hotunplugging by rerunning the srp-test software. It 
seems like you overlooked that this patch does not remove the 
blkcg_exit_queue() call from blk_cleanup_queue()? If a device is 
hotunplugged it is up to the block driver to call blk_cleanup_queue(). 
And blk_cleanup_queue() will call blkcg_exit_queue().


Bart.



Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-12 Thread t...@kernel.org
On Thu, Apr 12, 2018 at 06:58:21AM -0700, t...@kernel.org wrote:
> Cool, can we just factor out the queue lock from those drivers?  It's
> just really messy to be switching locks like we do in the cleanup
> path.

So, looking at a couple drivers, it looks like all we'd need is a
struct which contains a spinlock and a refcnt.  Switching the drivers
to use that is a bit tedious but straight-forward and the block layer
can simply put that struct on queue release.

Thanks.

-- 
tejun


Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-12 Thread t...@kernel.org
Hello,

On Thu, Apr 12, 2018 at 03:56:51PM +0200, h...@lst.de wrote:
> On Thu, Apr 12, 2018 at 06:48:12AM -0700, t...@kernel.org wrote:
> > > Which sounds like a very good reason not to use a driver controller
> > > lock for internals like blkcq.
> > > 
> > > In fact splitting the lock used for synchronizing access to queue
> > > fields from the driver controller lock used to synchronize I/O
> > > in the legacy path in long overdue.
> > 
> > It'd be probably a lot easier to make sure the shared lock doesn't go
> > away till all the request_queues using it go away.  The choice is
> > between refcnting something which carries the lock and double locking
> > in hot paths.  Can't think of many downsides of the former approach.
> 
> We've stopped sharing request_queues between different devices a while
> ago.  The problem is just that for legacy devices the driver still controls
> the lock life time, and it might be shorter than the queue lifetime.
> Note that for blk-mq we basically don't use the queue_lock at all,
> and certainly not in the hot path.

Cool, can we just factor out the queue lock from those drivers?  It's
just really messy to be switching locks like we do in the cleanup
path.

Thanks.

-- 
tejun


Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-12 Thread h...@lst.de
On Thu, Apr 12, 2018 at 06:48:12AM -0700, t...@kernel.org wrote:
> > Which sounds like a very good reason not to use a driver controller
> > lock for internals like blkcq.
> > 
> > In fact splitting the lock used for synchronizing access to queue
> > fields from the driver controller lock used to synchronize I/O
> > in the legacy path in long overdue.
> 
> It'd be probably a lot easier to make sure the shared lock doesn't go
> away till all the request_queues using it go away.  The choice is
> between refcnting something which carries the lock and double locking
> in hot paths.  Can't think of many downsides of the former approach.

We've stopped sharing request_queues between different devices a while
ago.  The problem is just that for legacy devices the driver still controls
the lock life time, and it might be shorter than the queue lifetime.
Note that for blk-mq we basically don't use the queue_lock at all,
and certainly not in the hot path.


Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-12 Thread Tejun Heo
On Wed, Apr 11, 2018 at 07:58:52PM -0600, Bart Van Assche wrote:
> Several block drivers call alloc_disk() followed by put_disk() if
> something fails before device_add_disk() is called without calling
> blk_cleanup_queue(). Make sure that also for this scenario a request
> queue is dissociated from the cgroup controller. This patch avoids
> that loading the parport_pc, paride and pf drivers triggers the
> following kernel crash:
> 
> BUG: KASAN: null-ptr-deref in pi_init+0x42e/0x580 [paride]
> Read of size 4 at addr 0008 by task modprobe/744
> Call Trace:
> dump_stack+0x9a/0xeb
> kasan_report+0x139/0x350
> pi_init+0x42e/0x580 [paride]
> pf_init+0x2bb/0x1000 [pf]
> do_one_initcall+0x8e/0x405
> do_init_module+0xd9/0x2f2
> load_module+0x3ab4/0x4700
> SYSC_finit_module+0x176/0x1a0
> do_syscall_64+0xee/0x2b0
> entry_SYSCALL_64_after_hwframe+0x42/0xb7
> 
> Reported-by: Alexandru Moise <00moses.alexande...@gmail.com>
> Fixes: a063057d7c73 ("block: Fix a race between request queue removal and the 
> block cgroup controller")
> Signed-off-by: Bart Van Assche 
> Cc: Alexandru Moise <00moses.alexande...@gmail.com>
> Cc: Tejun Heo 

So, this might be correct for this reported case but I don't think
it's correct in general.  There's no synchronization between blkcg
q->lock usages and blk_cleanup_queue().  e.g. hotunplugging and
shutting down a device while file operations are still in progress can
easily blow this up.

Thanks.

-- 
tejun


Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-12 Thread t...@kernel.org
Hello,

On Thu, Apr 12, 2018 at 03:14:40PM +0200, h...@lst.de wrote:
> > At least the SCSI ULP drivers drop the last reference to a disk after
> > the blk_cleanup_queue() call. As explained in the description of commit
> > a063057d7c73, removing a request queue from blkcg must happen before
> > blk_cleanup_queue() finishes because a block driver may free the
> > request queue spinlock immediately after blk_cleanup_queue() returns.
> > So I don't think that we can move the code that removes a request
> > queue from blkcg into put_disk(). Another challenge is that some block
> > drivers (e.g. skd) clear the disk->queue pointer if device_add_disk()
> > has not been called to avoid that put_disk() causes a request queue
> > reference count imbalance.
> 
> Which sounds like a very good reason not to use a driver controller
> lock for internals like blkcq.
> 
> In fact splitting the lock used for synchronizing access to queue
> fields from the driver controller lock used to synchronize I/O
> in the legacy path in long overdue.

It'd be probably a lot easier to make sure the shared lock doesn't go
away till all the request_queues using it go away.  The choice is
between refcnting something which carries the lock and double locking
in hot paths.  Can't think of many downsides of the former approach.

Thanks.

-- 
tejun


Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-12 Thread h...@lst.de
On Thu, Apr 12, 2018 at 11:52:11AM +, Bart Van Assche wrote:
> On Thu, 2018-04-12 at 07:34 +0200, Christoph Hellwig wrote:
> > On Wed, Apr 11, 2018 at 07:58:52PM -0600, Bart Van Assche wrote:
> > > Several block drivers call alloc_disk() followed by put_disk() if
> > > something fails before device_add_disk() is called without calling
> > > blk_cleanup_queue(). Make sure that also for this scenario a request
> > > queue is dissociated from the cgroup controller. This patch avoids
> > > that loading the parport_pc, paride and pf drivers triggers the
> > > following kernel crash:
> > 
> > Can we move the cleanup to put_disk in general and not just for
> > this case?  Having alloc/free routines pair up generally avoids
> > a lot of confusion.
> 
> Hello Christoph,
> 
> At least the SCSI ULP drivers drop the last reference to a disk after
> the blk_cleanup_queue() call. As explained in the description of commit
> a063057d7c73, removing a request queue from blkcg must happen before
> blk_cleanup_queue() finishes because a block driver may free the
> request queue spinlock immediately after blk_cleanup_queue() returns.
> So I don't think that we can move the code that removes a request
> queue from blkcg into put_disk(). Another challenge is that some block
> drivers (e.g. skd) clear the disk->queue pointer if device_add_disk()
> has not been called to avoid that put_disk() causes a request queue
> reference count imbalance.

Which sounds like a very good reason not to use a driver controller
lock for internals like blkcq.

In fact splitting the lock used for synchronizing access to queue
fields from the driver controller lock used to synchronize I/O
in the legacy path in long overdue.


Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-12 Thread Bart Van Assche
On Thu, 2018-04-12 at 07:34 +0200, Christoph Hellwig wrote:
> On Wed, Apr 11, 2018 at 07:58:52PM -0600, Bart Van Assche wrote:
> > Several block drivers call alloc_disk() followed by put_disk() if
> > something fails before device_add_disk() is called without calling
> > blk_cleanup_queue(). Make sure that also for this scenario a request
> > queue is dissociated from the cgroup controller. This patch avoids
> > that loading the parport_pc, paride and pf drivers triggers the
> > following kernel crash:
> 
> Can we move the cleanup to put_disk in general and not just for
> this case?  Having alloc/free routines pair up generally avoids
> a lot of confusion.

Hello Christoph,

At least the SCSI ULP drivers drop the last reference to a disk after
the blk_cleanup_queue() call. As explained in the description of commit
a063057d7c73, removing a request queue from blkcg must happen before
blk_cleanup_queue() finishes because a block driver may free the
request queue spinlock immediately after blk_cleanup_queue() returns.
So I don't think that we can move the code that removes a request
queue from blkcg into put_disk(). Another challenge is that some block
drivers (e.g. skd) clear the disk->queue pointer if device_add_disk()
has not been called to avoid that put_disk() causes a request queue
reference count imbalance.

Bart.





Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-11 Thread Christoph Hellwig
On Wed, Apr 11, 2018 at 07:58:52PM -0600, Bart Van Assche wrote:
> Several block drivers call alloc_disk() followed by put_disk() if
> something fails before device_add_disk() is called without calling
> blk_cleanup_queue(). Make sure that also for this scenario a request
> queue is dissociated from the cgroup controller. This patch avoids
> that loading the parport_pc, paride and pf drivers triggers the
> following kernel crash:

Can we move the cleanup to put_disk in general and not just for
this case?  Having alloc/free routines pair up generally avoids
a lot of confusion.


Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-11 Thread Alexandru Moise
On Thu, Apr 12, 2018 at 06:20:34AM +0200, Alexandru Moise wrote:
> On Wed, Apr 11, 2018 at 07:58:52PM -0600, Bart Van Assche wrote:
> > Several block drivers call alloc_disk() followed by put_disk() if
> > something fails before device_add_disk() is called without calling
> > blk_cleanup_queue(). Make sure that also for this scenario a request
> > queue is dissociated from the cgroup controller. This patch avoids
> > that loading the parport_pc, paride and pf drivers triggers the
> > following kernel crash:
> > 
> > BUG: KASAN: null-ptr-deref in pi_init+0x42e/0x580 [paride]
> > Read of size 4 at addr 0008 by task modprobe/744
> > Call Trace:
> > dump_stack+0x9a/0xeb
> > kasan_report+0x139/0x350
> > pi_init+0x42e/0x580 [paride]
> > pf_init+0x2bb/0x1000 [pf]
> > do_one_initcall+0x8e/0x405
> > do_init_module+0xd9/0x2f2
> > load_module+0x3ab4/0x4700
> > SYSC_finit_module+0x176/0x1a0
> > do_syscall_64+0xee/0x2b0
> > entry_SYSCALL_64_after_hwframe+0x42/0xb7
> > 
> > Reported-by: Alexandru Moise <00moses.alexande...@gmail.com>
> > Fixes: a063057d7c73 ("block: Fix a race between request queue removal and 
> > the block cgroup controller")
> > Signed-off-by: Bart Van Assche 
> > Cc: Alexandru Moise <00moses.alexande...@gmail.com>
> > Cc: Tejun Heo 
> > ---
> >  block/blk-sysfs.c | 26 ++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> > index f8457d6f0190..2e134da78f82 100644
> > --- a/block/blk-sysfs.c
> > +++ b/block/blk-sysfs.c
> > @@ -788,11 +788,37 @@ static void blk_free_queue_rcu(struct rcu_head 
> > *rcu_head)
> >  static void __blk_release_queue(struct work_struct *work)
> >  {
> > struct request_queue *q = container_of(work, typeof(*q), release_work);
> > +   struct blkcg_gq *gq;
> >  
> > if (test_bit(QUEUE_FLAG_POLL_STATS, >queue_flags))
> > blk_stat_remove_callback(q, q->poll_cb);
> > blk_stat_free_callback(q->poll_cb);
> >  
> > +   if (!blk_queue_dead(q)) {
> > +   /*
> > +* Last reference was dropped without having called
> > +* blk_cleanup_queue().
> > +*/
> > +   WARN_ONCE(blk_queue_init_done(q),
> > + "request queue %p has been registered but 
> > blk_cleanup_queue() has not been called for that queue\n",
> > + q);
> > +   bdi_put(q->backing_dev_info);
> > +   blkcg_exit_queue(q);
> > +
> > +   if (q->elevator) {
> > +   ioc_clear_queue(q);
> > +   elevator_exit(q, q->elevator);
> > +   }
> > +   }
> > +
> > +   rcu_read_lock();
> > +   gq = blkg_lookup(_root, q);
> > +   rcu_read_unlock();
> > +
> > +   WARN(gq,
> > +"request queue %p is being released but it has not yet been 
> > removed from the blkcg controller\n",
> > +q);
> > +
> > blk_free_queue_stats(q->stats);
> 
> This solution is good. Thanks for fixing this!
> 
> Tested-by: Alexandru Moise <00moses.alexande...@gmail.com>
> 
> ../Alex

Oh, you also need the no-ops for !CONFIG_BLK_CGROUP, also there is no
blkcg_root then as well.

Thanks,
../Alex
> 
> >  
> > blk_exit_rl(q, >root_rl);
> > -- 
> > 2.16.2
> > 


Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-11 Thread Alexandru Moise
On Wed, Apr 11, 2018 at 07:58:52PM -0600, Bart Van Assche wrote:
> Several block drivers call alloc_disk() followed by put_disk() if
> something fails before device_add_disk() is called without calling
> blk_cleanup_queue(). Make sure that also for this scenario a request
> queue is dissociated from the cgroup controller. This patch avoids
> that loading the parport_pc, paride and pf drivers triggers the
> following kernel crash:
> 
> BUG: KASAN: null-ptr-deref in pi_init+0x42e/0x580 [paride]
> Read of size 4 at addr 0008 by task modprobe/744
> Call Trace:
> dump_stack+0x9a/0xeb
> kasan_report+0x139/0x350
> pi_init+0x42e/0x580 [paride]
> pf_init+0x2bb/0x1000 [pf]
> do_one_initcall+0x8e/0x405
> do_init_module+0xd9/0x2f2
> load_module+0x3ab4/0x4700
> SYSC_finit_module+0x176/0x1a0
> do_syscall_64+0xee/0x2b0
> entry_SYSCALL_64_after_hwframe+0x42/0xb7
> 
> Reported-by: Alexandru Moise <00moses.alexande...@gmail.com>
> Fixes: a063057d7c73 ("block: Fix a race between request queue removal and the 
> block cgroup controller")
> Signed-off-by: Bart Van Assche 
> Cc: Alexandru Moise <00moses.alexande...@gmail.com>
> Cc: Tejun Heo 
> ---
>  block/blk-sysfs.c | 26 ++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index f8457d6f0190..2e134da78f82 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -788,11 +788,37 @@ static void blk_free_queue_rcu(struct rcu_head 
> *rcu_head)
>  static void __blk_release_queue(struct work_struct *work)
>  {
>   struct request_queue *q = container_of(work, typeof(*q), release_work);
> + struct blkcg_gq *gq;
>  
>   if (test_bit(QUEUE_FLAG_POLL_STATS, >queue_flags))
>   blk_stat_remove_callback(q, q->poll_cb);
>   blk_stat_free_callback(q->poll_cb);
>  
> + if (!blk_queue_dead(q)) {
> + /*
> +  * Last reference was dropped without having called
> +  * blk_cleanup_queue().
> +  */
> + WARN_ONCE(blk_queue_init_done(q),
> +   "request queue %p has been registered but 
> blk_cleanup_queue() has not been called for that queue\n",
> +   q);
> + bdi_put(q->backing_dev_info);
> + blkcg_exit_queue(q);
> +
> + if (q->elevator) {
> + ioc_clear_queue(q);
> + elevator_exit(q, q->elevator);
> + }
> + }
> +
> + rcu_read_lock();
> + gq = blkg_lookup(_root, q);
> + rcu_read_unlock();
> +
> + WARN(gq,
> +  "request queue %p is being released but it has not yet been 
> removed from the blkcg controller\n",
> +  q);
> +
>   blk_free_queue_stats(q->stats);

This solution is good. Thanks for fixing this!

Tested-by: Alexandru Moise <00moses.alexande...@gmail.com>

../Alex

>  
>   blk_exit_rl(q, >root_rl);
> -- 
> 2.16.2
> 


Re: [PATCH] block: Ensure that a request queue is dissociated from the cgroup controller

2018-04-11 Thread Alexandru Moise
On Wed, Apr 11, 2018 at 07:58:52PM -0600, Bart Van Assche wrote:
> Several block drivers call alloc_disk() followed by put_disk() if
> something fails before device_add_disk() is called without calling
> blk_cleanup_queue(). Make sure that also for this scenario a request
> queue is dissociated from the cgroup controller. This patch avoids
> that loading the parport_pc, paride and pf drivers triggers the
> following kernel crash:
> 
> BUG: KASAN: null-ptr-deref in pi_init+0x42e/0x580 [paride]
> Read of size 4 at addr 0008 by task modprobe/744
> Call Trace:
> dump_stack+0x9a/0xeb
> kasan_report+0x139/0x350
> pi_init+0x42e/0x580 [paride]
> pf_init+0x2bb/0x1000 [pf]
> do_one_initcall+0x8e/0x405
> do_init_module+0xd9/0x2f2
> load_module+0x3ab4/0x4700
> SYSC_finit_module+0x176/0x1a0
> do_syscall_64+0xee/0x2b0
> entry_SYSCALL_64_after_hwframe+0x42/0xb7
> 
> Reported-by: Alexandru Moise <00moses.alexande...@gmail.com>
> Fixes: a063057d7c73 ("block: Fix a race between request queue removal and the 
> block cgroup controller")
> Signed-off-by: Bart Van Assche 
> Cc: Alexandru Moise <00moses.alexande...@gmail.com>
> Cc: Tejun Heo 
> ---
>  block/blk-sysfs.c | 26 ++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index f8457d6f0190..2e134da78f82 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -788,11 +788,37 @@ static void blk_free_queue_rcu(struct rcu_head 
> *rcu_head)
>  static void __blk_release_queue(struct work_struct *work)
>  {
>   struct request_queue *q = container_of(work, typeof(*q), release_work);
> + struct blkcg_gq *gq;
>  
>   if (test_bit(QUEUE_FLAG_POLL_STATS, >queue_flags))
>   blk_stat_remove_callback(q, q->poll_cb);
>   blk_stat_free_callback(q->poll_cb);
>  
> + if (!blk_queue_dead(q)) {
> + /*
> +  * Last reference was dropped without having called
> +  * blk_cleanup_queue().
> +  */
> + WARN_ONCE(blk_queue_init_done(q),
> +   "request queue %p has been registered but 
> blk_cleanup_queue() has not been called for that queue\n",
> +   q);
> + bdi_put(q->backing_dev_info);
> + blkcg_exit_queue(q);
> +
> + if (q->elevator) {
> + ioc_clear_queue(q);
> + elevator_exit(q, q->elevator);
> + }
> + }
> +
> + rcu_read_lock();
> + gq = blkg_lookup(_root, q);
> + rcu_read_unlock();
> +
> + WARN(gq,
> +  "request queue %p is being released but it has not yet been 
> removed from the blkcg controller\n",
> +  q);
> +
>   blk_free_queue_stats(q->stats);

This solution is good. Thanks for fixing this!

Tested-by: Alexandru Moise <00moses.alexande...@gmail.com>

../Alex

>  
>   blk_exit_rl(q, >root_rl);
> -- 
> 2.16.2
>