[PATCH] block: flip iter directions in blk_rq_integrity_map_user()

[PATCH] block: flip iter directions in blk_rq_integrity_map_user()

[Caleb Sander Mateos]

blk_rq_integrity_map_user() creates the ubuf iter with ITER_DEST for
write-direction operations and ITER_SOURCE for read-direction ones.
This is backwards; writes use the user buffer as a source for metadata
and reads use it as a destination. Switch to the rq_data_dir() helper,
which maps writes to ITER_SOURCE (WRITE) and reads to ITER_DEST(READ).

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
Fixes: fe8f4ca7107e ("block: modify bio_integrity_map_user to accept iov_iter as argument")
---
 block/blk-integrity.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index a1678f0a9f81..e4e2567061f9 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -115,17 +115,12 @@ EXPORT_SYMBOL(blk_rq_map_integrity_sg);
 int blk_rq_integrity_map_user(struct request *rq, void __user *ubuf,
 			      ssize_t bytes)
 {
 	int ret;
 	struct iov_iter iter;
-	unsigned int direction;
 
-	if (op_is_write(req_op(rq)))
-		direction = ITER_DEST;
-	else
-		direction = ITER_SOURCE;
-	iov_iter_ubuf(&iter, direction, ubuf, bytes);
+	iov_iter_ubuf(&iter, rq_data_dir(rq), ubuf, bytes);
 	ret = bio_integrity_map_user(rq->bio, &iter);
 	if (ret)
 		return ret;
 
 	rq->nr_integrity_segments = blk_rq_count_integrity_sg(rq->q, rq->bio);
-- 
2.45.2

Re: [PATCH] block: flip iter directions in blk_rq_integrity_map_user()

[Jens Axboe]

On 6/3/25 12:47 PM, Caleb Sander Mateos wrote:
> blk_rq_integrity_map_user() creates the ubuf iter with ITER_DEST for
> write-direction operations and ITER_SOURCE for read-direction ones.
> This is backwards; writes use the user buffer as a source for metadata
> and reads use it as a destination. Switch to the rq_data_dir() helper,
> which maps writes to ITER_SOURCE (WRITE) and reads to ITER_DEST(READ).

Was going to ask "how did this ever work without splats", but looks like
a fairly recent change AND it's for integrity which isn't widely used.
But it does show a gap in testing for sure.

-- 
Jens Axboe

Re: [PATCH] block: flip iter directions in blk_rq_integrity_map_user()

[Jens Axboe]

On Tue, 03 Jun 2025 12:47:51 -0600, Caleb Sander Mateos wrote:
> blk_rq_integrity_map_user() creates the ubuf iter with ITER_DEST for
> write-direction operations and ITER_SOURCE for read-direction ones.
> This is backwards; writes use the user buffer as a source for metadata
> and reads use it as a destination. Switch to the rq_data_dir() helper,
> which maps writes to ITER_SOURCE (WRITE) and reads to ITER_DEST(READ).
> 
> 
> [...]

Applied, thanks!

[1/1] block: flip iter directions in blk_rq_integrity_map_user()
      commit: 43a67dd812c5d3de163c0b6971046b4a4b633d3f

Best regards,
-- 
Jens Axboe

Re: [PATCH] block: flip iter directions in blk_rq_integrity_map_user()

[Anuj gupta]

On Wed, Jun 4, 2025 at 12:24 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 6/3/25 12:47 PM, Caleb Sander Mateos wrote:
> > blk_rq_integrity_map_user() creates the ubuf iter with ITER_DEST for
> > write-direction operations and ITER_SOURCE for read-direction ones.
> > This is backwards; writes use the user buffer as a source for metadata
> > and reads use it as a destination. Switch to the rq_data_dir() helper,
> > which maps writes to ITER_SOURCE (WRITE) and reads to ITER_DEST(READ).
>
> Was going to ask "how did this ever work without splats", but looks like
> a fairly recent change AND it's for integrity which isn't widely used.
> But it does show a gap in testing for sure.
>

Yes, you're absolutely right. blk_rq_integrity_map_user() is currently
only used by nvme-passthru, and Keith recently added a test for that
path [1].

As for the user block integrity interface in general — it’s been a bit
tricky to write generic tests so far, mostly because there's no way to
query the device's integrity capabilities from userspace. But that
should become much easier once we have support for that via an ioctl[2].

[1] https://lore.kernel.org/io-uring/20250416162802.3614051-1-kbusch@meta.com/
[2] https://lore.kernel.org/all/20250527104237.2928-1-anuj20.g@samsung.com/

Re: [PATCH] block: flip iter directions in blk_rq_integrity_map_user()

[Anuj gupta]

Reviewed-by: Anuj Gupta <anuj20.g@samsung.com>

Re: [PATCH] block: flip iter directions in blk_rq_integrity_map_user()

[Jens Axboe]

On 6/3/25 5:34 PM, Anuj gupta wrote:
> On Wed, Jun 4, 2025 at 12:24?AM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 6/3/25 12:47 PM, Caleb Sander Mateos wrote:
>>> blk_rq_integrity_map_user() creates the ubuf iter with ITER_DEST for
>>> write-direction operations and ITER_SOURCE for read-direction ones.
>>> This is backwards; writes use the user buffer as a source for metadata
>>> and reads use it as a destination. Switch to the rq_data_dir() helper,
>>> which maps writes to ITER_SOURCE (WRITE) and reads to ITER_DEST(READ).
>>
>> Was going to ask "how did this ever work without splats", but looks like
>> a fairly recent change AND it's for integrity which isn't widely used.
>> But it does show a gap in testing for sure.
>>
> 
> Yes, you're absolutely right. blk_rq_integrity_map_user() is currently
> only used by nvme-passthru, and Keith recently added a test for that
> path [1].
> 
> As for the user block integrity interface in general ? it?s been a bit
> tricky to write generic tests so far, mostly because there's no way to
> query the device's integrity capabilities from userspace. But that
> should become much easier once we have support for that via an ioctl[2].
> 
> [1] https://lore.kernel.org/io-uring/20250416162802.3614051-1-kbusch@meta.com/
> [2] https://lore.kernel.org/all/20250527104237.2928-1-anuj20.g@samsung.com/

That makes sense, thanks for clarifying.

-- 
Jens Axboe

Re: [PATCH] block: flip iter directions in blk_rq_integrity_map_user()

[Keith Busch]

On Tue, Jun 03, 2025 at 12:54:05PM -0600, Jens Axboe wrote:
> On 6/3/25 12:47 PM, Caleb Sander Mateos wrote:
> > blk_rq_integrity_map_user() creates the ubuf iter with ITER_DEST for
> > write-direction operations and ITER_SOURCE for read-direction ones.
> > This is backwards; writes use the user buffer as a source for metadata
> > and reads use it as a destination. Switch to the rq_data_dir() helper,
> > which maps writes to ITER_SOURCE (WRITE) and reads to ITER_DEST(READ).
> 
> Was going to ask "how did this ever work without splats", but looks like
> a fairly recent change AND it's for integrity which isn't widely used.
> But it does show a gap in testing for sure.

The change is good and correct, but it doesn't look like normal tests
would find a problem here. The iter direction in this path only adds the
FOLL_WRITE flag, which appears to just check for writable access. Unless
you're specifically testing something using read-only PTE's, a test
wouldn't have triggered an early error. ?

Re: [PATCH] block: flip iter directions in blk_rq_integrity_map_user()

[Jens Axboe]

On 6/3/25 5:49 PM, Keith Busch wrote:
> On Tue, Jun 03, 2025 at 12:54:05PM -0600, Jens Axboe wrote:
>> On 6/3/25 12:47 PM, Caleb Sander Mateos wrote:
>>> blk_rq_integrity_map_user() creates the ubuf iter with ITER_DEST for
>>> write-direction operations and ITER_SOURCE for read-direction ones.
>>> This is backwards; writes use the user buffer as a source for metadata
>>> and reads use it as a destination. Switch to the rq_data_dir() helper,
>>> which maps writes to ITER_SOURCE (WRITE) and reads to ITER_DEST(READ).
>>
>> Was going to ask "how did this ever work without splats", but looks like
>> a fairly recent change AND it's for integrity which isn't widely used.
>> But it does show a gap in testing for sure.
> 
> The change is good and correct, but it doesn't look like normal tests
> would find a problem here. The iter direction in this path only adds the
> FOLL_WRITE flag, which appears to just check for writable access. Unless
> you're specifically testing something using read-only PTE's, a test
> wouldn't have triggered an early error. ?

Ah I missed that - yeah no way we would've spotted this one other than
under really funky configurations/setups.

-- 
Jens Axboe