[PATCH] ublk: remove WARN_ON_ONCE() in ublk_unmap_io()

[PATCH] ublk: remove WARN_ON_ONCE() in ublk_unmap_io()

[Caleb Sander Mateos]

io->res is set from struct ublksrv_io_cmd's result field, which is
controlled by the ublk server process, without any validation. It's thus
possible for userspace to trigger the io->res > rq_bytes warning.
ublk_copy_user_pages() already limits the copy length to the request
data length, so drop the warning.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 drivers/block/ublk_drv.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 4ca6ec738c93..4f30644756df 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1492,12 +1492,10 @@ static unsigned int ublk_unmap_io(bool need_map,
 
 	if (ublk_need_unmap_req(req)) {
 		struct iov_iter iter;
 		const int dir = ITER_SOURCE;
 
-		WARN_ON_ONCE(io->res > rq_bytes);
-
 		import_ubuf(dir, u64_to_user_ptr(io->buf.addr), io->res, &iter);
 		return ublk_copy_user_pages(req, 0, &iter, dir);
 	}
 	return rq_bytes;
 }
-- 
2.54.0

Re: [PATCH] ublk: remove WARN_ON_ONCE() in ublk_unmap_io()

[Ming Lei]

On Mon, Jul 27, 2026 at 11:24 AM Caleb Sander Mateos
<csander@purestorage.com> wrote:
>
> io->res is set from struct ublksrv_io_cmd's result field, which is
> controlled by the ublk server process, without any validation. It's thus
> possible for userspace to trigger the io->res > rq_bytes warning.
> ublk_copy_user_pages() already limits the copy length to the request
> data length, so drop the warning.
>
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> ---
>  drivers/block/ublk_drv.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 4ca6ec738c93..4f30644756df 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -1492,12 +1492,10 @@ static unsigned int ublk_unmap_io(bool need_map,
>
>         if (ublk_need_unmap_req(req)) {
>                 struct iov_iter iter;
>                 const int dir = ITER_SOURCE;
>
> -               WARN_ON_ONCE(io->res > rq_bytes);
> -
>                 import_ubuf(dir, u64_to_user_ptr(io->buf.addr), io->res, &iter);
>                 return ublk_copy_user_pages(req, 0, &iter, dir);
>         }
>         return rq_bytes;

The change may be fine, however access_ok() still can fail from import_ubuf(),
so can we consolidate ublk_unmap_io() a bit?


Thanks,
Ming Lei

Re: [PATCH] ublk: remove WARN_ON_ONCE() in ublk_unmap_io()

[Caleb Sander Mateos]

On Mon, Jul 27, 2026 at 7:47 PM Ming Lei <tom.leiming@gmail.com> wrote:
>
> On Mon, Jul 27, 2026 at 11:24 AM Caleb Sander Mateos
> <csander@purestorage.com> wrote:
> >
> > io->res is set from struct ublksrv_io_cmd's result field, which is
> > controlled by the ublk server process, without any validation. It's thus
> > possible for userspace to trigger the io->res > rq_bytes warning.
> > ublk_copy_user_pages() already limits the copy length to the request
> > data length, so drop the warning.
> >
> > Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> > ---
> >  drivers/block/ublk_drv.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index 4ca6ec738c93..4f30644756df 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -1492,12 +1492,10 @@ static unsigned int ublk_unmap_io(bool need_map,
> >
> >         if (ublk_need_unmap_req(req)) {
> >                 struct iov_iter iter;
> >                 const int dir = ITER_SOURCE;
> >
> > -               WARN_ON_ONCE(io->res > rq_bytes);
> > -
> >                 import_ubuf(dir, u64_to_user_ptr(io->buf.addr), io->res, &iter);
> >                 return ublk_copy_user_pages(req, 0, &iter, dir);
> >         }
> >         return rq_bytes;
>
> The change may be fine, however access_ok() still can fail from import_ubuf(),
> so can we consolidate ublk_unmap_io() a bit?

Sorry, not quite sure what you mean by "consolidate". Are you saying
there should be a check of the import_ubuf() return value in
ublk_map_io() and ublk_unmap_io()?

Thanks,
Caleb

Re: [PATCH] ublk: remove WARN_ON_ONCE() in ublk_unmap_io()

[Ming Lei]

On Tue, Jul 28, 2026 at 11:22:00AM -0700, Caleb Sander Mateos wrote:
> On Mon, Jul 27, 2026 at 7:47 PM Ming Lei <tom.leiming@gmail.com> wrote:
> >
> > On Mon, Jul 27, 2026 at 11:24 AM Caleb Sander Mateos
> > <csander@purestorage.com> wrote:
> > >
> > > io->res is set from struct ublksrv_io_cmd's result field, which is
> > > controlled by the ublk server process, without any validation. It's thus
> > > possible for userspace to trigger the io->res > rq_bytes warning.
> > > ublk_copy_user_pages() already limits the copy length to the request
> > > data length, so drop the warning.
> > >
> > > Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> > > ---
> > >  drivers/block/ublk_drv.c | 2 --
> > >  1 file changed, 2 deletions(-)
> > >
> > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > index 4ca6ec738c93..4f30644756df 100644
> > > --- a/drivers/block/ublk_drv.c
> > > +++ b/drivers/block/ublk_drv.c
> > > @@ -1492,12 +1492,10 @@ static unsigned int ublk_unmap_io(bool need_map,
> > >
> > >         if (ublk_need_unmap_req(req)) {
> > >                 struct iov_iter iter;
> > >                 const int dir = ITER_SOURCE;
> > >
> > > -               WARN_ON_ONCE(io->res > rq_bytes);
> > > -
> > >                 import_ubuf(dir, u64_to_user_ptr(io->buf.addr), io->res, &iter);
> > >                 return ublk_copy_user_pages(req, 0, &iter, dir);
> > >         }
> > >         return rq_bytes;
> >
> > The change may be fine, however access_ok() still can fail from import_ubuf(),
> > so can we consolidate ublk_unmap_io() a bit?
> 
> Sorry, not quite sure what you mean by "consolidate". Are you saying
> there should be a check of the import_ubuf() return value in
> ublk_map_io() and ublk_unmap_io()?

Yeah.

Thanks,
Ming