diff mbox series

[3/3] block: don't free submitter owned integrity payload on I/O completion

Message ID 20240628132736.668184-4-hch@lst.de (mailing list archive)
State New, archived
Headers show
Series [1/3] block: split integrity support out of bio.h | expand

Commit Message

Christoph Hellwig June 28, 2024, 1:27 p.m. UTC 
Currently __bio_integrity_endio unconditionally frees the integrity
payload.  While this works really well for block-layer generated
integrity payloads, it is a bad idea for those passed in by the
submitter, as it can't access the integrity data from the I/O completion
handler.

Change bio_integrity_endio to only call __bio_integrity_endio for
block layer generated integrity data, and leave freeing of submitter
allocated integrity data to bio_uninit which also gets called from
the final bio_put.  This requires that unmapping user mapped or copied
integrity data is done by the caller now.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio-integrity.c         | 56 ++++++++++++++++-------------------
 block/blk-map.c               |  3 ++
 block/blk.h                   |  4 ++-
 include/linux/bio-integrity.h |  8 +++--
 4 files changed, 37 insertions(+), 34 deletions(-)

Comments

Kanchan Joshi June 28, 2024, 4:16 p.m. UTC | #1 
On 6/28/2024 6:57 PM, Christoph Hellwig wrote:

Thanks for streamlining this. Few comments below.

>   static inline bool bio_integrity_endio(struct bio *bio)
>   {
> -	if (bio_integrity(bio))
> +	struct bio_integrity_payload *bip = bio_integrity(bio);
> +
> +	if (bip && (bip->bip_flags & BIP_BLOCK_INTEGRITY))
>   		return __bio_integrity_endio(bio);

Seems we will need similar BIP_BLOCK_INTEGRITY check inside bio_uninit 
too. At line 221 in below snippet [1].
As that also frees integrity by calling bio_integrity_free. Earlier that 
free was skipped due to BIP_INTEGRITY_USER flag. Now that flag has gone, 
integrity will get freed and after that control may go back to 
nvme-passthrough where it will try to unmap (and potentially copy back) 
integrity.

>   	return true;
>   }
> diff --git a/include/linux/bio-integrity.h b/include/linux/bio-integrity.h
> index 7b6e687d436de2..a5a8b44e9b118f 100644
> --- a/include/linux/bio-integrity.h
> +++ b/include/linux/bio-integrity.h
> @@ -10,8 +10,7 @@ enum bip_flags {
>   	BIP_CTRL_NOCHECK	= 1 << 2, /* disable HBA integrity checking */
>   	BIP_DISK_NOCHECK	= 1 << 3, /* disable disk integrity checking */
>   	BIP_IP_CHECKSUM		= 1 << 4, /* IP checksum */
> -	BIP_INTEGRITY_USER	= 1 << 5, /* Integrity payload is user address */

Seems this patch is with "for-6.11/block".
But with "for-next" you will see more places where this flag has been 
used. And there will be conflicts because we have this patch there [1]. 
Parts of this patch need changes to work with it. I can look more and 
test next week.

[1]
commit e038ee6189842e9662d2fc59d09dbcf48350cf99
Author: Anuj Gupta <anuj20.g@samsung.com>
Date:   Mon Jun 10 16:41:44 2024 +0530

     block: unmap and free user mapped integrity via submitter

     The user mapped intergity is copied back and unpinned by
     bio_integrity_free which is a low-level routine. Do it via the 
submitter
     rather than doing it in the low-level block layer code, to split the
     submitter side from the consumer side of the bio.

[2]
213 void bio_uninit(struct bio *bio)
214 {
215 #ifdef CONFIG_BLK_CGROUP
216         if (bio->bi_blkg) {
217                 blkg_put(bio->bi_blkg);
218                 bio->bi_blkg = NULL;
219         }
220 #endif
221         if (bio_integrity(bio))
222                 bio_integrity_free(bio);
Christoph Hellwig June 29, 2024, 5:02 a.m. UTC | #2 
On Fri, Jun 28, 2024 at 09:46:21PM +0530, Kanchan Joshi wrote:
> >   static inline bool bio_integrity_endio(struct bio *bio)
> >   {
> > -	if (bio_integrity(bio))
> > +	struct bio_integrity_payload *bip = bio_integrity(bio);
> > +
> > +	if (bip && (bip->bip_flags & BIP_BLOCK_INTEGRITY))
> >   		return __bio_integrity_endio(bio);
> 
> Seems we will need similar BIP_BLOCK_INTEGRITY check inside bio_uninit 
> too. At line 221 in below snippet [1].
> As that also frees integrity by calling bio_integrity_free. Earlier that 
> free was skipped due to BIP_INTEGRITY_USER flag. Now that flag has gone, 
> integrity will get freed and after that control may go back to 
> nvme-passthrough where it will try to unmap (and potentially copy back) 
> integrity.

bio_integrity_free clears the REQ_INTEGRITY flag and the bi_integrity
pointer, and bio_uninit checks for those using bio_integrity() first.
The check is alredy required for the existing code and that doesn't
change.

> Seems this patch is with "for-6.11/block".
> But with "for-next" you will see more places where this flag has been 
> used. And there will be conflicts because we have this patch there [1]. 
> Parts of this patch need changes to work with it. I can look more and 
> test next week.

Oh, the patch actually does part of what this one does, _and_ I've
reviewed it.  Jens, maybe just skip the patch we are replying to
(my one) for now, and I'll resend it for the next merge window
as the conflicts vs the 6.10 tree would be too annoying.
diff mbox series

Patch

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index ad296849aa2a9a..8c6447bcddede2 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -22,9 +22,17 @@  void blk_flush_integrity(void)
 	flush_workqueue(kintegrityd_wq);
 }
 
-static void __bio_integrity_free(struct bio_set *bs,
-				 struct bio_integrity_payload *bip)
+/**
+ * bio_integrity_free - Free bio integrity payload
+ * @bio:	bio containing bip to be freed
+ *
+ * Description: Free the integrity portion of a bio.
+ */
+void bio_integrity_free(struct bio *bio)
 {
+	struct bio_integrity_payload *bip = bio_integrity(bio);
+	struct bio_set *bs = bio->bi_pool;
+
 	if (bs && mempool_initialized(&bs->bio_integrity_pool)) {
 		if (bip->bip_vec)
 			bvec_free(&bs->bvec_integrity_pool, bip->bip_vec,
@@ -33,6 +41,8 @@  static void __bio_integrity_free(struct bio_set *bs,
 	} else {
 		kfree(bip);
 	}
+	bio->bi_integrity = NULL;
+	bio->bi_opf &= ~REQ_INTEGRITY;
 }
 
 /**
@@ -86,7 +96,10 @@  struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio,
 
 	return bip;
 err:
-	__bio_integrity_free(bs, bip);
+	if (bs && mempool_initialized(&bs->bio_integrity_pool))
+		mempool_free(bip, &bs->bio_integrity_pool);
+	else
+		kfree(bip);
 	return ERR_PTR(-ENOMEM);
 }
 EXPORT_SYMBOL(bio_integrity_alloc);
@@ -118,9 +131,10 @@  static void bio_integrity_uncopy_user(struct bio_integrity_payload *bip)
 	bio_integrity_unpin_bvec(copy, nr_vecs, true);
 }
 
-static void bio_integrity_unmap_user(struct bio_integrity_payload *bip)
+void bio_integrity_unmap_user(struct bio *bio)
 {
-	bool dirty = bio_data_dir(bip->bip_bio) == READ;
+	struct bio_integrity_payload *bip = bio_integrity(bio);
+	bool dirty = bio_data_dir(bio) == READ;
 
 	if (bip->bip_flags & BIP_COPY_USER) {
 		if (dirty)
@@ -131,28 +145,7 @@  static void bio_integrity_unmap_user(struct bio_integrity_payload *bip)
 
 	bio_integrity_unpin_bvec(bip->bip_vec, bip->bip_max_vcnt, dirty);
 }
-
-/**
- * bio_integrity_free - Free bio integrity payload
- * @bio:	bio containing bip to be freed
- *
- * Description: Used to free the integrity portion of a bio. Usually
- * called from bio_free().
- */
-void bio_integrity_free(struct bio *bio)
-{
-	struct bio_integrity_payload *bip = bio_integrity(bio);
-	struct bio_set *bs = bio->bi_pool;
-
-	if (bip->bip_flags & BIP_BLOCK_INTEGRITY)
-		kfree(bvec_virt(bip->bip_vec));
-	else if (bip->bip_flags & BIP_INTEGRITY_USER)
-		bio_integrity_unmap_user(bip);
-
-	__bio_integrity_free(bs, bip);
-	bio->bi_integrity = NULL;
-	bio->bi_opf &= ~REQ_INTEGRITY;
-}
+EXPORT_SYMBOL_GPL(bio_integrity_unmap_user);
 
 /**
  * bio_integrity_add_page - Attach integrity metadata
@@ -252,7 +245,7 @@  static int bio_integrity_copy_user(struct bio *bio, struct bio_vec *bvec,
 		goto free_bip;
 	}
 
-	bip->bip_flags |= BIP_INTEGRITY_USER | BIP_COPY_USER;
+	bip->bip_flags |= BIP_COPY_USER;
 	bip->bip_iter.bi_sector = seed;
 	return 0;
 free_bip:
@@ -272,7 +265,6 @@  static int bio_integrity_init_user(struct bio *bio, struct bio_vec *bvec,
 		return PTR_ERR(bip);
 
 	memcpy(bip->bip_vec, bvec, nr_vecs * sizeof(*bvec));
-	bip->bip_flags |= BIP_INTEGRITY_USER;
 	bip->bip_iter.bi_sector = seed;
 	bip->bip_iter.bi_size = len;
 	return 0;
@@ -479,6 +471,8 @@  static void bio_integrity_verify_fn(struct work_struct *work)
 	struct bio *bio = bip->bip_bio;
 
 	blk_integrity_verify(bio);
+
+	kfree(bvec_virt(bip->bip_vec));
 	bio_integrity_free(bio);
 	bio_endio(bio);
 }
@@ -499,13 +493,13 @@  bool __bio_integrity_endio(struct bio *bio)
 	struct blk_integrity *bi = blk_get_integrity(bio->bi_bdev->bd_disk);
 	struct bio_integrity_payload *bip = bio_integrity(bio);
 
-	if (bio_op(bio) == REQ_OP_READ && !bio->bi_status &&
-	    (bip->bip_flags & BIP_BLOCK_INTEGRITY) && bi->csum_type) {
+	if (bio_op(bio) == REQ_OP_READ && !bio->bi_status && bi->csum_type) {
 		INIT_WORK(&bip->bip_work, bio_integrity_verify_fn);
 		queue_work(kintegrityd_wq, &bip->bip_work);
 		return false;
 	}
 
+	kfree(bvec_virt(bip->bip_vec));
 	bio_integrity_free(bio);
 	return true;
 }
diff --git a/block/blk-map.c b/block/blk-map.c
index bce144091128f6..0e1167b239342f 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -757,6 +757,9 @@  int blk_rq_unmap_user(struct bio *bio)
 			bio_release_pages(bio, bio_data_dir(bio) == READ);
 		}
 
+		if (bio_integrity(bio))
+			bio_integrity_unmap_user(bio);
+
 		next_bio = bio;
 		bio = bio->bi_next;
 		blk_mq_map_bio_put(next_bio);
diff --git a/block/blk.h b/block/blk.h
index 401e604f35d2cf..e8a6e7497c8d36 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -206,7 +206,9 @@  bool __bio_integrity_endio(struct bio *);
 void bio_integrity_free(struct bio *bio);
 static inline bool bio_integrity_endio(struct bio *bio)
 {
-	if (bio_integrity(bio))
+	struct bio_integrity_payload *bip = bio_integrity(bio);
+
+	if (bip && (bip->bip_flags & BIP_BLOCK_INTEGRITY))
 		return __bio_integrity_endio(bio);
 	return true;
 }
diff --git a/include/linux/bio-integrity.h b/include/linux/bio-integrity.h
index 7b6e687d436de2..a5a8b44e9b118f 100644
--- a/include/linux/bio-integrity.h
+++ b/include/linux/bio-integrity.h
@@ -10,8 +10,7 @@  enum bip_flags {
 	BIP_CTRL_NOCHECK	= 1 << 2, /* disable HBA integrity checking */
 	BIP_DISK_NOCHECK	= 1 << 3, /* disable disk integrity checking */
 	BIP_IP_CHECKSUM		= 1 << 4, /* IP checksum */
-	BIP_INTEGRITY_USER	= 1 << 5, /* Integrity payload is user address */
-	BIP_COPY_USER		= 1 << 6, /* Kernel bounce buffer in use */
+	BIP_COPY_USER		= 1 << 5, /* Kernel bounce buffer in use */
 };
 
 struct bio_integrity_payload {
@@ -74,6 +73,7 @@  struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio, gfp_t gfp,
 int bio_integrity_add_page(struct bio *bio, struct page *page, unsigned int len,
 		unsigned int offset);
 int bio_integrity_map_user(struct bio *bio, void __user *ubuf, ssize_t len, u32 seed);
+void bio_integrity_unmap_user(struct bio *bio);
 bool bio_integrity_prep(struct bio *bio);
 void bio_integrity_advance(struct bio *bio, unsigned int bytes_done);
 void bio_integrity_trim(struct bio *bio);
@@ -105,6 +105,10 @@  static inline int bio_integrity_map_user(struct bio *bio, void __user *ubuf,
 	return -EINVAL;
 }
 
+static inline void bio_integrity_unmap_user(struct bio *bio)
+{
+}
+
 static inline bool bio_integrity_prep(struct bio *bio)
 {
 	return true;