Potentially broken error path in bio_map_user_iov()

Potentially broken error path in bio_map_user_iov()

[Lukas Bulwahn]

Dear Christoph,

in the function bio_map_user_iov() in ./block/blk-map.c, there is an
error branch for unlikely(offs & queue_dma_alignment(rq->q)), where
the supposingly expected error return value of the function is set,
i.e., ret = -EINVAL;. However, the variable ret is unconditionally
reset by the blk_rq_append_bio(...) call below, so that this 'ret =
-EINVAL;' assignment has no effect.

I am unsure which control flow you expect for this error case through
the bio_map_user_iov() function. Maybe you just want a patch like this
below to return the error code if it is at least once set?

diff --git a/block/blk-map.c b/block/blk-map.c
index 4526adde0156..4a3f6703f46f 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -235,7 +235,7 @@ static int bio_map_user_iov(struct request *rq,
struct iov_iter *iter,
 {
        unsigned int max_sectors = queue_max_hw_sectors(rq->q);
        struct bio *bio;
-       int ret;
+       int ret = 0;
        int j;

        if (!iov_iter_count(iter))
@@ -296,6 +296,9 @@ static int bio_map_user_iov(struct request *rq,
struct iov_iter *iter,
                        break;
        }

+       if (ret)
+               goto out_unmap;
+
        ret = blk_rq_append_bio(rq, bio);
        if (ret)
                goto out_unmap;

I know too little about this function and its intent to create a
proper patch, though.

I also looked at the previous versions, but the error code of this
error branch was really never effectively returned before either (as
far as I grasped the earlier versions of this function). So, this
error path probably never worked as intended.


Best regards,

Lukas

Re: Potentially broken error path in bio_map_user_iov()

[Christoph Hellwig]

Hi Lukas,

we can just remove the queue_dma_alignment check entirely, the caller
already ensures bio_copy_user_iov is called instead of this case.

Re: Potentially broken error path in bio_map_user_iov()

[Lukas Bulwahn]

On Mon, Jan 3, 2022 at 5:35 PM Christoph Hellwig <hch@lst.de> wrote:
>
> Hi Lukas,
>
> we can just remove the queue_dma_alignment check entirely, the caller
> already ensures bio_copy_user_iov is called instead of this case.

Okay, I have sent you a patch to do so:

https://lore.kernel.org/all/20220104120158.20177-1-lukas.bulwahn@gmail.com/

Feel free to pick it, reword it, rework it or ignore it.

Lukas