On Fri, Jul 01, 2022 at 07:07:45PM +0100, Al Viro wrote:
> On Fri, Jul 01, 2022 at 11:53:44AM -0600, Keith Busch wrote:
> > On Fri, Jul 01, 2022 at 06:40:40PM +0100, Al Viro wrote:
> > > -static void bio_put_pages(struct page **pages, size_t size, size_t off)
> > > -{
> > > - size_t i, nr = DIV_ROUND_UP(size + (off & ~PAGE_MASK), PAGE_SIZE);
> > > -
> > > - for (i = 0; i < nr; i++)
> > > - put_page(pages[i]);
> > > -}
> > > -
> > > static int bio_iov_add_page(struct bio *bio, struct page *page,
> > > unsigned int len, unsigned int offset)
> > > {
> > > @@ -1228,11 +1220,11 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
> > > * the iov data will be picked up in the next bio iteration.
> > > */
> > > size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset);
> > > - if (size > 0)
> > > - size = ALIGN_DOWN(size, bdev_logical_block_size(bio->bi_bdev));
> > > if (unlikely(size <= 0))
> > > return size ? size : -EFAULT;
> > > + nr_pages = DIV_ROUND_UP(offset + size, PAGE_SIZE);
> > >
> > > + size = ALIGN_DOWN(size, bdev_logical_block_size(bio->bi_bdev));
> >
> > This isn't quite right. The result of the ALIGN_DOWN could be 0, so whatever
> > page we got before would be leaked since unused pages are only released on an
> > add_page error. I was about to reply with a patch that fixes this, but here's
> > the one that I'm currently testing:
>
> AFAICS, result is broken; you might end up consuming some data and leaving
> iterator not advanced at all. With no way for the caller to tell which way it
> went.
Looks like the possibility of a non-advancing iterator goes all the way back to
the below commit.
commit 576ed9135489c723fb39b97c4e2c73428d06dd78
Author: Christoph Hellwig <hch@xxxxxx>
Date: Thu Sep 20 08:28:21 2018 +0200
block: use bio_add_page in bio_iov_iter_get_pages
I guess the error condition never occured, and nor should it if the bio's
available vectors is accounted for correctly.
