linux-kernel - Re: [PATCH v2 2/2] ublk: use rq_for_each

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CADUfDZq-CPJdDdtJMH7imu2rMUffJYdyFZHFyePni_Rf-AdBtw@mail.gmail.com>
Date: Thu, 6 Nov 2025 08:49:05 -0800
From: Caleb Sander Mateos <csander@...estorage.com>
To: Ming Lei <ming.lei@...hat.com>
Cc: Jens Axboe <axboe@...nel.dk>, linux-block@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] ublk: use rq_for_each_bvec() for user copy

On Wed, Nov 5, 2025 at 8:27 PM Ming Lei <ming.lei@...hat.com> wrote:
>
> On Wed, Nov 05, 2025 at 01:28:23PM -0700, Caleb Sander Mateos wrote:
> > ublk_advance_io_iter() and ublk_copy_io_pages() currently open-code the
> > iteration over request's bvecs. Switch to the rq_for_each_bvec() macro
> > provided by blk-mq to avoid reaching into the bio internals and simplify
> > the code. Unlike bio_iter_iovec(), rq_for_each_bvec() can return
> > multi-page bvecs. So switch from copy_{to,from}_iter() to
> > copy_page_{to,from}_iter() to map and copy each page in the bvec.
> >
> > Suggested-by: Ming Lei <ming.lei@...hat.com>
> > Signed-off-by: Caleb Sander Mateos <csander@...estorage.com>
> > ---
> >  drivers/block/ublk_drv.c | 78 ++++++++++++----------------------------
> >  1 file changed, 23 insertions(+), 55 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index 40eee3e15a4c..929d40fe0250 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -911,81 +911,49 @@ static const struct block_device_operations ub_fops = {
> >       .open =         ublk_open,
> >       .free_disk =    ublk_free_disk,
> >       .report_zones = ublk_report_zones,
> >  };
> >
> > -struct ublk_io_iter {
> > -     struct bio *bio;
> > -     struct bvec_iter iter;
> > -};
> > -
> > -/* return how many bytes are copied */
> > -static size_t ublk_copy_io_pages(struct ublk_io_iter *data,
> > -             struct iov_iter *uiter, int dir)
> > +/*
> > + * Copy data between request pages and io_iter, and 'offset'
> > + * is the start point of linear offset of request.
> > + */
> > +static size_t ublk_copy_user_pages(const struct request *req,
> > +             unsigned offset, struct iov_iter *uiter, int dir)
> >  {
> > +     struct req_iterator iter;
> > +     struct bio_vec bv;
> >       size_t done = 0;
> >
> > -     for (;;) {
> > -             struct bio_vec bv = bio_iter_iovec(data->bio, data->iter);
> > -             void *bv_buf = bvec_kmap_local(&bv);
> > +     rq_for_each_bvec(bv, req, iter) {
> >               size_t copied;
> >
> > +             if (offset >= bv.bv_len) {
> > +                     offset -= bv.bv_len;
> > +                     continue;
> > +             }
> > +
> > +             bv.bv_offset += offset;
> > +             bv.bv_len -= offset;
> > +             bv.bv_page += bv.bv_offset / PAGE_SIZE;
> > +             bv.bv_offset %= PAGE_SIZE;
>
> The above two lines are not needed because copy_page_*_iter() handles
> it already.

Yes, I realized that, but was trying to avoid the error in
page_copy_sane(). Though it sounds like updating the starting page
isn't sufficient, as the bvec may still span multiple pages.

>
> >               if (dir == ITER_DEST)
> > -                     copied = copy_to_iter(bv_buf, bv.bv_len, uiter);
> > +                     copied = copy_page_to_iter(
> > +                             bv.bv_page, bv.bv_offset, bv.bv_len, uiter);
>
> WARN in page_copy_sane() is triggered because copy_page_*_iter() requires
> all pages belong to one same compound page.
>
> One quick fix is to replace rq_for_each_bvec() with rq_for_each_segment().

Sure, I can switch to rq_for_each_segment(). But then there's not much
point in using copy_page_{to,from}_iter(), right? It allows skipping
the bvec_kmap_local()/kunmap_local(), but incurs additional overhead
to support copying multiple base pages within a single compound page,
which rq_for_each_segment() will never produce.

Best,
Caleb