* [PATCH 1/3] block: ensure iov_iter advances for added pages
@ 2022-07-05 15:45 Keith Busch
2022-07-05 15:45 ` [PATCH 2/3] block: ensure bio_iov_add_page can't fail Keith Busch
2022-07-05 15:45 ` [PATCH 3/3] block: fix leaking page ref on truncated direct io Keith Busch
0 siblings, 2 replies; 5+ messages in thread
From: Keith Busch @ 2022-07-05 15:45 UTC (permalink / raw
To: linux-fsdevel, linux-block; +Cc: Jens Axboe, Al Viro, Keith Busch
From: Keith Busch <kbusch@kernel.org>
There are cases where a bio may not accept additional pages, and the iov
needs to advance to the last data length that was accepted. The zone
append used to handle this correctly, but was inadvertently broken when
the setup was made common with the normal r/w case.
Fixes: 576ed9135489c ("block: use bio_add_page in bio_iov_iter_get_pages")
Fixes: c58c0074c54c2 ("block/bio: remove duplicate append pages code")
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
block/bio.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 933ea3210954..fdd58461b78f 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1211,6 +1211,7 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
ssize_t size, left;
unsigned len, i;
size_t offset;
+ int ret = 0;
/*
* Move page array up in the allocated memory for the bio vecs as far as
@@ -1235,7 +1236,6 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
for (left = size, i = 0; left > 0; left -= len, i++) {
struct page *page = pages[i];
- int ret;
len = min_t(size_t, PAGE_SIZE - offset, left);
if (bio_op(bio) == REQ_OP_ZONE_APPEND)
@@ -1246,13 +1246,13 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
if (ret) {
bio_put_pages(pages + i, left, offset);
- return ret;
+ break;
}
offset = 0;
}
- iov_iter_advance(iter, size);
- return 0;
+ iov_iter_advance(iter, size - left);
+ return ret;
}
/**
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/3] block: ensure bio_iov_add_page can't fail
2022-07-05 15:45 [PATCH 1/3] block: ensure iov_iter advances for added pages Keith Busch
@ 2022-07-05 15:45 ` Keith Busch
2022-07-05 15:45 ` [PATCH 3/3] block: fix leaking page ref on truncated direct io Keith Busch
1 sibling, 0 replies; 5+ messages in thread
From: Keith Busch @ 2022-07-05 15:45 UTC (permalink / raw
To: linux-fsdevel, linux-block; +Cc: Jens Axboe, Al Viro, Keith Busch
From: Keith Busch <kbusch@kernel.org>
Adding the page could fail on the bio_full() condition, which checks for
either exceeding the bio's max segments or total size exceeding
UINT_MAX. We already ensure the max segments can't be exceeded, so just
ensure the total size won't reach the limit. This simplifies error
handling and removes unnecessary repeated bio_full() checks.
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
block/bio.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index fdd58461b78f..01223f8086ed 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1165,8 +1165,6 @@ static int bio_iov_add_page(struct bio *bio, struct page *page,
bool same_page = false;
if (!__bio_try_merge_page(bio, page, len, offset, &same_page)) {
- if (WARN_ON_ONCE(bio_full(bio, len)))
- return -EINVAL;
__bio_add_page(bio, page, len, offset);
return 0;
}
@@ -1228,7 +1226,8 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
* result to ensure the bio's total size is correct. The remainder of
* the iov data will be picked up in the next bio iteration.
*/
- size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset);
+ size = iov_iter_get_pages(iter, pages, UINT_MAX - bio->bi_iter.bi_size,
+ nr_pages, &offset);
if (size > 0)
size = ALIGN_DOWN(size, bdev_logical_block_size(bio->bi_bdev));
if (unlikely(size <= 0))
@@ -1238,16 +1237,16 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
struct page *page = pages[i];
len = min_t(size_t, PAGE_SIZE - offset, left);
- if (bio_op(bio) == REQ_OP_ZONE_APPEND)
+ if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
ret = bio_iov_add_zone_append_page(bio, page, len,
offset);
- else
- ret = bio_iov_add_page(bio, page, len, offset);
+ if (ret) {
+ bio_put_pages(pages + i, left, offset);
+ break;
+ }
+ } else
+ bio_iov_add_page(bio, page, len, offset);
- if (ret) {
- bio_put_pages(pages + i, left, offset);
- break;
- }
offset = 0;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/3] block: fix leaking page ref on truncated direct io
2022-07-05 15:45 [PATCH 1/3] block: ensure iov_iter advances for added pages Keith Busch
2022-07-05 15:45 ` [PATCH 2/3] block: ensure bio_iov_add_page can't fail Keith Busch
@ 2022-07-05 15:45 ` Keith Busch
2022-07-05 15:54 ` Al Viro
1 sibling, 1 reply; 5+ messages in thread
From: Keith Busch @ 2022-07-05 15:45 UTC (permalink / raw
To: linux-fsdevel, linux-block; +Cc: Jens Axboe, Al Viro, Keith Busch
From: Keith Busch <kbusch@kernel.org>
The size being added to a bio from an iov is aligned to a block size
after the pages were gotten. If the new aligned size truncates the last
page, its reference was being leaked. Ensure all pages that were not
added to the bio have their reference released.
Since this essentially requires doing the same that bio_put_pages(), and
there was only one caller for that function, this patch makes the
put_page() loop common for everyone.
Fixes: b1a000d3b8ec5 ("block: relax direct io memory alignment")
Reported-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
block/bio.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 01223f8086ed..082436736d69 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,10 +1220,16 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
*/
size = iov_iter_get_pages(iter, pages, UINT_MAX - bio->bi_iter.bi_size,
nr_pages, &offset);
- if (size > 0)
+ if (size > 0) {
+ nr_pages = DIV_ROUND_UP(offset + size, PAGE_SIZE);
size = ALIGN_DOWN(size, bdev_logical_block_size(bio->bi_bdev));
- if (unlikely(size <= 0))
- return size ? size : -EFAULT;
+ } else
+ nr_pages = 0;
+
+ if (unlikely(size <= 0)) {
+ ret = size ? size : -EFAULT;
+ goto out;
+ }
for (left = size, i = 0; left > 0; left -= len, i++) {
struct page *page = pages[i];
@@ -1240,10 +1238,8 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
ret = bio_iov_add_zone_append_page(bio, page, len,
offset);
- if (ret) {
- bio_put_pages(pages + i, left, offset);
+ if (ret)
break;
- }
} else
bio_iov_add_page(bio, page, len, offset);
@@ -1251,6 +1247,10 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
}
iov_iter_advance(iter, size - left);
+out:
+ while (i < nr_pages)
+ put_page(pages[i++]);
+
return ret;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] block: fix leaking page ref on truncated direct io
2022-07-05 15:45 ` [PATCH 3/3] block: fix leaking page ref on truncated direct io Keith Busch
@ 2022-07-05 15:54 ` Al Viro
2022-07-05 16:14 ` Keith Busch
0 siblings, 1 reply; 5+ messages in thread
From: Al Viro @ 2022-07-05 15:54 UTC (permalink / raw
To: Keith Busch; +Cc: linux-fsdevel, linux-block, Jens Axboe, Keith Busch
On Tue, Jul 05, 2022 at 08:45:06AM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> The size being added to a bio from an iov is aligned to a block size
> after the pages were gotten. If the new aligned size truncates the last
> page, its reference was being leaked. Ensure all pages that were not
> added to the bio have their reference released.
>
> Since this essentially requires doing the same that bio_put_pages(), and
> there was only one caller for that function, this patch makes the
> put_page() loop common for everyone.
>
> Fixes: b1a000d3b8ec5 ("block: relax direct io memory alignment")
> Reported-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
I still very much dislike this. Background: iov_iter_get_pages should
advance the fucking iterator by the amount it has grabbed. It's
really much cleaner that way. So your round-down-then-fuck-off-if-zero
is going to be a clumsy.
Whatever; I can deal with that on top of your patch. Where would you
have it go wrt tree? Could you do a branch based at the last of your
original series, so that both Jens and I could pull from it?
One thing I would really like to avoid is having the entire #for-5.20/block
in ancestors of those commits; that would make for a monumental headache
with iov_iter series ;-/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] block: fix leaking page ref on truncated direct io
2022-07-05 15:54 ` Al Viro
@ 2022-07-05 16:14 ` Keith Busch
0 siblings, 0 replies; 5+ messages in thread
From: Keith Busch @ 2022-07-05 16:14 UTC (permalink / raw
To: Al Viro; +Cc: Keith Busch, linux-fsdevel, linux-block, Jens Axboe
On Tue, Jul 05, 2022 at 04:54:18PM +0100, Al Viro wrote:
> On Tue, Jul 05, 2022 at 08:45:06AM -0700, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> >
> > The size being added to a bio from an iov is aligned to a block size
> > after the pages were gotten. If the new aligned size truncates the last
> > page, its reference was being leaked. Ensure all pages that were not
> > added to the bio have their reference released.
> >
> > Since this essentially requires doing the same that bio_put_pages(), and
> > there was only one caller for that function, this patch makes the
> > put_page() loop common for everyone.
> >
> > Fixes: b1a000d3b8ec5 ("block: relax direct io memory alignment")
> > Reported-by: Al Viro <viro@zeniv.linux.org.uk>
> > Signed-off-by: Keith Busch <kbusch@kernel.org>
>
> I still very much dislike this. Background: iov_iter_get_pages should
> advance the fucking iterator by the amount it has grabbed. It's
> really much cleaner that way. So your round-down-then-fuck-off-if-zero
> is going to be a clumsy.
I currently don't see a better way to do it, but I'll be happy if you come up
with something.
> Whatever; I can deal with that on top of your patch. Where would you
> have it go wrt tree? Could you do a branch based at the last of your
> original series, so that both Jens and I could pull from it?
Branch pushed here:
https://git.kernel.org/pub/scm/linux/kernel/git/kbusch/linux.git/log/?h=alignment-fixes-rebased
> One thing I would really like to avoid is having the entire #for-5.20/block
> in ancestors of those commits; that would make for a monumental headache
> with iov_iter series ;-/
I'm sorry this is clashing with your work. Please let me know if there's
anything else I can do to help.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-05 16:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-05 15:45 [PATCH 1/3] block: ensure iov_iter advances for added pages Keith Busch
2022-07-05 15:45 ` [PATCH 2/3] block: ensure bio_iov_add_page can't fail Keith Busch
2022-07-05 15:45 ` [PATCH 3/3] block: fix leaking page ref on truncated direct io Keith Busch
2022-07-05 15:54 ` Al Viro
2022-07-05 16:14 ` Keith Busch
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).