[PATCH v2] block: unmap and free user mapped integrity via submitter

[PATCH v2] block: unmap and free user mapped integrity via submitter

[Anuj Gupta]

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.

Signed-off-by: Anuj Gupta <anuj20.g@samsung.com>
Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
---
Changes in v2:
- create a helper for unmap logic (Keith)
- return if integrity is not user-mapped (Jens)
- v1: https://lore.kernel.org/linux-block/20240510094429.2489-1-anuj20.g@samsung.com/
---
 block/bio-integrity.c     | 26 ++++++++++++++++++++++++--
 drivers/nvme/host/ioctl.c | 15 +++++++++++----
 include/linux/bio.h       |  4 ++++
 3 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 2e3e8e04961e..8b528e12136f 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -144,16 +144,38 @@ 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_INTEGRITY_USER)
+		return;
 	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;
 }
 
+/**
+ * bio_integrity_unmap_free_user - Unmap and free bio user integrity payload
+ * @bio:	bio containing bip to be unmapped and freed
+ *
+ * Description: Used to unmap and free the user mapped integrity portion of a
+ * bio. Submitter attaching the user integrity buffer is responsible for
+ * unmapping and freeing it during completion.
+ */
+void bio_integrity_unmap_free_user(struct bio *bio)
+{
+	struct bio_integrity_payload *bip = bio_integrity(bio);
+	struct bio_set *bs = bio->bi_pool;
+
+	if (WARN_ON_ONCE(!(bip->bip_flags & BIP_INTEGRITY_USER)))
+		return;
+	bio_integrity_unmap_user(bip);
+	__bio_integrity_free(bs, bip);
+	bio->bi_integrity = NULL;
+	bio->bi_opf &= ~REQ_INTEGRITY;
+}
+EXPORT_SYMBOL(bio_integrity_unmap_free_user);
+
 /**
  * bio_integrity_add_page - Attach integrity metadata
  * @bio:	bio to update
diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 499a8bb7cac7..2dff5933cae9 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -111,6 +111,13 @@ static struct request *nvme_alloc_user_request(struct request_queue *q,
 	return req;
 }
 
+static void nvme_unmap_bio(struct bio *bio)
+{
+	if (bio_integrity(bio))
+		bio_integrity_unmap_free_user(bio);
+	blk_rq_unmap_user(bio);
+}
+
 static int nvme_map_user_request(struct request *req, u64 ubuffer,
 		unsigned bufflen, void __user *meta_buffer, unsigned meta_len,
 		u32 meta_seed, struct io_uring_cmd *ioucmd, unsigned int flags)
@@ -157,7 +164,7 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 
 out_unmap:
 	if (bio)
-		blk_rq_unmap_user(bio);
+		nvme_unmap_bio(bio);
 out:
 	blk_mq_free_request(req);
 	return ret;
@@ -195,7 +202,7 @@ static int nvme_submit_user_cmd(struct request_queue *q,
 	if (result)
 		*result = le64_to_cpu(nvme_req(req)->result.u64);
 	if (bio)
-		blk_rq_unmap_user(bio);
+		nvme_unmap_bio(bio);
 	blk_mq_free_request(req);
 
 	if (effects)
@@ -406,7 +413,7 @@ static void nvme_uring_task_cb(struct io_uring_cmd *ioucmd,
 	struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
 
 	if (pdu->bio)
-		blk_rq_unmap_user(pdu->bio);
+		nvme_unmap_bio(pdu->bio);
 	io_uring_cmd_done(ioucmd, pdu->status, pdu->result, issue_flags);
 }
 
@@ -432,7 +439,7 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
 	 */
 	if (blk_rq_is_poll(req)) {
 		if (pdu->bio)
-			blk_rq_unmap_user(pdu->bio);
+			nvme_unmap_bio(pdu->bio);
 		io_uring_cmd_iopoll_done(ioucmd, pdu->result, pdu->status);
 	} else {
 		io_uring_cmd_do_in_task_lazy(ioucmd, nvme_uring_task_cb);
diff --git a/include/linux/bio.h b/include/linux/bio.h
index d5379548d684..818e93612947 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -731,6 +731,7 @@ static inline bool bioset_initialized(struct bio_set *bs)
 		bip_for_each_vec(_bvl, _bio->bi_integrity, _iter)
 
 int bio_integrity_map_user(struct bio *bio, void __user *ubuf, ssize_t len, u32 seed);
+void bio_integrity_unmap_free_user(struct bio *bio);
 extern struct bio_integrity_payload *bio_integrity_alloc(struct bio *, gfp_t, unsigned int);
 extern int bio_integrity_add_page(struct bio *, struct page *, unsigned int, unsigned int);
 extern bool bio_integrity_prep(struct bio *);
@@ -807,6 +808,9 @@ static inline int bio_integrity_map_user(struct bio *bio, void __user *ubuf,
 {
 	return -EINVAL;
 }
+static inline void bio_integrity_unmap_free_user(struct bio *bio)
+{
+}
 
 #endif /* CONFIG_BLK_DEV_INTEGRITY */
 
-- 
2.25.1

Re: [PATCH v2] block: unmap and free user mapped integrity via submitter

[Christoph Hellwig]

> diff --git a/block/bio-integrity.c b/block/bio-integrity.c
> index 2e3e8e04961e..8b528e12136f 100644
> --- a/block/bio-integrity.c
> +++ b/block/bio-integrity.c
> @@ -144,16 +144,38 @@ 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_INTEGRITY_USER)
> +		return;
>  	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;

This looks correct.  I wish we could go one step further (maybe
in a separate patch/series) to also move freeing the bio integrity
data to the callers.  In fact I wonder if there is any point in
doing this early separate free vs just doing it as part of the
final bio put.  With that we could also entirely remove the
BIP_INTEGRITY_USER flag.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2] block: unmap and free user mapped integrity via submitter

[Kanchan Joshi]

On 5/20/2024 9:19 PM, Christoph Hellwig wrote:
>> diff --git a/block/bio-integrity.c b/block/bio-integrity.c
>> index 2e3e8e04961e..8b528e12136f 100644
>> --- a/block/bio-integrity.c
>> +++ b/block/bio-integrity.c
>> @@ -144,16 +144,38 @@ 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_INTEGRITY_USER)
>> +		return;
>>   	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;
> 
> This looks correct.  I wish we could go one step further (maybe
> in a separate patch/series) to also move freeing the bio integrity
> data to the callers.  In fact I wonder if there is any point in
> doing this early separate free vs just doing it as part of the
> final bio put.

Tried to think in this direction in past.
But bio_put will not be called if bio is statically allocated. Currently 
that gets freed implicitly because bio_endio is called and it tries to 
free the integrity.

Re: [PATCH v2] block: unmap and free user mapped integrity via submitter

[Anuj gupta]

On Mon, May 13, 2024 at 3:02 PM Anuj Gupta <anuj20.g@samsung.com> wrote:
>
> 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.
>
> Signed-off-by: Anuj Gupta <anuj20.g@samsung.com>
> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
> ---
> Changes in v2:
> - create a helper for unmap logic (Keith)
> - return if integrity is not user-mapped (Jens)
> - v1: https://lore.kernel.org/linux-block/20240510094429.2489-1-anuj20.g@samsung.com/
> ---
>  block/bio-integrity.c     | 26 ++++++++++++++++++++++++--
>  drivers/nvme/host/ioctl.c | 15 +++++++++++----
>  include/linux/bio.h       |  4 ++++
>  3 files changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/block/bio-integrity.c b/block/bio-integrity.c
> index 2e3e8e04961e..8b528e12136f 100644
> --- a/block/bio-integrity.c
> +++ b/block/bio-integrity.c
> @@ -144,16 +144,38 @@ 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_INTEGRITY_USER)
> +               return;
>         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;
>  }
>
> +/**
> + * bio_integrity_unmap_free_user - Unmap and free bio user integrity payload
> + * @bio:       bio containing bip to be unmapped and freed
> + *
> + * Description: Used to unmap and free the user mapped integrity portion of a
> + * bio. Submitter attaching the user integrity buffer is responsible for
> + * unmapping and freeing it during completion.
> + */
> +void bio_integrity_unmap_free_user(struct bio *bio)
> +{
> +       struct bio_integrity_payload *bip = bio_integrity(bio);
> +       struct bio_set *bs = bio->bi_pool;
> +
> +       if (WARN_ON_ONCE(!(bip->bip_flags & BIP_INTEGRITY_USER)))
> +               return;
> +       bio_integrity_unmap_user(bip);
> +       __bio_integrity_free(bs, bip);
> +       bio->bi_integrity = NULL;
> +       bio->bi_opf &= ~REQ_INTEGRITY;
> +}
> +EXPORT_SYMBOL(bio_integrity_unmap_free_user);
> +
>  /**
>   * bio_integrity_add_page - Attach integrity metadata
>   * @bio:       bio to update
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 499a8bb7cac7..2dff5933cae9 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -111,6 +111,13 @@ static struct request *nvme_alloc_user_request(struct request_queue *q,
>         return req;
>  }
>
> +static void nvme_unmap_bio(struct bio *bio)
> +{
> +       if (bio_integrity(bio))
> +               bio_integrity_unmap_free_user(bio);
> +       blk_rq_unmap_user(bio);
> +}
> +
>  static int nvme_map_user_request(struct request *req, u64 ubuffer,
>                 unsigned bufflen, void __user *meta_buffer, unsigned meta_len,
>                 u32 meta_seed, struct io_uring_cmd *ioucmd, unsigned int flags)
> @@ -157,7 +164,7 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
>
>  out_unmap:
>         if (bio)
> -               blk_rq_unmap_user(bio);
> +               nvme_unmap_bio(bio);
>  out:
>         blk_mq_free_request(req);
>         return ret;
> @@ -195,7 +202,7 @@ static int nvme_submit_user_cmd(struct request_queue *q,
>         if (result)
>                 *result = le64_to_cpu(nvme_req(req)->result.u64);
>         if (bio)
> -               blk_rq_unmap_user(bio);
> +               nvme_unmap_bio(bio);
>         blk_mq_free_request(req);
>
>         if (effects)
> @@ -406,7 +413,7 @@ static void nvme_uring_task_cb(struct io_uring_cmd *ioucmd,
>         struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
>
>         if (pdu->bio)
> -               blk_rq_unmap_user(pdu->bio);
> +               nvme_unmap_bio(pdu->bio);
>         io_uring_cmd_done(ioucmd, pdu->status, pdu->result, issue_flags);
>  }
>
> @@ -432,7 +439,7 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
>          */
>         if (blk_rq_is_poll(req)) {
>                 if (pdu->bio)
> -                       blk_rq_unmap_user(pdu->bio);
> +                       nvme_unmap_bio(pdu->bio);
>                 io_uring_cmd_iopoll_done(ioucmd, pdu->result, pdu->status);
>         } else {
>                 io_uring_cmd_do_in_task_lazy(ioucmd, nvme_uring_task_cb);
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index d5379548d684..818e93612947 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -731,6 +731,7 @@ static inline bool bioset_initialized(struct bio_set *bs)
>                 bip_for_each_vec(_bvl, _bio->bi_integrity, _iter)
>
>  int bio_integrity_map_user(struct bio *bio, void __user *ubuf, ssize_t len, u32 seed);
> +void bio_integrity_unmap_free_user(struct bio *bio);
>  extern struct bio_integrity_payload *bio_integrity_alloc(struct bio *, gfp_t, unsigned int);
>  extern int bio_integrity_add_page(struct bio *, struct page *, unsigned int, unsigned int);
>  extern bool bio_integrity_prep(struct bio *);
> @@ -807,6 +808,9 @@ static inline int bio_integrity_map_user(struct bio *bio, void __user *ubuf,
>  {
>         return -EINVAL;
>  }
> +static inline void bio_integrity_unmap_free_user(struct bio *bio)
> +{
> +}
>
>  #endif /* CONFIG_BLK_DEV_INTEGRITY */
>
> --
> 2.25.1
>
>
Hi Jens,
This has got the necessary reviews. Can this be picked up?
Or do you see anything missing?