[PATCH] dm-zoned: Fix overflow in exposed capacity

[PATCH] dm-zoned: Fix overflow in exposed capacity

[Damien Le Moal]

Cast unsigned int to sector_t before shifting. Otherwise, the target
length overflows and becomes incorrect.

Also fix an incorrect setting of the target suspend
operation.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/md/dm-zoned-target.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
index 4438f30..f947166 100644
--- a/drivers/md/dm-zoned-target.c
+++ b/drivers/md/dm-zoned-target.c
@@ -773,7 +773,7 @@ static int dmz_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	ti->split_discard_bios = true;
 
 	/* The exposed capacity is the number of chunks that can be mapped */
-	ti->len = dmz_nr_chunks(dmz->metadata) << dev->zone_nr_sectors_shift;
+	ti->len = (sector_t)dmz_nr_chunks(dmz->metadata) << dev->zone_nr_sectors_shift;
 
 	/* Zone BIO */
 	dmz->bio_set = bioset_create_nobvec(DMZ_MIN_BIOS, 0);
@@ -944,7 +944,8 @@ static struct target_type dmz_type = {
 	.end_io		 = dmz_end_io,
 	.io_hints	 = dmz_io_hints,
 	.prepare_ioctl	 = dmz_prepare_ioctl,
-	.suspend	 = dmz_suspend,
+	.presuspend	 = dmz_suspend,
+	.presuspend_undo = dmz_resume,
 	.resume		 = dmz_resume,
 	.iterate_devices = dmz_iterate_devices,
 };
-- 
2.9.4

Re: dm-zoned: Fix overflow in exposed capacity

[Mike Snitzer]

On Fri, Jun 09 2017 at 12:06am -0400,
Damien Le Moal <damien.lemoal@wdc.com> wrote:

> Cast unsigned int to sector_t before shifting. Otherwise, the target
> length overflows and becomes incorrect.
> 
> Also fix an incorrect setting of the target suspend
> operation.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/md/dm-zoned-target.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
> index 4438f30..f947166 100644
> --- a/drivers/md/dm-zoned-target.c
> +++ b/drivers/md/dm-zoned-target.c
> @@ -773,7 +773,7 @@ static int dmz_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>  	ti->split_discard_bios = true;
>  
>  	/* The exposed capacity is the number of chunks that can be mapped */
> -	ti->len = dmz_nr_chunks(dmz->metadata) << dev->zone_nr_sectors_shift;
> +	ti->len = (sector_t)dmz_nr_chunks(dmz->metadata) << dev->zone_nr_sectors_shift;
>  
>  	/* Zone BIO */
>  	dmz->bio_set = bioset_create_nobvec(DMZ_MIN_BIOS, 0);

I folded this in, thanks.

> @@ -944,7 +944,8 @@ static struct target_type dmz_type = {
>  	.end_io		 = dmz_end_io,
>  	.io_hints	 = dmz_io_hints,
>  	.prepare_ioctl	 = dmz_prepare_ioctl,
> -	.suspend	 = dmz_suspend,
> +	.presuspend	 = dmz_suspend,
> +	.presuspend_undo = dmz_resume,
>  	.resume		 = dmz_resume,
>  	.iterate_devices = dmz_iterate_devices,
>  };

I've switched from .presuspend to .postsuspend, and eliminated the
.presuspend_undo

I'm not seeing any reason for .presuspend_undo

See dm-cache-target.c for a .postsuspend that is pretty comparable to
what dm-zoned's is doing.

As for .presuspend_undo, dm-thinp has a need for it but most target do
not.  And I'm not seeing why dm-zoned needs it.

I've pushed to linux-dm.git's 'for-next' to get more test coverage;
doesn't mean that I consider dm-zoned's review complete.

Thanks,
Mike

Re: dm-zoned: Fix overflow in exposed capacity

[Damien Le Moal]

Mike,

On 6/9/17 21:03, Mike Snitzer wrote:
> I've switched from .presuspend to .postsuspend, and eliminated the
> .presuspend_undo
> 
> I'm not seeing any reason for .presuspend_undo
> 
> See dm-cache-target.c for a .postsuspend that is pretty comparable to
> what dm-zoned's is doing.
> 
> As for .presuspend_undo, dm-thinp has a need for it but most target do
> not.  And I'm not seeing why dm-zoned needs it.

Thanks for the pointer. I looked at dm-thin for examples, but that was
not the best one. I will study all these methods more.

> I've pushed to linux-dm.git's 'for-next' to get more test coverage;
> doesn't mean that I consider dm-zoned's review complete.

Thank you. Let me know if you see anything not satisfying. I will keep
testing on my side to make sure there are no obvious problems remaining.
We are also finishing a set of test case for blktests for the dm zoned
block device series. Will be submitting that soon.

Best regards.

-- 
Damien Le Moal
Western Digital Research

rebased dm-zoned changes on jens/for-4.13/block [was: Re: dm-zoned: Fix overflow in exposed capacity]

[Mike Snitzer]

On Fri, Jun 09 2017 at  8:03P -0400,
Mike Snitzer <snitzer@redhat.com> wrote:
 
> I've pushed to linux-dm.git's 'for-next' to get more test coverage;
> doesn't mean that I consider dm-zoned's review complete.

I rebased ontop of Jens' latest 'for-4.13/block' to pull in all of
Christoph's error code rework.

I had to fix various commits in your dm-zoned series.  Please
review/retest dm-zoned to verify I didn't break anything.

Thanks,
Mike

FYI, here is the incremental diff for the dm-zoned commit:

From: Mike Snitzer <snitzer@redhat.com>
Date: Mon, 12 Jun 2017 16:44:50 -0400
Subject: [PATCH] dm zoned: switch over to 4.13 error codes

---
 drivers/md/dm-zoned-metadata.c |  6 +++---
 drivers/md/dm-zoned-target.c   | 37 ++++++++++++++++++-------------------
 drivers/md/dm-zoned.h          |  2 +-
 3 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/md/dm-zoned-metadata.c b/drivers/md/dm-zoned-metadata.c
index 4618441c..b8f7d1d 100644
--- a/drivers/md/dm-zoned-metadata.c
+++ b/drivers/md/dm-zoned-metadata.c
@@ -691,14 +691,14 @@ static int dmz_log_dirty_mblocks(struct dmz_metadata *zmd,
 /*
  * Flush dirty metadata blocks.
  */
-int dmz_flush_metadata(struct dmz_metadata *zmd)
+blk_status_t dmz_flush_metadata(struct dmz_metadata *zmd)
 {
 	struct dmz_mblock *mblk;
 	struct list_head write_list;
 	int ret;
 
 	if (WARN_ON(!zmd))
-		return 0;
+		return BLK_STS_OK;
 
 	INIT_LIST_HEAD(&write_list);
 
@@ -769,7 +769,7 @@ int dmz_flush_metadata(struct dmz_metadata *zmd)
 	dmz_unlock_flush(zmd);
 	up_write(&zmd->mblk_sem);
 
-	return ret;
+	return errno_to_blk_status(ret);
 }
 
 /*
diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
index 54055f4..c9fdd02 100644
--- a/drivers/md/dm-zoned-target.c
+++ b/drivers/md/dm-zoned-target.c
@@ -20,7 +20,7 @@ struct dmz_bioctx {
 	struct dm_zone		*zone;
 	struct bio		*bio;
 	atomic_t		ref;
-	int			error;
+	blk_status_t		error;
 };
 
 /*
@@ -74,12 +74,12 @@ struct dmz_target {
 /*
  * Target BIO completion.
  */
-static inline void dmz_bio_endio(struct bio *bio, int err)
+static inline void dmz_bio_endio(struct bio *bio, blk_status_t *error)
 {
 	struct dmz_bioctx *bioctx = dm_per_bio_data(bio, sizeof(struct dmz_bioctx));
 
-	if (bioctx->error == 0 && err != 0)
-		bioctx->error = err;
+	if (bioctx->error == BLK_STS_OK && *error)
+		bioctx->error = *error;
 	bio_endio(bio);
 }
 
@@ -90,10 +90,9 @@ static inline void dmz_bio_endio(struct bio *bio, int err)
 static void dmz_read_bio_end_io(struct bio *bio)
 {
 	struct dmz_bioctx *bioctx = bio->bi_private;
-	int err = bio->bi_error;
 
 	bio_put(bio);
-	dmz_bio_endio(bioctx->bio, err);
+	dmz_bio_endio(bioctx->bio, &bio->bi_status);
 }
 
 /*
@@ -393,7 +392,7 @@ static void dmz_handle_bio(struct dmz_target *dmz, struct dm_chunk_work *cw,
 	struct dmz_bioctx *bioctx = dm_per_bio_data(bio, sizeof(struct dmz_bioctx));
 	struct dmz_metadata *zmd = dmz->metadata;
 	struct dm_zone *zone;
-	int ret;
+	blk_status_t ret;
 
 	/*
 	 * Write may trigger a zone allocation. So make sure the
@@ -436,7 +435,7 @@ static void dmz_handle_bio(struct dmz_target *dmz, struct dm_chunk_work *cw,
 	default:
 		dmz_dev_err(dmz->dev, "Unsupported BIO operation 0x%x",
 			    bio_op(bio));
-		ret = -EIO;
+		ret = BLK_STS_IOERR;
 	}
 
 	/*
@@ -446,7 +445,7 @@ static void dmz_handle_bio(struct dmz_target *dmz, struct dm_chunk_work *cw,
 	if (zone)
 		dmz_put_chunk_mapping(zmd, zone);
 out:
-	dmz_bio_endio(bio, ret);
+	dmz_bio_endio(bio, &ret);
 
 	dmz_unlock_metadata(zmd);
 }
@@ -504,7 +503,7 @@ static void dmz_flush_work(struct work_struct *work)
 {
 	struct dmz_target *dmz = container_of(work, struct dmz_target, flush_work.work);
 	struct bio *bio;
-	int ret;
+	blk_status_t ret;
 
 	/* Flush dirty metadata blocks */
 	ret = dmz_flush_metadata(dmz->metadata);
@@ -518,7 +517,7 @@ static void dmz_flush_work(struct work_struct *work)
 		if (!bio)
 			break;
 
-		dmz_bio_endio(bio, ret);
+		dmz_bio_endio(bio, &ret);
 	}
 
 	queue_delayed_work(dmz->flush_wq, &dmz->flush_work, DMZ_FLUSH_PERIOD);
@@ -593,14 +592,14 @@ static int dmz_map(struct dm_target *ti, struct bio *bio)
 
 	/* The BIO should be block aligned */
 	if ((nr_sectors & DMZ_BLOCK_SECTORS_MASK) || (sector & DMZ_BLOCK_SECTORS_MASK))
-		return -EIO;
+		return DM_MAPIO_KILL;
 
 	/* Initialize the BIO context */
 	bioctx->target = dmz;
 	bioctx->zone = NULL;
 	bioctx->bio = bio;
 	atomic_set(&bioctx->ref, 1);
-	bioctx->error = 0;
+	bioctx->error = BLK_STS_OK;
 
 	/* Set the BIO pending in the flush list */
 	if (bio_op(bio) == REQ_OP_FLUSH || (!nr_sectors && bio_op(bio) == REQ_OP_WRITE)) {
@@ -626,30 +625,30 @@ static int dmz_map(struct dm_target *ti, struct bio *bio)
 /*
  * Completed target BIO processing.
  */
-static int dmz_end_io(struct dm_target *ti, struct bio *bio, int err)
+static int dmz_end_io(struct dm_target *ti, struct bio *bio, blk_status_t *error)
 {
 	struct dmz_bioctx *bioctx = dm_per_bio_data(bio, sizeof(struct dmz_bioctx));
 
-	if (bioctx->error == 0 && err != 0)
-		bioctx->error = err;
+	if (bioctx->error == BLK_STS_OK && *error)
+		bioctx->error = *error;
 
 	if (!atomic_dec_and_test(&bioctx->ref))
 		return DM_ENDIO_INCOMPLETE;
 
 	/* Done */
-	bio->bi_error = bioctx->error;
+	bio->bi_status = bioctx->error;
 
 	if (bioctx->zone) {
 		struct dm_zone *zone = bioctx->zone;
 
-		if (err && bio_op(bio) == REQ_OP_WRITE) {
+		if (*error && bio_op(bio) == REQ_OP_WRITE) {
 			if (dmz_is_seq(zone))
 				set_bit(DMZ_SEQ_WRITE_ERR, &zone->flags);
 		}
 		dmz_deactivate_zone(zone);
 	}
 
-	return bioctx->error;
+	return DM_ENDIO_DONE;
 }
 
 /*
diff --git a/drivers/md/dm-zoned.h b/drivers/md/dm-zoned.h
index 12419f0..76ebcf8 100644
--- a/drivers/md/dm-zoned.h
+++ b/drivers/md/dm-zoned.h
@@ -169,7 +169,7 @@ void dmz_lock_metadata(struct dmz_metadata *zmd);
 void dmz_unlock_metadata(struct dmz_metadata *zmd);
 void dmz_lock_flush(struct dmz_metadata *zmd);
 void dmz_unlock_flush(struct dmz_metadata *zmd);
-int dmz_flush_metadata(struct dmz_metadata *zmd);
+blk_status_t dmz_flush_metadata(struct dmz_metadata *zmd);
 
 unsigned int dmz_id(struct dmz_metadata *zmd, struct dm_zone *zone);
 sector_t dmz_start_sect(struct dmz_metadata *zmd, struct dm_zone *zone);
-- 
2.10.1

Re: rebased dm-zoned changes on jens/for-4.13/block [was: Re: dm-zoned: Fix overflow in exposed capacity]

[Damien Le Moal]

Mike,

Thank you. I just posted a follow up patch with a fix for a
reference-after-free (see below). I tested without problems.

On 6/12/17 14:28, Mike Snitzer wrote:
> On Fri, Jun 09 2017 at  8:03P -0400,
> Mike Snitzer <snitzer@redhat.com> wrote:
>  
>> I've pushed to linux-dm.git's 'for-next' to get more test coverage;
>> doesn't mean that I consider dm-zoned's review complete.
> 
> I rebased ontop of Jens' latest 'for-4.13/block' to pull in all of
> Christoph's error code rework.
> 
> I had to fix various commits in your dm-zoned series.  Please
> review/retest dm-zoned to verify I didn't break anything.
> 
> Thanks,
> Mike
> 
> FYI, here is the incremental diff for the dm-zoned commit:
> 
> From: Mike Snitzer <snitzer@redhat.com>
> Date: Mon, 12 Jun 2017 16:44:50 -0400
> Subject: [PATCH] dm zoned: switch over to 4.13 error codes
> 
> ---
>  drivers/md/dm-zoned-metadata.c |  6 +++---
>  drivers/md/dm-zoned-target.c   | 37 ++++++++++++++++++-------------------
>  drivers/md/dm-zoned.h          |  2 +-
>  3 files changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/md/dm-zoned-metadata.c b/drivers/md/dm-zoned-metadata.c
> index 4618441c..b8f7d1d 100644
> --- a/drivers/md/dm-zoned-metadata.c
> +++ b/drivers/md/dm-zoned-metadata.c
> @@ -691,14 +691,14 @@ static int dmz_log_dirty_mblocks(struct dmz_metadata *zmd,
>  /*
>   * Flush dirty metadata blocks.
>   */
> -int dmz_flush_metadata(struct dmz_metadata *zmd)
> +blk_status_t dmz_flush_metadata(struct dmz_metadata *zmd)
>  {
>  	struct dmz_mblock *mblk;
>  	struct list_head write_list;
>  	int ret;
>  
>  	if (WARN_ON(!zmd))
> -		return 0;
> +		return BLK_STS_OK;
>  
>  	INIT_LIST_HEAD(&write_list);
>  
> @@ -769,7 +769,7 @@ int dmz_flush_metadata(struct dmz_metadata *zmd)
>  	dmz_unlock_flush(zmd);
>  	up_write(&zmd->mblk_sem);
>  
> -	return ret;
> +	return errno_to_blk_status(ret);
>  }

I changed this back to returning an errno and do the
errno_to_blk_status() conversion when calling dm_zoned_endio(). Doing
so, all other functions in dm-zoned-target.c returning an errno can be
left as is.

>  /*
> diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
> index 54055f4..c9fdd02 100644
> --- a/drivers/md/dm-zoned-target.c
> +++ b/drivers/md/dm-zoned-target.c
> @@ -20,7 +20,7 @@ struct dmz_bioctx {
>  	struct dm_zone		*zone;
>  	struct bio		*bio;
>  	atomic_t		ref;
> -	int			error;
> +	blk_status_t		error;
>  };
>  
>  /*
> @@ -74,12 +74,12 @@ struct dmz_target {
>  /*
>   * Target BIO completion.
>   */
> -static inline void dmz_bio_endio(struct bio *bio, int err)
> +static inline void dmz_bio_endio(struct bio *bio, blk_status_t *error)
>  {
>  	struct dmz_bioctx *bioctx = dm_per_bio_data(bio, sizeof(struct dmz_bioctx));
>  
> -	if (bioctx->error == 0 && err != 0)
> -		bioctx->error = err;
> +	if (bioctx->error == BLK_STS_OK && *error)
> +		bioctx->error = *error;
>  	bio_endio(bio);
>  }

I changed this to "blk_status_t error". No need for a pointer.

>  
> @@ -90,10 +90,9 @@ static inline void dmz_bio_endio(struct bio *bio, int err)
>  static void dmz_read_bio_end_io(struct bio *bio)
>  {
>  	struct dmz_bioctx *bioctx = bio->bi_private;
> -	int err = bio->bi_error;
>  
>  	bio_put(bio);
> -	dmz_bio_endio(bioctx->bio, err);
> +	dmz_bio_endio(bioctx->bio, &bio->bi_status);
>  }

There is a reference after free here. So we need to keep the on stack
status.

>  
>  /*
> @@ -393,7 +392,7 @@ static void dmz_handle_bio(struct dmz_target *dmz, struct dm_chunk_work *cw,
>  	struct dmz_bioctx *bioctx = dm_per_bio_data(bio, sizeof(struct dmz_bioctx));
>  	struct dmz_metadata *zmd = dmz->metadata;
>  	struct dm_zone *zone;
> -	int ret;
> +	blk_status_t ret;
>  
>  	/*
>  	 * Write may trigger a zone allocation. So make sure the
> @@ -436,7 +435,7 @@ static void dmz_handle_bio(struct dmz_target *dmz, struct dm_chunk_work *cw,
>  	default:
>  		dmz_dev_err(dmz->dev, "Unsupported BIO operation 0x%x",
>  			    bio_op(bio));
> -		ret = -EIO;
> +		ret = BLK_STS_IOERR;
>  	}

The conversion to blk_status_t is done when calling dm_zoned_endio()
after this. So I reverted this change.

>  
>  	/*
> @@ -446,7 +445,7 @@ static void dmz_handle_bio(struct dmz_target *dmz, struct dm_chunk_work *cw,
>  	if (zone)
>  		dmz_put_chunk_mapping(zmd, zone);
>  out:
> -	dmz_bio_endio(bio, ret);
> +	dmz_bio_endio(bio, &ret);
>  
>  	dmz_unlock_metadata(zmd);
>  }
> @@ -504,7 +503,7 @@ static void dmz_flush_work(struct work_struct *work)
>  {
>  	struct dmz_target *dmz = container_of(work, struct dmz_target, flush_work.work);
>  	struct bio *bio;
> -	int ret;
> +	blk_status_t ret;
>  
>  	/* Flush dirty metadata blocks */
>  	ret = dmz_flush_metadata(dmz->metadata);
> @@ -518,7 +517,7 @@ static void dmz_flush_work(struct work_struct *work)
>  		if (!bio)
>  			break;
>  
> -		dmz_bio_endio(bio, ret);
> +		dmz_bio_endio(bio, &ret);
>  	}
>  
>  	queue_delayed_work(dmz->flush_wq, &dmz->flush_work, DMZ_FLUSH_PERIOD);
> @@ -593,14 +592,14 @@ static int dmz_map(struct dm_target *ti, struct bio *bio)
>  
>  	/* The BIO should be block aligned */
>  	if ((nr_sectors & DMZ_BLOCK_SECTORS_MASK) || (sector & DMZ_BLOCK_SECTORS_MASK))
> -		return -EIO;
> +		return DM_MAPIO_KILL;
>  
>  	/* Initialize the BIO context */
>  	bioctx->target = dmz;
>  	bioctx->zone = NULL;
>  	bioctx->bio = bio;
>  	atomic_set(&bioctx->ref, 1);
> -	bioctx->error = 0;
> +	bioctx->error = BLK_STS_OK;
>  
>  	/* Set the BIO pending in the flush list */
>  	if (bio_op(bio) == REQ_OP_FLUSH || (!nr_sectors && bio_op(bio) == REQ_OP_WRITE)) {
> @@ -626,30 +625,30 @@ static int dmz_map(struct dm_target *ti, struct bio *bio)
>  /*
>   * Completed target BIO processing.
>   */
> -static int dmz_end_io(struct dm_target *ti, struct bio *bio, int err)
> +static int dmz_end_io(struct dm_target *ti, struct bio *bio, blk_status_t *error)
>  {
>  	struct dmz_bioctx *bioctx = dm_per_bio_data(bio, sizeof(struct dmz_bioctx));
>  
> -	if (bioctx->error == 0 && err != 0)
> -		bioctx->error = err;
> +	if (bioctx->error == BLK_STS_OK && *error)
> +		bioctx->error = *error;
>  
>  	if (!atomic_dec_and_test(&bioctx->ref))
>  		return DM_ENDIO_INCOMPLETE;
>  
>  	/* Done */
> -	bio->bi_error = bioctx->error;
> +	bio->bi_status = bioctx->error;
>  
>  	if (bioctx->zone) {
>  		struct dm_zone *zone = bioctx->zone;
>  
> -		if (err && bio_op(bio) == REQ_OP_WRITE) {
> +		if (*error && bio_op(bio) == REQ_OP_WRITE) {
>  			if (dmz_is_seq(zone))
>  				set_bit(DMZ_SEQ_WRITE_ERR, &zone->flags);
>  		}
>  		dmz_deactivate_zone(zone);
>  	}
>  
> -	return bioctx->error;
> +	return DM_ENDIO_DONE;
>  }
>  
>  /*
> diff --git a/drivers/md/dm-zoned.h b/drivers/md/dm-zoned.h
> index 12419f0..76ebcf8 100644
> --- a/drivers/md/dm-zoned.h
> +++ b/drivers/md/dm-zoned.h
> @@ -169,7 +169,7 @@ void dmz_lock_metadata(struct dmz_metadata *zmd);
>  void dmz_unlock_metadata(struct dmz_metadata *zmd);
>  void dmz_lock_flush(struct dmz_metadata *zmd);
>  void dmz_unlock_flush(struct dmz_metadata *zmd);
> -int dmz_flush_metadata(struct dmz_metadata *zmd);
> +blk_status_t dmz_flush_metadata(struct dmz_metadata *zmd);
>  
>  unsigned int dmz_id(struct dmz_metadata *zmd, struct dm_zone *zone);
>  sector_t dmz_start_sect(struct dmz_metadata *zmd, struct dm_zone *zone);
> 

Best regards.

-- 
Damien Le Moal,
Western Digital Research