Re: [PATCH] ocfs2: fix buffer head management in ocfs2_read_blocks()

On 5/29/26 5:41 PM, Dmitry Antipov wrote:
> In 'ocfs2_read_blocks()', caller should't assume that buffer head
> returned by 'sb_getblk()' is exclusively owned and so 'put_bh()'
> always drops b_count from 1 to 0. If it is not so, buffer head
> remains on hold and likely to be returned by the next call to
> 'sb_getblk()' unchanged - that is, with BH_Uptodate bit set even
> if it has failed validation previously, thus allowing to insert
> that buffer head into OCFS2 metadata cache and submit it to upper
> layers. To avoid such a scenario, BH_Uptodate should be cleared
> immediately after 'validate()' callback has detected some data
> inconsistency.
> 
> Reported-by: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=caacd220635a9cc3bac9
> Fixes: cf76c78595ca ("ocfs2: don't put and assigning null to bh allocated outside")
> Signed-off-by: Dmitry Antipov <[email protected]>

Looks fine.
Reviewed-by: Joseph Qi <[email protected]>

> ---
> After introducing the series started with 775c17386a6fd ("ocfs2:
> validate dx_root extent list fields during block read"), reproducer
> from https://syzkaller.appspot.com/bug?extid=30b53487d00b4f7f0922
> has started to trigger KASAN use-after-free again, and this patch
> should fix this as well.
> ---
>  fs/ocfs2/buffer_head_io.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c
> index 701d27d908d4..6114299b121e 100644
> --- a/fs/ocfs2/buffer_head_io.c
> +++ b/fs/ocfs2/buffer_head_io.c
> @@ -350,8 +350,6 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr,
>  						wait_on_buffer(bh);
>  					put_bh(bh);
>  					bhs[i] = NULL;
> -				} else if (bh && buffer_uptodate(bh)) {
> -					clear_buffer_uptodate(bh);
>  				}
>  				continue;
>  			}
> @@ -380,8 +378,11 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr,
>  				BUG_ON(buffer_jbd(bh));
>  				clear_buffer_needs_validate(bh);
>  				status = validate(sb, bh);
> -				if (status)
> +				if (status) {
> +					if (buffer_uptodate(bh))
> +						clear_buffer_uptodate(bh);
>  					goto read_failure;
> +				}
>  			}
>  		}
>