[PATCH] dm: initialize queuedata and congested_data early

[PATCH] dm: initialize queuedata and congested_data early

[Mikulas Patocka]

This fixes a possible race when md->queue->backing_dev_info.congested_fn 
is changed.

Note that Zdenek is seeing some other memory corruption where 
dm_any_congested is called with invalid argument, but it is unlikely to be 
fixed by this patch.

Mikulas

-

From: Mikulas Patocka <mpatocka@redhat.com>

The patch bfebd1cdb497a57757c83f5fbf1a29931591e2a4 ("dm: add full blk-mq
support to request-based DM") moves the initialization of fields
queuedata, backing_dev_info.congested_fn and
backing_dev_info.congested_data from the function dm_init_md_queue (that
is called when the device is created) to dm_init_old_md_queue (that is
called when type of device is determined).

There is no locking when accessing these variables, thus it is possible
that other part of the kernel sees queue->backing_dev_info.congested_fn
initialized and md->queue->backing_dev_info.congested_data uninitialized,
passing incorrect parameter to the function dm_any_congested.

This patch fixes this race condition by moving initialization of queuedata
and backing_dev_info.congested_data to the function dm_init_md_queue, so
that these values are initialized when the device is created.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Fixes: bfebd1cdb497 ("dm: add full blk-mq support to request-based DM")
Cc: stable@vger.kernel.org	# v4.1+

---
 drivers/md/dm.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux-4.3-rc7/drivers/md/dm.c
===================================================================
--- linux-4.3-rc7.orig/drivers/md/dm.c	2015-10-27 23:25:41.000000000 +0100
+++ linux-4.3-rc7/drivers/md/dm.c	2015-10-27 23:26:45.000000000 +0100
@@ -2198,6 +2198,9 @@ static void dm_init_md_queue(struct mapp
 	 * This queue is new, so no concurrency on the queue_flags.
 	 */
 	queue_flag_clear_unlocked(QUEUE_FLAG_STACKABLE, md->queue);
+
+	md->queue->queuedata = md;
+	md->queue->backing_dev_info.congested_data = md;
 }
 
 static void dm_init_old_md_queue(struct mapped_device *md)
@@ -2208,9 +2211,7 @@ static void dm_init_old_md_queue(struct 
 	/*
 	 * Initialize aspects of queue that aren't relevant for blk-mq
 	 */
-	md->queue->queuedata = md;
 	md->queue->backing_dev_info.congested_fn = dm_any_congested;
-	md->queue->backing_dev_info.congested_data = md;
 
 	blk_queue_bounce_limit(md->queue, BLK_BOUNCE_ANY);
 }

Re: dm: initialize queuedata and congested_data early

[Mike Snitzer]

On Tue, Oct 27 2015 at  7:06pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> This fixes a possible race when md->queue->backing_dev_info.congested_fn 
> is changed.
> 
> Note that Zdenek is seeing some other memory corruption where 
> dm_any_congested is called with invalid argument, but it is unlikely to be 
> fixed by this patch.
> 
> Mikulas
> 
> -
> 
> From: Mikulas Patocka <mpatocka@redhat.com>
> 
> The patch bfebd1cdb497a57757c83f5fbf1a29931591e2a4 ("dm: add full blk-mq
> support to request-based DM") moves the initialization of fields
> queuedata, backing_dev_info.congested_fn and
> backing_dev_info.congested_data from the function dm_init_md_queue (that
> is called when the device is created) to dm_init_old_md_queue (that is
> called when type of device is determined).
> 
> There is no locking when accessing these variables, thus it is possible
> that other part of the kernel sees queue->backing_dev_info.congested_fn
> initialized and md->queue->backing_dev_info.congested_data uninitialized,
> passing incorrect parameter to the function dm_any_congested.
> 
> This patch fixes this race condition by moving initialization of queuedata
> and backing_dev_info.congested_data to the function dm_init_md_queue, so
> that these values are initialized when the device is created.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Fixes: bfebd1cdb497 ("dm: add full blk-mq support to request-based DM")
> Cc: stable@vger.kernel.org	# v4.1+
> 
> ---
>  drivers/md/dm.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Index: linux-4.3-rc7/drivers/md/dm.c
> ===================================================================
> --- linux-4.3-rc7.orig/drivers/md/dm.c	2015-10-27 23:25:41.000000000 +0100
> +++ linux-4.3-rc7/drivers/md/dm.c	2015-10-27 23:26:45.000000000 +0100
> @@ -2198,6 +2198,9 @@ static void dm_init_md_queue(struct mapp
>  	 * This queue is new, so no concurrency on the queue_flags.
>  	 */
>  	queue_flag_clear_unlocked(QUEUE_FLAG_STACKABLE, md->queue);
> +
> +	md->queue->queuedata = md;
> +	md->queue->backing_dev_info.congested_data = md;
>  }

I don't like these moving to dm_init_md_queue() because they aren't
needed for blk-mq.  No sense establishing data that will go unused in
the blk-mq case.
  
>  static void dm_init_old_md_queue(struct mapped_device *md)
> @@ -2208,9 +2211,7 @@ static void dm_init_old_md_queue(struct 
>  	/*
>  	 * Initialize aspects of queue that aren't relevant for blk-mq
>  	 */
> -	md->queue->queuedata = md;
>  	md->queue->backing_dev_info.congested_fn = dm_any_congested;
> -	md->queue->backing_dev_info.congested_data = md;
>  
>  	blk_queue_bounce_limit(md->queue, BLK_BOUNCE_ANY);
>  }

Wouldn't it be sufficient to simply reorder so the congested_fn
assignment is last?  E.g.:

	md->queue->queuedata = md;
	md->queue->backing_dev_info.congested_data = md;
	md->queue->backing_dev_info.congested_fn = dm_any_congested;

Re: dm: initialize queuedata and congested_data early

[Mikulas Patocka]

On Wed, 28 Oct 2015, Mike Snitzer wrote:

> On Tue, Oct 27 2015 at  7:06pm -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > This fixes a possible race when md->queue->backing_dev_info.congested_fn 
> > is changed.
> > 
> > Note that Zdenek is seeing some other memory corruption where 
> > dm_any_congested is called with invalid argument, but it is unlikely to be 
> > fixed by this patch.
> > 
> > Mikulas
> > 
> > -
> > 
> > From: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > The patch bfebd1cdb497a57757c83f5fbf1a29931591e2a4 ("dm: add full blk-mq
> > support to request-based DM") moves the initialization of fields
> > queuedata, backing_dev_info.congested_fn and
> > backing_dev_info.congested_data from the function dm_init_md_queue (that
> > is called when the device is created) to dm_init_old_md_queue (that is
> > called when type of device is determined).
> > 
> > There is no locking when accessing these variables, thus it is possible
> > that other part of the kernel sees queue->backing_dev_info.congested_fn
> > initialized and md->queue->backing_dev_info.congested_data uninitialized,
> > passing incorrect parameter to the function dm_any_congested.
> > 
> > This patch fixes this race condition by moving initialization of queuedata
> > and backing_dev_info.congested_data to the function dm_init_md_queue, so
> > that these values are initialized when the device is created.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Fixes: bfebd1cdb497 ("dm: add full blk-mq support to request-based DM")
> > Cc: stable@vger.kernel.org	# v4.1+
> > 
> > ---
> >  drivers/md/dm.c |    5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > Index: linux-4.3-rc7/drivers/md/dm.c
> > ===================================================================
> > --- linux-4.3-rc7.orig/drivers/md/dm.c	2015-10-27 23:25:41.000000000 +0100
> > +++ linux-4.3-rc7/drivers/md/dm.c	2015-10-27 23:26:45.000000000 +0100
> > @@ -2198,6 +2198,9 @@ static void dm_init_md_queue(struct mapp
> >  	 * This queue is new, so no concurrency on the queue_flags.
> >  	 */
> >  	queue_flag_clear_unlocked(QUEUE_FLAG_STACKABLE, md->queue);
> > +
> > +	md->queue->queuedata = md;
> > +	md->queue->backing_dev_info.congested_data = md;
> >  }
> 
> I don't like these moving to dm_init_md_queue() because they aren't
> needed for blk-mq.  No sense establishing data that will go unused in
> the blk-mq case.

Another possibility is to always set congested_fn = dm_any_congested and 
test in dm_any_congested if the device is multiqueue.

> >  static void dm_init_old_md_queue(struct mapped_device *md)
> > @@ -2208,9 +2211,7 @@ static void dm_init_old_md_queue(struct 
> >  	/*
> >  	 * Initialize aspects of queue that aren't relevant for blk-mq
> >  	 */
> > -	md->queue->queuedata = md;
> >  	md->queue->backing_dev_info.congested_fn = dm_any_congested;
> > -	md->queue->backing_dev_info.congested_data = md;
> >  
> >  	blk_queue_bounce_limit(md->queue, BLK_BOUNCE_ANY);
> >  }
> 
> Wouldn't it be sufficient to simply reorder so the congested_fn
> assignment is last?  E.g.:
> 
> 	md->queue->queuedata = md;
> 	md->queue->backing_dev_info.congested_data = md;
> 	md->queue->backing_dev_info.congested_fn = dm_any_congested;

This is incorrect - the compiler or processor may reorder memory accesses 
- it needs write barrier when modifying the fields and read barrier when 
reading them.

The write barrier could be added here, but the read barrier would slow 
down all calls to the function congested_fn.

Mikulas

Re: dm: initialize queuedata and congested_data early

[Mike Snitzer]

On Thu, Oct 29 2015 at 12:20pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Wed, 28 Oct 2015, Mike Snitzer wrote:
> 
> > On Tue, Oct 27 2015 at  7:06pm -0400,
> > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > 
> > > This fixes a possible race when md->queue->backing_dev_info.congested_fn 
> > > is changed.
> > > 
> > > Note that Zdenek is seeing some other memory corruption where 
> > > dm_any_congested is called with invalid argument, but it is unlikely to be 
> > > fixed by this patch.
> > > 
> > > Mikulas
> > > 
> > > -
> > > 
> > > From: Mikulas Patocka <mpatocka@redhat.com>
> > > 
> > > The patch bfebd1cdb497a57757c83f5fbf1a29931591e2a4 ("dm: add full blk-mq
> > > support to request-based DM") moves the initialization of fields
> > > queuedata, backing_dev_info.congested_fn and
> > > backing_dev_info.congested_data from the function dm_init_md_queue (that
> > > is called when the device is created) to dm_init_old_md_queue (that is
> > > called when type of device is determined).
> > > 
> > > There is no locking when accessing these variables, thus it is possible
> > > that other part of the kernel sees queue->backing_dev_info.congested_fn
> > > initialized and md->queue->backing_dev_info.congested_data uninitialized,
> > > passing incorrect parameter to the function dm_any_congested.
> > > 
> > > This patch fixes this race condition by moving initialization of queuedata
> > > and backing_dev_info.congested_data to the function dm_init_md_queue, so
> > > that these values are initialized when the device is created.
> > > 
> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > Fixes: bfebd1cdb497 ("dm: add full blk-mq support to request-based DM")
> > > Cc: stable@vger.kernel.org	# v4.1+
> > > 
> > > ---
> > >  drivers/md/dm.c |    5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > Index: linux-4.3-rc7/drivers/md/dm.c
> > > ===================================================================
> > > --- linux-4.3-rc7.orig/drivers/md/dm.c	2015-10-27 23:25:41.000000000 +0100
> > > +++ linux-4.3-rc7/drivers/md/dm.c	2015-10-27 23:26:45.000000000 +0100
> > > @@ -2198,6 +2198,9 @@ static void dm_init_md_queue(struct mapp
> > >  	 * This queue is new, so no concurrency on the queue_flags.
> > >  	 */
> > >  	queue_flag_clear_unlocked(QUEUE_FLAG_STACKABLE, md->queue);
> > > +
> > > +	md->queue->queuedata = md;
> > > +	md->queue->backing_dev_info.congested_data = md;
> > >  }
> > 
> > I don't like these moving to dm_init_md_queue() because they aren't
> > needed for blk-mq.  No sense establishing data that will go unused in
> > the blk-mq case.
> 
> Another possibility is to always set congested_fn = dm_any_congested and 
> test in dm_any_congested if the device is multiqueue.
> 
> > >  static void dm_init_old_md_queue(struct mapped_device *md)
> > > @@ -2208,9 +2211,7 @@ static void dm_init_old_md_queue(struct 
> > >  	/*
> > >  	 * Initialize aspects of queue that aren't relevant for blk-mq
> > >  	 */
> > > -	md->queue->queuedata = md;
> > >  	md->queue->backing_dev_info.congested_fn = dm_any_congested;
> > > -	md->queue->backing_dev_info.congested_data = md;
> > >  
> > >  	blk_queue_bounce_limit(md->queue, BLK_BOUNCE_ANY);
> > >  }
> > 
> > Wouldn't it be sufficient to simply reorder so the congested_fn
> > assignment is last?  E.g.:
> > 
> > 	md->queue->queuedata = md;
> > 	md->queue->backing_dev_info.congested_data = md;
> > 	md->queue->backing_dev_info.congested_fn = dm_any_congested;
> 
> This is incorrect - the compiler or processor may reorder memory accesses 
> - it needs write barrier when modifying the fields and read barrier when 
> reading them.
> 
> The write barrier could be added here, but the read barrier would slow 
> down all calls to the function congested_fn.

Fair point.  I'll get your patch staged for 4.4 and cc stable.