Re: [PATCH] fat: hold truncate_lock in fat_write_failed()

[Andrew Morton]

On Wed, 21 Oct 2026 13:21:04 -0300 Matheus Alves de Almeida <matheus.aalmeida@xxxxxxxxxxxx> wrote:

> The current fat_write_failed() implementation calls into
> fat_truncate_blocks() without holding truncate_lock. Make it so it is
> held exclusive while truncating.
> 
> Fixes: 459f6ed3b8c7 ("fat: convert to use the new truncate convention.")
> Signed-off-by: Matheus Alves de Almeida <matheus.aalmeida@xxxxxxxxxxxx>

Thanks.  My AI reviewbot said:

: Please expand the changelog to describe the failed/short extending write
: racing with FIBMAP. The mapper uses i_blocks, so it can walk beyond i_size
: while fat_write_failed() is cutting that part of the chain. That can
: produce a bogus "request beyond EOF" error and make the filesystem
: read-only with the default error policy.
: 
: Also, please change the Fixes attribution to 16fab2015099:
: "fat: permit to return phy block number by fibmap in fallocated region".
: That introduced the beyond-EOF FIBMAP lookup. Before it, the lookup was
: bounded by i_size, so the removed clusters were outside its range.
: 459f6ed3b8c7 introduced the helper, but not this failure mechanism.
: 
: I don't see a need to move the lock above truncate_pagecache(). Ordinary
: reads are bounded by i_size, and the cluster containing the partial EOF
: block is retained. FIBMAP is the case which needs this exclusion.
: 
: This looks like a stable candidate once validated and merged upstream.
: Please consider adding Cc: stable@xxxxxxxxxxxxxxx. I checked the source
: and a reduced TLA+ model, but haven't built or runtime-tested the patch.

So I made some changes and ended up with the below.  Please check.

From: Matheus Alves de Almeida <matheus.aalmeida@xxxxxxxxxxxx>
Subject: fat: hold truncate_lock in fat_write_failed()
Date: Wed, 21 Oct 2026 13:21:04 -0300

The current fat_write_failed() implementation calls into
fat_truncate_blocks() without holding truncate_lock.

A failed or short extending write racing with FIBMAP can spuriously make a
FAT filesystem read-only.  FIBMAP bounds its lookup by i_blocks, so it can
walk allocated clusters beyond i_size while fat_write_failed() is removing
them.  If the chain is shortened during the walk, the mapper can report a
bogus "request beyond EOF" error, triggering the default read-only error
policy.  Hold truncate_lock for write around the block cleanup to
serialize it with _fat_bmap().

For comparison, fat_setattr() correctly holds truncate_lock during
truncation:

	down_write(&MSDOS_I(inode)->truncate_lock);
	truncate_setsize(inode, attr->ia_size);
	fat_truncate_blocks(inode, attr->ia_size);
	up_write(&MSDOS_I(inode)->truncate_lock);

Make it so ->truncate_lock is held exclusive while truncating.

Link: https://lore.kernel.org/20261021162105.62525-1-matheus.aalmeida@xxxxxxxxxxxx
Fixes: 16fab2015099 ("fat: permit to return phy block number by fibmap in fallocated region")
Fixes: 459f6ed3b8c7 ("fat: convert to use the new truncate convention.")
Signed-off-by: Matheus Alves de Almeida <matheus.aalmeida@xxxxxxxxxxxx>
Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Acked-by: OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>
Cc: Pedro Falcato <pfalcato@xxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx>
---

 fs/fat/inode.c |    2 ++
 1 file changed, 2 insertions(+)

--- a/fs/fat/inode.c~fat-hold-truncate_lock-in-fat_write_failed
+++ a/fs/fat/inode.c
@@ -217,7 +217,9 @@ static void fat_write_failed(struct addr
 
 	if (to > inode->i_size) {
 		truncate_pagecache(inode, inode->i_size);
+		down_write(&MSDOS_I(inode)->truncate_lock);
 		fat_truncate_blocks(inode, inode->i_size);
+		up_write(&MSDOS_I(inode)->truncate_lock);
 	}
 }
 
_