| Message ID | 1544739929-21651-3-git-send-email-sandeen@sandeen.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iomap: 1 cleanup, 1 fix, 1 optimization | expand |
On Thu, Dec 13, 2018 at 04:25:28PM -0600, Eric Sandeen wrote: > From: Eric Sandeen <sandeen@redhat.com> > > iomap_is_partially_uptodate() is intended to check wither blocks within > the selected range of a not-uptodate page are uptodate; if the range we > care about is up to date, it's an optimization. > > However, the iomap implementation continues to check all blocks up to > from+count, which is beyond the page, and can even be well beyond the > iop->uptodate bitmap. The issue is that the caller does not limit the range to the page it is checking to see if it is up to date. Hence we have to clamp it. > I think the worst that will happen is that we may eventually find a zero > bit and return "not partially uptodate" when it would have otherwise > returned true, and skip the optimization. Still, it's clearly an invalid > memory access that must be fixed. It depends on how far beyond the page the count extends. :P > So: fix this by limiting the search to within the page as is done in the > non-iomap variant, block_is_partially_uptodate(). > > Zorro noticed thiswhen KASAN went off for 512 byte blocks on a 64k > page system: > > BUG: KASAN: slab-out-of-bounds in iomap_is_partially_uptodate+0x1a0/0x1e0 > Read of size 8 at addr ffff800120c3a318 by task fsstress/22337 > > Reported-by: Zorro Lang <zlang@redhat.com> > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > Signed-off-by: Eric Sandeen <sandeen@sandeen.net> SOB issue. :) But hte code is the same as the patch I wrote yesterday for this and tested overnight, so Reviewed-by: Dave Chinner <dchinner@redhat.com> -Dave.
On 12/13/18 8:57 PM, Dave Chinner wrote: > On Thu, Dec 13, 2018 at 04:25:28PM -0600, Eric Sandeen wrote: >> From: Eric Sandeen <sandeen@redhat.com> >> >> iomap_is_partially_uptodate() is intended to check wither blocks within >> the selected range of a not-uptodate page are uptodate; if the range we >> care about is up to date, it's an optimization. >> >> However, the iomap implementation continues to check all blocks up to >> from+count, which is beyond the page, and can even be well beyond the >> iop->uptodate bitmap. > > The issue is that the caller does not limit the range to the page it > is checking to see if it is up to date. Hence we have to clamp it. yes. (It occurs to me that maybe we should fix/change this in the caller instead so the implementations don't all have to do the same thing?) >> I think the worst that will happen is that we may eventually find a zero >> bit and return "not partially uptodate" when it would have otherwise >> returned true, and skip the optimization. Still, it's clearly an invalid >> memory access that must be fixed. > > It depends on how far beyond the page the count extends. :P > >> So: fix this by limiting the search to within the page as is done in the >> non-iomap variant, block_is_partially_uptodate(). >> >> Zorro noticed thiswhen KASAN went off for 512 byte blocks on a 64k >> page system: >> >> BUG: KASAN: slab-out-of-bounds in iomap_is_partially_uptodate+0x1a0/0x1e0 >> Read of size 8 at addr ffff800120c3a318 by task fsstress/22337 >> >> Reported-by: Zorro Lang <zlang@redhat.com> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> Signed-off-by: Eric Sandeen <sandeen@sandeen.net> > > SOB issue. :) yeah :/ thanks for the review, -Eric > But hte code is the same as the patch I wrote yesterday for this and > tested overnight, so > > Reviewed-by: Dave Chinner <dchinner@redhat.com> > > -Dave. >
diff --git a/fs/iomap.c b/fs/iomap.c index d6bc98a..ce837d9 100644 --- a/fs/iomap.c +++ b/fs/iomap.c @@ -492,16 +492,29 @@ struct iomap_readpage_ctx { } EXPORT_SYMBOL_GPL(iomap_readpages); +/* + * iomap_is_partially_uptodate checks whether blocks within a page are + * uptodate or not. + * + * Returns true if all blocks which correspond to a file portion + * we want to read within the page are uptodate. + */ int iomap_is_partially_uptodate(struct page *page, unsigned long from, unsigned long count) { struct iomap_page *iop = to_iomap_page(page); struct inode *inode = page->mapping->host; - unsigned first = from >> inode->i_blkbits; - unsigned last = (from + count - 1) >> inode->i_blkbits; + unsigned len, first, last; unsigned i; + /* Limit range to one page */ + len = min_t(unsigned, PAGE_SIZE - from, count); + + /* First and last blocks in range within page */ + first = from >> inode->i_blkbits; + last = (from + len - 1) >> inode->i_blkbits; + if (iop) { for (i = first; i <= last; i++) if (!test_bit(i, iop->uptodate))