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

[PATCH] 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>
---
 block/bio-integrity.c     | 25 +++++++++++++++++++++++--
 drivers/nvme/host/ioctl.c | 20 ++++++++++++++++----
 include/linux/bio.h       |  4 ++++
 3 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 2e3e8e04961e..82a0fa91bb8f 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -144,16 +144,37 @@ 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;
+
+	WARN_ON(!(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(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..8016dcf23ad1 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -156,8 +156,11 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 	return ret;
 
 out_unmap:
-	if (bio)
+	if (bio) {
+		if (bio_integrity(bio))
+			bio_integrity_unmap_free_user(bio);
 		blk_rq_unmap_user(bio);
+	}
 out:
 	blk_mq_free_request(req);
 	return ret;
@@ -194,8 +197,11 @@ static int nvme_submit_user_cmd(struct request_queue *q,
 	ret = nvme_execute_rq(req, false);
 	if (result)
 		*result = le64_to_cpu(nvme_req(req)->result.u64);
-	if (bio)
+	if (bio) {
+		if (bio_integrity(bio))
+			bio_integrity_unmap_free_user(bio);
 		blk_rq_unmap_user(bio);
+	}
 	blk_mq_free_request(req);
 
 	if (effects)
@@ -405,8 +411,11 @@ 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)
+	if (pdu->bio) {
+		if (bio_integrity(pdu->bio))
+			bio_integrity_unmap_free_user(pdu->bio);
 		blk_rq_unmap_user(pdu->bio);
+	}
 	io_uring_cmd_done(ioucmd, pdu->status, pdu->result, issue_flags);
 }
 
@@ -431,8 +440,11 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
 	 * Otherwise, move the completion to task work.
 	 */
 	if (blk_rq_is_poll(req)) {
-		if (pdu->bio)
+		if (pdu->bio) {
+			if (bio_integrity(pdu->bio))
+				bio_integrity_unmap_free_user(pdu->bio);
 			blk_rq_unmap_user(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] block: unmap and free user mapped integrity via submitter

[Jens Axboe]

On 5/10/24 3:44 AM, Anuj Gupta wrote:
> +/**
> + * 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;
> +
> +	WARN_ON(!(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(bio_integrity_unmap_free_user);

Should this be a

	if (WARN_ON_ONCE(!(bip->bip_flags & BIP_INTEGRITY_USER)))
		return;

?

-- 
Jens Axboe

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

[Anuj gupta]

On Fri, May 10, 2024 at 7:15 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 5/10/24 3:44 AM, Anuj Gupta wrote:
> > +/**
> > + * 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;
> > +
> > +     WARN_ON(!(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(bio_integrity_unmap_free_user);
>
> Should this be a
>
>         if (WARN_ON_ONCE(!(bip->bip_flags & BIP_INTEGRITY_USER)))
>                 return;
>
> ?

Right, I will fix this in the next version.

>
> --
> Jens Axboe
>
--
Anuj Gupta

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

[Keith Busch]

On Fri, May 10, 2024 at 03:14:29PM +0530, Anuj Gupta 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.

Thanks, this looks pretty good.

>  out_unmap:
> -	if (bio)
> +	if (bio) {
> +		if (bio_integrity(bio))
> +			bio_integrity_unmap_free_user(bio);
>  		blk_rq_unmap_user(bio);
> +	}

Since we're adding more cleanup responsibilities on the caller, and this
pattern is repeated 4 times, I think a little helper function is
warranted: 'nvme_unmap_bio(struct bio *bio)', or something like that.

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

[Anuj gupta]

On Sat, May 11, 2024 at 12:31 AM Keith Busch <kbusch@kernel.org> wrote:
>
> On Fri, May 10, 2024 at 03:14:29PM +0530, Anuj Gupta 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.
>
> Thanks, this looks pretty good.
>
> >  out_unmap:
> > -     if (bio)
> > +     if (bio) {
> > +             if (bio_integrity(bio))
> > +                     bio_integrity_unmap_free_user(bio);
> >               blk_rq_unmap_user(bio);
> > +     }
>
> Since we're adding more cleanup responsibilities on the caller, and this
> pattern is repeated 4 times, I think a little helper function is
> warranted: 'nvme_unmap_bio(struct bio *bio)', or something like that.

Makes sense, I will add this in the next version.

Thanks,
Anuj Gupta