| Message ID | 20230918110510.66470-5-hare@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: update buffer_head for Large-block I/O | expand |
On Mon, Sep 18, 2023 at 01:04:56PM +0200, Hannes Reinecke wrote: > Use accessor functions block_index_to_sector() and block_sector_to_index() > to translate the page index into the block sector and vice versa. You missed two in grow_dev_page() (which I just happened upon): bh = folio_buffers(folio); if (bh) { if (bh->b_size == size) { end_block = folio_init_buffers(folio, bdev, (sector_t)index << sizebits, size); goto done; } ... spin_lock(&inode->i_mapping->private_lock); link_dev_buffers(folio, bh); end_block = folio_init_buffers(folio, bdev, (sector_t)index << sizebits, size); Can UBSAN be of help here? It should catch shifting by a negative amount. That sizebits is calculated in grow_buffers: sizebits = PAGE_SHIFT - __ffs(size);
On Fri, Oct 20, 2023 at 08:37:26PM +0100, Matthew Wilcox wrote: > On Mon, Sep 18, 2023 at 01:04:56PM +0200, Hannes Reinecke wrote: > > Use accessor functions block_index_to_sector() and block_sector_to_index() > > to translate the page index into the block sector and vice versa. > > You missed two in grow_dev_page() (which I just happened upon): I have fixes here. The key part of the first patch is: static sector_t folio_init_buffers(struct folio *folio, - struct block_device *bdev, sector_t block, int size) + struct block_device *bdev, int size) { struct buffer_head *head = folio_buffers(folio); struct buffer_head *bh = head; bool uptodate = folio_test_uptodate(folio); + sector_t block = folio_pos(folio) / size; sector_t end_block = blkdev_max_block(bdev, size); (and then there's the cruft of removing the arguments from folio_init_buffers) The second patch is: static bool grow_buffers(struct block_device *bdev, sector_t block, unsigned size, gfp_t gfp) { - pgoff_t index; - int sizebits; - - sizebits = PAGE_SHIFT - __ffs(size); - index = block >> sizebits; + loff_t pos; [...] - if (unlikely(index != block >> sizebits)) { + if (check_mul_overflow(block, size, &pos) || pos > MAX_LFS_FILESIZE) { I'll send a proper patch series tomorrow once the fstests are done running.
On 10/20/23 21:37, Matthew Wilcox wrote: > On Mon, Sep 18, 2023 at 01:04:56PM +0200, Hannes Reinecke wrote: >> Use accessor functions block_index_to_sector() and block_sector_to_index() >> to translate the page index into the block sector and vice versa. > > You missed two in grow_dev_page() (which I just happened upon): > > bh = folio_buffers(folio); > if (bh) { > if (bh->b_size == size) { > end_block = folio_init_buffers(folio, bdev, > (sector_t)index << sizebits, size); > goto done; > } > ... > spin_lock(&inode->i_mapping->private_lock); > link_dev_buffers(folio, bh); > end_block = folio_init_buffers(folio, bdev, > (sector_t)index << sizebits, size); > > Can UBSAN be of help here? It should catch shifting by a negative > amount. That sizebits is calculated in grow_buffers: > > sizebits = PAGE_SHIFT - __ffs(size); > Ah, thanks. I'm currently working with Luis and Pankay to get a combined patchset; I'll make sure to have that included. Cheers, Hannes
diff --git a/fs/buffer.c b/fs/buffer.c index 2379564e5aea..66895432d91f 100644 --- a/fs/buffer.c +++ b/fs/buffer.c @@ -199,7 +199,7 @@ __find_get_block_slow(struct block_device *bdev, sector_t block) int all_mapped = 1; static DEFINE_RATELIMIT_STATE(last_warned, HZ, 1); - index = block >> (PAGE_SHIFT - bd_inode->i_blkbits); + index = block_sector_to_index(block, bd_inode->i_blkbits); folio = __filemap_get_folio(bd_mapping, index, FGP_ACCESSED, 0); if (IS_ERR(folio)) goto out; @@ -1702,13 +1702,13 @@ void clean_bdev_aliases(struct block_device *bdev, sector_t block, sector_t len) struct inode *bd_inode = bdev->bd_inode; struct address_space *bd_mapping = bd_inode->i_mapping; struct folio_batch fbatch; - pgoff_t index = block >> (PAGE_SHIFT - bd_inode->i_blkbits); + pgoff_t index = block_sector_to_index(block, bd_inode->i_blkbits); pgoff_t end; int i, count; struct buffer_head *bh; struct buffer_head *head; - end = (block + len - 1) >> (PAGE_SHIFT - bd_inode->i_blkbits); + end = block_sector_to_index(block + len - 1, bd_inode->i_blkbits); folio_batch_init(&fbatch); while (filemap_get_folios(bd_mapping, &index, end, &fbatch)) { count = folio_batch_count(&fbatch); @@ -1835,7 +1835,7 @@ int __block_write_full_folio(struct inode *inode, struct folio *folio, blocksize = bh->b_size; bbits = block_size_bits(blocksize); - block = (sector_t)folio->index << (PAGE_SHIFT - bbits); + block = block_index_to_sector(folio->index, bbits); last_block = (i_size_read(inode) - 1) >> bbits; /* @@ -2087,7 +2087,7 @@ int __block_write_begin_int(struct folio *folio, loff_t pos, unsigned len, blocksize = head->b_size; bbits = block_size_bits(blocksize); - block = (sector_t)folio->index << (PAGE_SHIFT - bbits); + block = block_index_to_sector(folio->index, bbits); for(bh = head, block_start = 0; bh != head || !block_start; block++, block_start=block_end, bh = bh->b_this_page) { @@ -2369,7 +2369,7 @@ int block_read_full_folio(struct folio *folio, get_block_t *get_block) blocksize = head->b_size; bbits = block_size_bits(blocksize); - iblock = (sector_t)folio->index << (PAGE_SHIFT - bbits); + iblock = block_index_to_sector(folio->index, bbits); lblock = (limit+blocksize-1) >> bbits; bh = head; nr = 0; @@ -2657,7 +2657,7 @@ int block_truncate_page(struct address_space *mapping, return 0; length = blocksize - length; - iblock = (sector_t)index << (PAGE_SHIFT - inode->i_blkbits); + iblock = block_index_to_sector(index, inode->i_blkbits); folio = filemap_grab_folio(mapping, index); if (IS_ERR(folio))
Use accessor functions block_index_to_sector() and block_sector_to_index() to translate the page index into the block sector and vice versa. Signed-off-by: Hannes Reinecke <hare@suse.de> --- fs/buffer.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)