| Message ID | alpine.LRH.2.21.2304171433370.17217@file01.intranet.prod.int.rdu2.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: fix a crash when bio_for_each_folio_all iterates over an empty bio | expand |
On Mon, Apr 17, 2023 at 03:11:57PM -0400, Mikulas Patocka wrote: > If we use bio_for_each_folio_all on an empty bio, it will access the first > bio vector unconditionally (it is uninitialized) and it may crash > depending on the uninitialized data. Wait, how do we have an empty bio in the first place?
On Mon, 17 Apr 2023, Matthew Wilcox wrote: > On Mon, Apr 17, 2023 at 03:11:57PM -0400, Mikulas Patocka wrote: > > If we use bio_for_each_folio_all on an empty bio, it will access the first > > bio vector unconditionally (it is uninitialized) and it may crash > > depending on the uninitialized data. > > Wait, how do we have an empty bio in the first place? dm-crypt (with the patch that you suggested here: https://www.spinics.net/lists/kernel/msg4691572.html https://lore.kernel.org/linux-mm/alpine.LRH.2.21.2302161619430.5436@file01.intranet.prod.int.rdu2.redhat.com/T/ ) calls: gfp_t gfp_mask = GFP_NOWAIT | __GFP_HIGHMEM; ... pages = mempool_alloc(&cc->page_pool, gfp_mask); if (!pages) { crypt_free_buffer_pages(cc, clone); bio_put(clone); gfp_mask |= __GFP_DIRECT_RECLAIM; order = 0; goto retry; } If the mempool_alloc of the first page fails (it may happen because it uses GFP_NOWAIT), crypt_free_buffer_pages will be called with an empty bio. I also hit this bug when fixing a dm-flakey target - it is triggered by this: https://people.redhat.com/~mpatocka/patches/kernel/dm-flakey/dm-flakey-02-clone-pages.patch I think that this patch doesn't have to be backported to the stable kernels (because there is currently no user that calls bio_for_each_folio_all on an empty bio), but for the next merge window, dm-crypt and dm-flakey is going to use it. Mikulas
On Mon, Apr 17, 2023 at 08:32:54PM +0100, Matthew Wilcox wrote: > On Mon, Apr 17, 2023 at 03:11:57PM -0400, Mikulas Patocka wrote: > > If we use bio_for_each_folio_all on an empty bio, it will access the first > > bio vector unconditionally (it is uninitialized) and it may crash > > depending on the uninitialized data. > > Wait, how do we have an empty bio in the first place? flush bios are always empty. Not sure iterating over them makes much sense, though.
On Mon, Apr 17 2023 at 3:11P -0400, Mikulas Patocka <mpatocka@redhat.com> wrote: > If we use bio_for_each_folio_all on an empty bio, it will access the first > bio vector unconditionally (it is uninitialized) and it may crash > depending on the uninitialized data. > > This patch fixes it by checking the parameter "i" against "bio->bi_vcnt" > and returning NULL fi->folio if it is out of range. > > The patch also drops the test "if (fi->_i + 1 < bio->bi_vcnt)" from > bio_next_folio because the same condition is already being checked in > bio_first_folio. > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> This fix is a prereq for this dm-crypt patch to use folios: https://patchwork.kernel.org/project/dm-devel/patch/alpine.LRH.2.21.2302161619430.5436@file01.intranet.prod.int.rdu2.redhat.com/ Mikulas explained why an empty bio is possible here: https://listman.redhat.com/archives/dm-devel/2023-April/053916.html Reviewed-by: Mike Snitzer <snitzer@kernel.org>
On Tue, Apr 18 2023 at 11:20P -0400, Mike Snitzer <snitzer@kernel.org> wrote: > On Mon, Apr 17 2023 at 3:11P -0400, > Mikulas Patocka <mpatocka@redhat.com> wrote: > > > If we use bio_for_each_folio_all on an empty bio, it will access the first > > bio vector unconditionally (it is uninitialized) and it may crash > > depending on the uninitialized data. > > > > This patch fixes it by checking the parameter "i" against "bio->bi_vcnt" > > and returning NULL fi->folio if it is out of range. > > > > The patch also drops the test "if (fi->_i + 1 < bio->bi_vcnt)" from > > bio_next_folio because the same condition is already being checked in > > bio_first_folio. > > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > This fix is a prereq for this dm-crypt patch to use folios: > https://patchwork.kernel.org/project/dm-devel/patch/alpine.LRH.2.21.2302161619430.5436@file01.intranet.prod.int.rdu2.redhat.com/ > Mikulas explained why an empty bio is possible here: > https://listman.redhat.com/archives/dm-devel/2023-April/053916.html > > Reviewed-by: Mike Snitzer <snitzer@kernel.org> Hey Jens (and Matthew), Can you please pick this up? Without it DM needs to do the checking (by open-coding a fixed variant of bio_for_each_folio_all); while we _could_ do that: fixing bio_first_folio seems best. Thanks, Mike
Index: linux-2.6/include/linux/bio.h =================================================================== --- linux-2.6.orig/include/linux/bio.h +++ linux-2.6/include/linux/bio.h @@ -279,15 +279,19 @@ struct folio_iter { static inline void bio_first_folio(struct folio_iter *fi, struct bio *bio, int i) { - struct bio_vec *bvec = bio_first_bvec_all(bio) + i; + if (i < bio->bi_vcnt) { + struct bio_vec *bvec = bio_first_bvec_all(bio) + i; - fi->folio = page_folio(bvec->bv_page); - fi->offset = bvec->bv_offset + - PAGE_SIZE * (bvec->bv_page - &fi->folio->page); - fi->_seg_count = bvec->bv_len; - fi->length = min(folio_size(fi->folio) - fi->offset, fi->_seg_count); - fi->_next = folio_next(fi->folio); - fi->_i = i; + fi->folio = page_folio(bvec->bv_page); + fi->offset = bvec->bv_offset + + PAGE_SIZE * (bvec->bv_page - &fi->folio->page); + fi->_seg_count = bvec->bv_len; + fi->length = min(folio_size(fi->folio) - fi->offset, fi->_seg_count); + fi->_next = folio_next(fi->folio); + fi->_i = i; + } else { + fi->folio = NULL; + } } static inline void bio_next_folio(struct folio_iter *fi, struct bio *bio) @@ -298,10 +302,8 @@ static inline void bio_next_folio(struct fi->offset = 0; fi->length = min(folio_size(fi->folio), fi->_seg_count); fi->_next = folio_next(fi->folio); - } else if (fi->_i + 1 < bio->bi_vcnt) { - bio_first_folio(fi, bio, fi->_i + 1); } else { - fi->folio = NULL; + bio_first_folio(fi, bio, fi->_i + 1); } }
If we use bio_for_each_folio_all on an empty bio, it will access the first bio vector unconditionally (it is uninitialized) and it may crash depending on the uninitialized data. This patch fixes it by checking the parameter "i" against "bio->bi_vcnt" and returning NULL fi->folio if it is out of range. The patch also drops the test "if (fi->_i + 1 < bio->bi_vcnt)" from bio_next_folio because the same condition is already being checked in bio_first_folio. Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> --- include/linux/bio.h | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)