Re: [PATCH v4 08/19] ublk: move offset check out of __ublk_check_and_get_req()

[Alexander Atanasov]

On 12.01.26 20:29, Caleb Sander Mateos wrote:
On Mon, Jan 12, 2026 at 10:17 AM Alexander Atanasov <a...@zazolabs.com> wrote:

On 8.01.26 11:19, Caleb Sander Mateos wrote:
__ublk_check_and_get_req() checks that the passed in offset is within
the data length of the specified ublk request. However, only user copy
(ublk_check_and_get_req()) supports accessing ublk request data at a
nonzero offset. Zero-copy buffer registration (ublk_register_io_buf())
always passes 0 for the offset, so the check is unnecessary. Move the
check from __ublk_check_and_get_req() to ublk_check_and_get_req().

Signed-off-by: Caleb Sander Mateos <csan...@purestorage.com>
Reviewed-by: Ming Lei <ming....@redhat.com>
---
   drivers/block/ublk_drv.c | 16 +++++++++-------
   1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index e7697dc4a812..8eefb838b563 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -253,11 +253,11 @@ struct ublk_params_header {


[snip]

@@ -2603,13 +2603,10 @@ static inline struct request 
*__ublk_check_and_get_req(struct ublk_device *ub,
               goto fail_put;

       if (!ublk_rq_has_data(req))
               goto fail_put;

-     if (offset > blk_rq_bytes(req))
-             goto fail_put;
-
       return req;
   fail_put:
       ublk_put_req_ref(io, req);
       return NULL;
   }
@@ -2687,14 +2684,19 @@ ublk_user_copy(struct kiocb *iocb, struct iov_iter 
*iter, int dir)

       if (tag >= ub->dev_info.queue_depth)
               return -EINVAL;

       io = &ubq->ios[tag];
-     req = __ublk_check_and_get_req(ub, q_id, tag, io, buf_off);
+     req = __ublk_check_and_get_req(ub, q_id, tag, io);
       if (!req)
               return -EINVAL;

+     if (buf_off > blk_rq_bytes(req)) {
+             ret = -EINVAL;
+             goto out;
+     }
+

Offset is zero based, bytes are count so it should be >= here.

It will work this way but for buf_off == blk_rq_bytes(req) user will get
0 instead of EINVAL.

This is the existing behavior in __ublk_check_and_get_req(). I agree
allowing buf_off == blk_rq_bytes(req) seems odd, but changing it now
could break ublk servers relying on the current behavior.


I saw it came from the existing version but I doubt that any existing 
server rely on this. In general no code expects to get EOF from a block 
device. It is a user error, classic off by one, to give offset equal to 
the end. If the server have sane error handling it should either detect 
it has a bug and fix it, or does not care at all and work as expected.

The usual pattern is variation of:

while (left > 0) {
    ret = read|write(buf+offset, ....);
    if (ret < 0) goto err;
    left -= ret;
    offset += ret;
}

This gets into a nice infinite loop, and I have actually hit this kind 
of bug in other unrelated code inside the kernel - I guess it is present 
in the original code this is based on.

For example there is/was a case in ext4 that initially returned 0 for a 
write in some edge case but that was changed to return a proper -EAGAIN 
later on iirc to avoid such confusion.

So, if it is not required to be like this by some standard,
it might be worth considering to change.


Best,
Caleb


static size_t ublk_copy_user_pages(const struct request *req,
                  unsigned offset, struct iov_iter *uiter, int dir)
{
         size_t done = 0;
...
          rq_for_each_segment(bv, req, iter) {
...
                  if (offset >= bv.bv_len) {
                          offset -= bv.bv_len; // bv_len is same as
blk_rq_bytes(req)
                          continue; // this breaks the loop when ==
                  }
...
         }
         return done; // done is never incremented
}

       if (!ublk_check_ubuf_dir(req, dir)) {
               ret = -EACCES;
               goto out;
       }


--
have fun,
alex


--
have fun,
alex