* [RFC] bio: fix page leak bio_add_hw_page failure
@ 2021-07-19 10:53 Pavel Begunkov
2021-07-19 15:34 ` Ming Lei
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Pavel Begunkov @ 2021-07-19 10:53 UTC (permalink / raw)
To: Jens Axboe, linux-block; +Cc: asml.silence, Keith Busch
__bio_iov_append_get_pages() doesn't put not appended pages on
bio_add_hw_page() failure, so potentially leaking them, fix it. Also, do
the same for __bio_iov_iter_get_pages(), even though it looks like it
can't be triggered by userspace in this case.
Fixes: 0512a75b98f8 ("block: Introduce REQ_OP_ZONE_APPEND")
Cc: stable@vger.kernel.org # 5.8+
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
I haven't tested the fail path, thus RFC. Would be great if someone can
do it or take over the fix.
block/bio.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 1fab762e079b..d95e3456ba0c 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -979,6 +979,14 @@ static int bio_iov_bvec_set_append(struct bio *bio, struct iov_iter *iter)
return 0;
}
+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]);
+}
+
#define PAGE_PTRS_PER_BVEC (sizeof(struct bio_vec) / sizeof(struct page *))
/**
@@ -1023,8 +1031,10 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
if (same_page)
put_page(page);
} else {
- if (WARN_ON_ONCE(bio_full(bio, len)))
- return -EINVAL;
+ if (WARN_ON_ONCE(bio_full(bio, len))) {
+ bio_put_pages(pages + i, left, offset);
+ return -EINVAL;
+ }
__bio_add_page(bio, page, len, offset);
}
offset = 0;
@@ -1069,6 +1079,7 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter)
len = min_t(size_t, PAGE_SIZE - offset, left);
if (bio_add_hw_page(q, bio, page, len, offset,
max_append_sectors, &same_page) != len) {
+ bio_put_pages(pages + i, left, offset);
ret = -EINVAL;
break;
}
--
2.32.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [RFC] bio: fix page leak bio_add_hw_page failure 2021-07-19 10:53 [RFC] bio: fix page leak bio_add_hw_page failure Pavel Begunkov @ 2021-07-19 15:34 ` Ming Lei 2021-07-19 17:06 ` Pavel Begunkov 2021-08-08 12:57 ` Pavel Begunkov 2021-08-24 14:05 ` Jens Axboe 2 siblings, 1 reply; 7+ messages in thread From: Ming Lei @ 2021-07-19 15:34 UTC (permalink / raw) To: Pavel Begunkov; +Cc: Jens Axboe, linux-block, Keith Busch On Mon, Jul 19, 2021 at 11:53:00AM +0100, Pavel Begunkov wrote: > __bio_iov_append_get_pages() doesn't put not appended pages on > bio_add_hw_page() failure, so potentially leaking them, fix it. Also, do > the same for __bio_iov_iter_get_pages(), even though it looks like it > can't be triggered by userspace in this case. > > Fixes: 0512a75b98f8 ("block: Introduce REQ_OP_ZONE_APPEND") > Cc: stable@vger.kernel.org # 5.8+ > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > > I haven't tested the fail path, thus RFC. Would be great if someone can > do it or take over the fix. > > block/bio.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/block/bio.c b/block/bio.c > index 1fab762e079b..d95e3456ba0c 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -979,6 +979,14 @@ static int bio_iov_bvec_set_append(struct bio *bio, struct iov_iter *iter) > return 0; > } > > +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]); > +} > + > #define PAGE_PTRS_PER_BVEC (sizeof(struct bio_vec) / sizeof(struct page *)) > > /** > @@ -1023,8 +1031,10 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) > if (same_page) > put_page(page); > } else { > - if (WARN_ON_ONCE(bio_full(bio, len))) > - return -EINVAL; > + if (WARN_ON_ONCE(bio_full(bio, len))) { > + bio_put_pages(pages + i, left, offset); > + return -EINVAL; > + } It is unlikely to happen: unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt; struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt; struct page **pages = (struct page **)bv; pages += entries_left * (PAGE_PTRS_PER_BVEC - 1); size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset); > __bio_add_page(bio, page, len, offset); > } > offset = 0; > @@ -1069,6 +1079,7 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter) > len = min_t(size_t, PAGE_SIZE - offset, left); > if (bio_add_hw_page(q, bio, page, len, offset, > max_append_sectors, &same_page) != len) { > + bio_put_pages(pages + i, left, offset); Same with above. Thanks, Ming ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] bio: fix page leak bio_add_hw_page failure 2021-07-19 15:34 ` Ming Lei @ 2021-07-19 17:06 ` Pavel Begunkov 2021-07-20 2:11 ` Ming Lei 0 siblings, 1 reply; 7+ messages in thread From: Pavel Begunkov @ 2021-07-19 17:06 UTC (permalink / raw) To: Ming Lei; +Cc: Jens Axboe, linux-block, Keith Busch On 7/19/21 4:34 PM, Ming Lei wrote: > On Mon, Jul 19, 2021 at 11:53:00AM +0100, Pavel Begunkov wrote: >> __bio_iov_append_get_pages() doesn't put not appended pages on >> bio_add_hw_page() failure, so potentially leaking them, fix it. Also, do >> the same for __bio_iov_iter_get_pages(), even though it looks like it >> can't be triggered by userspace in this case. >> >> Fixes: 0512a75b98f8 ("block: Introduce REQ_OP_ZONE_APPEND") >> Cc: stable@vger.kernel.org # 5.8+ >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >> --- >> >> I haven't tested the fail path, thus RFC. Would be great if someone can >> do it or take over the fix. >> >> block/bio.c | 15 +++++++++++++-- >> 1 file changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/block/bio.c b/block/bio.c >> index 1fab762e079b..d95e3456ba0c 100644 >> --- a/block/bio.c >> +++ b/block/bio.c >> @@ -979,6 +979,14 @@ static int bio_iov_bvec_set_append(struct bio *bio, struct iov_iter *iter) >> return 0; >> } >> >> +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]); >> +} >> + >> #define PAGE_PTRS_PER_BVEC (sizeof(struct bio_vec) / sizeof(struct page *)) >> >> /** >> @@ -1023,8 +1031,10 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) >> if (same_page) >> put_page(page); >> } else { >> - if (WARN_ON_ONCE(bio_full(bio, len))) >> - return -EINVAL; >> + if (WARN_ON_ONCE(bio_full(bio, len))) { >> + bio_put_pages(pages + i, left, offset); >> + return -EINVAL; >> + } > > It is unlikely to happen: > > unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt; > struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt; > struct page **pages = (struct page **)bv; > > pages += entries_left * (PAGE_PTRS_PER_BVEC - 1); > size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset); Agree, mentioned in the commit, however ... >> __bio_add_page(bio, page, len, offset); >> } >> offset = 0; >> @@ -1069,6 +1079,7 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter) >> len = min_t(size_t, PAGE_SIZE - offset, left); >> if (bio_add_hw_page(q, bio, page, len, offset, >> max_append_sectors, &same_page) != len) { >> + bio_put_pages(pages + i, left, offset); > > Same with above. ... bio_add_hw_page() is more complex and additionally does checks against queue_max_zone_append_sectors(), queue_max_segments(), and queue_virt_boundary() in of bvec_gap_to_prev(). It may be unlikely, but are you sure that those are just safety checks? It's not so obvious to me, so would be great if you could point out the other place where the verification is done. -- Pavel Begunkov ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] bio: fix page leak bio_add_hw_page failure 2021-07-19 17:06 ` Pavel Begunkov @ 2021-07-20 2:11 ` Ming Lei 2021-07-21 14:44 ` Pavel Begunkov 0 siblings, 1 reply; 7+ messages in thread From: Ming Lei @ 2021-07-20 2:11 UTC (permalink / raw) To: Pavel Begunkov; +Cc: Jens Axboe, linux-block, Keith Busch On Mon, Jul 19, 2021 at 06:06:49PM +0100, Pavel Begunkov wrote: > On 7/19/21 4:34 PM, Ming Lei wrote: > > On Mon, Jul 19, 2021 at 11:53:00AM +0100, Pavel Begunkov wrote: > >> __bio_iov_append_get_pages() doesn't put not appended pages on > >> bio_add_hw_page() failure, so potentially leaking them, fix it. Also, do > >> the same for __bio_iov_iter_get_pages(), even though it looks like it > >> can't be triggered by userspace in this case. > >> > >> Fixes: 0512a75b98f8 ("block: Introduce REQ_OP_ZONE_APPEND") > >> Cc: stable@vger.kernel.org # 5.8+ > >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > >> --- > >> > >> I haven't tested the fail path, thus RFC. Would be great if someone can > >> do it or take over the fix. > >> > >> block/bio.c | 15 +++++++++++++-- > >> 1 file changed, 13 insertions(+), 2 deletions(-) > >> > >> diff --git a/block/bio.c b/block/bio.c > >> index 1fab762e079b..d95e3456ba0c 100644 > >> --- a/block/bio.c > >> +++ b/block/bio.c > >> @@ -979,6 +979,14 @@ static int bio_iov_bvec_set_append(struct bio *bio, struct iov_iter *iter) > >> return 0; > >> } > >> > >> +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]); > >> +} > >> + > >> #define PAGE_PTRS_PER_BVEC (sizeof(struct bio_vec) / sizeof(struct page *)) > >> > >> /** > >> @@ -1023,8 +1031,10 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) > >> if (same_page) > >> put_page(page); > >> } else { > >> - if (WARN_ON_ONCE(bio_full(bio, len))) > >> - return -EINVAL; > >> + if (WARN_ON_ONCE(bio_full(bio, len))) { > >> + bio_put_pages(pages + i, left, offset); > >> + return -EINVAL; > >> + } > > > > It is unlikely to happen: > > > > unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt; > > struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt; > > struct page **pages = (struct page **)bv; > > > > pages += entries_left * (PAGE_PTRS_PER_BVEC - 1); > > size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset); > > Agree, mentioned in the commit, however ... > > >> __bio_add_page(bio, page, len, offset); > >> } > >> offset = 0; > >> @@ -1069,6 +1079,7 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter) > >> len = min_t(size_t, PAGE_SIZE - offset, left); > >> if (bio_add_hw_page(q, bio, page, len, offset, > >> max_append_sectors, &same_page) != len) { > >> + bio_put_pages(pages + i, left, offset); > > > > Same with above. > > ... bio_add_hw_page() is more complex and additionally does checks > against queue_max_zone_append_sectors(), queue_max_segments(), and > queue_virt_boundary() in of bvec_gap_to_prev(). > > It may be unlikely, but are you sure that those are just safety > checks? It's not so obvious to me, so would be great if you could > point out the other place where the verification is done. OK, bio_add_hw_page() is special, and it needs the handling, but __bio_iov_iter_get_pages() needn't that since it is so obvious. Thanks, Ming ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] bio: fix page leak bio_add_hw_page failure 2021-07-20 2:11 ` Ming Lei @ 2021-07-21 14:44 ` Pavel Begunkov 0 siblings, 0 replies; 7+ messages in thread From: Pavel Begunkov @ 2021-07-21 14:44 UTC (permalink / raw) To: Ming Lei; +Cc: Jens Axboe, linux-block, Keith Busch On 7/20/21 3:11 AM, Ming Lei wrote: > On Mon, Jul 19, 2021 at 06:06:49PM +0100, Pavel Begunkov wrote: >> On 7/19/21 4:34 PM, Ming Lei wrote: >>> On Mon, Jul 19, 2021 at 11:53:00AM +0100, Pavel Begunkov wrote: >>>> __bio_iov_append_get_pages() doesn't put not appended pages on >>>> bio_add_hw_page() failure, so potentially leaking them, fix it. Also, do >>>> the same for __bio_iov_iter_get_pages(), even though it looks like it >>>> can't be triggered by userspace in this case. >>>> >>>> Fixes: 0512a75b98f8 ("block: Introduce REQ_OP_ZONE_APPEND") >>>> Cc: stable@vger.kernel.org # 5.8+ >>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>>> --- >>>> >>>> I haven't tested the fail path, thus RFC. Would be great if someone can >>>> do it or take over the fix. >>>> >>>> block/bio.c | 15 +++++++++++++-- >>>> 1 file changed, 13 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/block/bio.c b/block/bio.c >>>> index 1fab762e079b..d95e3456ba0c 100644 >>>> --- a/block/bio.c >>>> +++ b/block/bio.c >>>> @@ -979,6 +979,14 @@ static int bio_iov_bvec_set_append(struct bio *bio, struct iov_iter *iter) >>>> return 0; >>>> } >>>> >>>> +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]); >>>> +} >>>> + >>>> #define PAGE_PTRS_PER_BVEC (sizeof(struct bio_vec) / sizeof(struct page *)) >>>> >>>> /** >>>> @@ -1023,8 +1031,10 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) >>>> if (same_page) >>>> put_page(page); >>>> } else { >>>> - if (WARN_ON_ONCE(bio_full(bio, len))) >>>> - return -EINVAL; >>>> + if (WARN_ON_ONCE(bio_full(bio, len))) { >>>> + bio_put_pages(pages + i, left, offset); >>>> + return -EINVAL; >>>> + } >>> >>> It is unlikely to happen: >>> >>> unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt; >>> struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt; >>> struct page **pages = (struct page **)bv; >>> >>> pages += entries_left * (PAGE_PTRS_PER_BVEC - 1); >>> size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset); >> >> Agree, mentioned in the commit, however ... >> >>>> __bio_add_page(bio, page, len, offset); >>>> } >>>> offset = 0; >>>> @@ -1069,6 +1079,7 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter) >>>> len = min_t(size_t, PAGE_SIZE - offset, left); >>>> if (bio_add_hw_page(q, bio, page, len, offset, >>>> max_append_sectors, &same_page) != len) { >>>> + bio_put_pages(pages + i, left, offset); >>> >>> Same with above. >> >> ... bio_add_hw_page() is more complex and additionally does checks >> against queue_max_zone_append_sectors(), queue_max_segments(), and >> queue_virt_boundary() in of bvec_gap_to_prev(). >> >> It may be unlikely, but are you sure that those are just safety >> checks? It's not so obvious to me, so would be great if you could >> point out the other place where the verification is done. > > OK, bio_add_hw_page() is special, and it needs the handling, but > __bio_iov_iter_get_pages() needn't that since it is so obvious. Right. I don't mind to drop the first chunk, but it doesn't hurt, and I'd guess the bug came from copy-pasting and editing __bio_iov_iter_get_pages(). That's the reason I added it in the first place. -- Pavel Begunkov ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] bio: fix page leak bio_add_hw_page failure 2021-07-19 10:53 [RFC] bio: fix page leak bio_add_hw_page failure Pavel Begunkov 2021-07-19 15:34 ` Ming Lei @ 2021-08-08 12:57 ` Pavel Begunkov 2021-08-24 14:05 ` Jens Axboe 2 siblings, 0 replies; 7+ messages in thread From: Pavel Begunkov @ 2021-08-08 12:57 UTC (permalink / raw) To: Jens Axboe, linux-block; +Cc: Keith Busch On 7/19/21 11:53 AM, Pavel Begunkov wrote: > __bio_iov_append_get_pages() doesn't put not appended pages on > bio_add_hw_page() failure, so potentially leaking them, fix it. Also, do > the same for __bio_iov_iter_get_pages(), even though it looks like it > can't be triggered by userspace in this case. Any comments? > > Fixes: 0512a75b98f8 ("block: Introduce REQ_OP_ZONE_APPEND") > Cc: stable@vger.kernel.org # 5.8+ > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > > I haven't tested the fail path, thus RFC. Would be great if someone can > do it or take over the fix. > > block/bio.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/block/bio.c b/block/bio.c > index 1fab762e079b..d95e3456ba0c 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -979,6 +979,14 @@ static int bio_iov_bvec_set_append(struct bio *bio, struct iov_iter *iter) > return 0; > } > > +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]); > +} > + > #define PAGE_PTRS_PER_BVEC (sizeof(struct bio_vec) / sizeof(struct page *)) > > /** > @@ -1023,8 +1031,10 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) > if (same_page) > put_page(page); > } else { > - if (WARN_ON_ONCE(bio_full(bio, len))) > - return -EINVAL; > + if (WARN_ON_ONCE(bio_full(bio, len))) { > + bio_put_pages(pages + i, left, offset); > + return -EINVAL; > + } > __bio_add_page(bio, page, len, offset); > } > offset = 0; > @@ -1069,6 +1079,7 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter) > len = min_t(size_t, PAGE_SIZE - offset, left); > if (bio_add_hw_page(q, bio, page, len, offset, > max_append_sectors, &same_page) != len) { > + bio_put_pages(pages + i, left, offset); > ret = -EINVAL; > break; > } > -- Pavel Begunkov ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] bio: fix page leak bio_add_hw_page failure 2021-07-19 10:53 [RFC] bio: fix page leak bio_add_hw_page failure Pavel Begunkov 2021-07-19 15:34 ` Ming Lei 2021-08-08 12:57 ` Pavel Begunkov @ 2021-08-24 14:05 ` Jens Axboe 2 siblings, 0 replies; 7+ messages in thread From: Jens Axboe @ 2021-08-24 14:05 UTC (permalink / raw) To: Pavel Begunkov, linux-block; +Cc: Keith Busch On 7/19/21 4:53 AM, Pavel Begunkov wrote: > __bio_iov_append_get_pages() doesn't put not appended pages on > bio_add_hw_page() failure, so potentially leaking them, fix it. Also, do > the same for __bio_iov_iter_get_pages(), even though it looks like it > can't be triggered by userspace in this case. Applied for 5.15, thanks. -- Jens Axboe ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-08-24 14:05 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-19 10:53 [RFC] bio: fix page leak bio_add_hw_page failure Pavel Begunkov 2021-07-19 15:34 ` Ming Lei 2021-07-19 17:06 ` Pavel Begunkov 2021-07-20 2:11 ` Ming Lei 2021-07-21 14:44 ` Pavel Begunkov 2021-08-08 12:57 ` Pavel Begunkov 2021-08-24 14:05 ` Jens Axboe
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 NNTP newsgroup(s).