diff mbox series

[04/18] fs/buffer.c: use accessor function to translate page index to sectors

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

Commit Message

Hannes Reinecke Sept. 18, 2023, 11:04 a.m. UTC
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(-)

Comments

Matthew Wilcox Oct. 20, 2023, 7:37 p.m. UTC | #1
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);
Matthew Wilcox Oct. 21, 2023, 5:08 a.m. UTC | #2
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.
Hannes Reinecke Oct. 23, 2023, 5:03 a.m. UTC | #3
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 mbox series

Patch

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))