On Thu, Jun 30, 2022 at 08:07:24PM -0600, Keith Busch wrote:
> On Thu, Jun 30, 2022 at 11:39:36PM +0100, Al Viro wrote:
> > On Thu, Jun 30, 2022 at 11:11:27PM +0100, Al Viro wrote:
> >
> > > ... and the first half of that thing conflicts with "block: relax direct
> > > io memory alignment" in -next...
> >
> > BTW, looking at that commit - are you sure that bio_put_pages() on failure
> > exit will do the right thing? We have grabbed a bunch of page references;
> > the amount if DIV_ROUND_UP(offset + size, PAGE_SIZE). And that's before
> > your
> > size = ALIGN_DOWN(size, bdev_logical_block_size(bio->bi_bdev));
>
> Thanks for the catch, it does look like a page reference could get leaked here.
>
> > in there. IMO the following would be more obviously correct:
> > size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset);
> > if (unlikely(size <= 0))
> > return size ? size : -EFAULT;
> >
> > nr_pages = DIV_ROUND_UP(size + offset, PAGE_SIZE);
> > size = ALIGN_DOWN(size, bdev_logical_block_size(bio->bi_bdev));
> >
> > for (left = size, i = 0; left > 0; left -= len, i++) {
> > ...
> > if (ret) {
> > while (i < nr_pages)
> > put_page(pages[i++]);
> > return ret;
> > }
> > ...
> >
> > and get rid of bio_put_pages() entirely. Objections?
>
>
> I think that makes sense. I'll give your idea a test run tomorrow.
See vfs.git#block-fixes, along with #work.iov_iter_get_pages-3 in there.
Seems to work here...
If you are OK with #block-fixes (it's one commit on top of
bf8d08532bc1 "iomap: add support for dma aligned direct-io" in
block.git), the easiest way to deal with the conflicts would be
to have that branch pulled into block.git. Jens, would you be
OK with that in terms of tree topology? Provided that patch
itself looks sane to you, of course...
FWOW, the patch in question is
commit 863965bb7e52997851af3a107ec3e4d8c7050cbd
Author: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Date: Fri Jul 1 13:15:36 2022 -0400
__bio_iov_iter_get_pages(): make sure we don't leak page refs on failure
Calculate the number of pages we'd grabbed before trimming size down.
And don't bother with bio_put_pages() - an explicit cleanup loop is
easier to follow...
Fixes: b1a000d3b8ec "block: relax direct io memory alignment"
Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
diff --git a/block/bio.c b/block/bio.c
index 933ea3210954..59be4eca1192 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1151,14 +1151,6 @@ void bio_iov_bvec_set(struct bio *bio, struct iov_iter *iter)
bio_set_flag(bio, BIO_CLONED);
}
-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));
for (left = size, i = 0; left > 0; left -= len, i++) {
struct page *page = pages[i];
int ret;
@@ -1245,7 +1237,8 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
ret = bio_iov_add_page(bio, page, len, offset);
if (ret) {
- bio_put_pages(pages + i, left, offset);
+ while (i < nr_pages)
+ put_page(pages[i++]);
return ret;
}
offset = 0;
