Re: [RFC 3/8] fs/buffer: restart block_read_full_folio() to avoid array overflow

[Matthew Wilcox <willy@xxxxxxxxxxxxx>]

On Wed, Nov 13, 2024 at 01:47:22AM -0800, Luis Chamberlain wrote:
> +++ b/fs/buffer.c
> @@ -2366,7 +2366,7 @@ int block_read_full_folio(struct folio *folio, get_block_t *get_block)
>  {
>  	struct inode *inode = folio->mapping->host;
>  	sector_t iblock, lblock;
> -	struct buffer_head *bh, *head, *arr[MAX_BUF_PER_PAGE];
> +	struct buffer_head *bh, *head, *restart_bh = NULL, *arr[MAX_BUF_PER_PAGE];

MAX_BUF_PER_PAGE is a pain.  There are configs like hexagon which have
256kB pages and so this array ends up being 512 * 8 bytes = 4kB in size
which spews stack growth warnings.  Can we just make this 8?

> @@ -2385,6 +2385,7 @@ int block_read_full_folio(struct folio *folio, get_block_t *get_block)
>  	iblock = div_u64(folio_pos(folio), blocksize);
>  	lblock = div_u64(limit + blocksize - 1, blocksize);
>  	bh = head;
> +restart:
>  	nr = 0;
>  	i = 0;
>  
> @@ -2417,7 +2418,12 @@ int block_read_full_folio(struct folio *folio, get_block_t *get_block)
>  				continue;
>  		}
>  		arr[nr++] = bh;
> -	} while (i++, iblock++, (bh = bh->b_this_page) != head);
> +	} while (i++, iblock++, (bh = bh->b_this_page) != head && nr < MAX_BUF_PER_PAGE);
> +
> +	if (nr == MAX_BUF_PER_PAGE && bh != head)
> +		restart_bh = bh;
> +	else
> +		restart_bh = NULL;
>  
>  	if (fully_mapped)
>  		folio_set_mappedtodisk(folio);
> @@ -2450,6 +2456,15 @@ int block_read_full_folio(struct folio *folio, get_block_t *get_block)
>  		else
>  			submit_bh(REQ_OP_READ, bh);
>  	}
> +
> +	/*
> +	 * Found more buffers than 'arr' could hold,
> +	 * restart to submit the remaining ones.
> +	 */
> +	if (restart_bh) {
> +		bh = restart_bh;
> +		goto restart;
> +	}
>  	return 0;

This isn't right.

Let's assume we need 16 blocks to fill in this folio and we have 8
entries in 'arr'.

        nr = 0;
        i = 0;

        do {
                if (buffer_uptodate(bh))
                        continue;
...
                arr[nr++] = bh;
        } while (i++, iblock++, (bh = bh->b_this_page) != head);

        for (i = 0; i < nr; i++) {
                bh = arr[i];
                        submit_bh(REQ_OP_READ, bh);

OK, so first time round, we've submitted 8 I/Os.  Now we see that
restart_bh is not NULL and so we go round again.

This time, we happen to find that the last 8 BHs are uptodate.
And so we take this path:

        if (!nr) {
                /*
                 * All buffers are uptodate or get_block() returned an
                 * error when trying to map them - we can finish the read.
                 */
                folio_end_read(folio, !page_error);

oops, we forgot about the 8 buffers we already submitted for read.