exfat: fix iomap mapping vs truncate race
Subsystems (2)
cover exfat: fix iomap mapping vs truncate race
From: Youling Tang <[email protected]>
This series fixes two related problems in the exfat iomap path.
Patch 1 fixes a race between iomap mapping and truncate: i_size was read twice, and a shrink in between could make the mapping length negative, which underflowed the cluster count.
Patch 2 fixes the handling of the EXFAT_EOF_CLUSTER sentinel returned by exfat_map_cluster(). The iomap code used to convert it into a physical address; it now maps the valid prefix of the range and reports only the remaining part as a hole, matching the old buffer_head code.
Patch 2 relies on the non-negative length guaranteed by patch 1 for the hole mapping.
Youling Tang (2):
exfat: fix iomap mapping when writeback races with truncate
exfat: don't map EXFAT_EOF_CLUSTER as a physical cluster
fs/exfat/iomap.c | 38 ++++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-)
Signature · 1 line
2.43.0
PATCH 1/2 exfat: fix iomap mapping when writeback races with truncate
From: Youling Tang <[email protected]>
__exfat_iomap_begin() reads i_size twice: once to detect a range that starts beyond EOF, and once to clamp the mapping to EOF. A concurrent truncate can shrink i_size between the two reads, so the clamp can produce a negative length. The unsigned cluster index subtraction then underflows, making exfat_map_cluster() believe it has to allocate a huge number of clusters and report a hole.
Fix this by reading i_size once for both the EOF check and the block-aligned clamp.
--- fs/exfat/iomap.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
fs/exfat/iomap.c +5 -4
@@ -51,8 +51,10 @@ static int __exfat_iomap_begin(struct inode *inode, loff_t offset, loff_t lengthbool balloc = false;if (!may_alloc) { +loff_t isize =i_size_read (inode); +/* Completely beyond EOF. Treat as hole */ -if (i_size_read (inode) <= offset) { +if (offset >= isize) { iomap->type =IOMAP_HOLE ; iomap->addr =IOMAP_NULL_ADDR ; iomap->offset = offset;
@@ -61,9 +63,8 @@ static int __exfat_iomap_begin(struct inode *inode, loff_t offset, loff_t length }/* Clamp length if the requested range goes beyond i_size */ -if (offset + length >i_size_read (inode)) - length =round_up (i_size_read (inode), -i_blocksize (inode)) - offset; +if (offset + length > isize) + length =round_up (isize,i_blocksize (inode)) - offset; } num_clusters = exfat_bytes_to_cluster_round_up(sbi,
Signature · 1 line
2.43.0
Re: [PATCH 1/2] exfat: fix iomap mapping when writeback races with truncate
Quoted 9 lines · Youling Tang
From: Youling Tang <[email protected]>
__exfat_iomap_begin()readsi_sizetwice: once to detect a range that starts beyond EOF, and once to clamp the mapping to EOF. A concurrent truncate can shrinki_sizebetween the two reads, so the clamp can produce a negative length. The unsigned cluster index subtraction then underflows, makingexfat_map_cluster()believe it has to allocate a huge number of clusters and report a hole.
Quoted 6 lines
Fix this by reading
i_sizeonce for both the EOF check and the block-aligned clamp.Fixes 82a81a7352bc ("exfat: add iomap buffered I/O support")Signed-off-by Youling Tang <[email protected]>
Looks good to me:
Quoted 32 lines
--- fs/exfat/iomap.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)fs/exfat/iomap.c +5
-4@@ -51,8 +51,10 @@ static int __exfat_iomap_begin(struct inode *inode, loff_t offset, loff_t lengthbool balloc = false;if (!may_alloc) { +loff_t isize =i_size_read (inode); +/* Completely beyond EOF. Treat as hole */ -if (i_size_read (inode) <= offset) { +if (offset >= isize) { iomap->type =IOMAP_HOLE ; iomap->addr =IOMAP_NULL_ADDR ; iomap->offset = offset;@@ -61,9 +63,8 @@ static int __exfat_iomap_begin(struct inode *inode, loff_t offset, loff_t length }/* Clamp length if the requested range goes beyond i_size */ -if (offset + length >i_size_read (inode)) - length =round_up (i_size_read (inode), -i_blocksize (inode)) - offset; +if (offset + length > isize) + length =round_up (isize,i_blocksize (inode)) - offset; } num_clusters = exfat_bytes_to_cluster_round_up(sbi,
PATCH 2/2 exfat: don't map EXFAT_EOF_CLUSTER as a physical cluster
In reply to Youling TangFrom: Youling Tang <[email protected]>
exfat_map_cluster() reports a hole by returning success with *clu == EXFAT_EOF_CLUSTER for !create. __exfat_iomap_begin() does not check that sentinel and converts it into a physical address far beyond the device. A writeback that races with the i_size/i_blocks window in exfat_cont_expand() can hit this: it submits a bio past the end of the device, and fsync() or msync() returns -EIO. If the resulting iomap length is zero, iomap_add_to_ioend() can loop on a zero-length bio and hang a concurrent truncate.
When a range starts before the end of the cluster chain but extends past it, exfat_map_cluster() returns the sentinel for the whole range. Map the valid prefix first and only report the part beyond the chain as a hole. Otherwise a writeback that straddles the end of the chain would skip valid dirty data.
Report the remaining range as a hole instead of turning the sentinel into an address, matching the old exfat_get_block() behavior.
--- fs/exfat/iomap.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
fs/exfat/iomap.c +29 -0
@@ -74,6 +74,22 @@ static int __exfat_iomap_begin(struct inode *inode, loff_t offset, loff_t length iomap->bdev = inode->i_sb->s_bdev; iomap->offset = offset; +if (!may_alloc) { +unsigned int ondisk_clusters = +exfat_bytes_to_cluster (sbi,exfat_ondisk_size (inode)); +unsigned int clu_offset = +exfat_bytes_to_cluster (sbi, offset); + +/* +* exfat_map_cluster() returns the EOF sentinel for the whole +* range if it extends past the cluster chain. Map the valid +* prefix first so the hole only covers the part beyond it. +*/ +if (clu_offset < ondisk_clusters && + num_clusters > ondisk_clusters - clu_offset) + num_clusters = ondisk_clusters - clu_offset; + } + err =exfat_map_cluster (inode,exfat_bytes_to_cluster (sbi, offset), &cluster, &num_clusters, may_alloc, &balloc);if (err == -ENOSPC && !(flags &IOMAP_DIRECT )) {
@@ -90,6 +106,19 @@ static int __exfat_iomap_begin(struct inode *inode, loff_t offset, loff_t lengthif (err) goto out; +/* +* exfat_map_cluster() reports a hole by returning success with +* EXFAT_EOF_CLUSTER. Return it as a hole instead of turning the +* sentinel into a physical address. +*/ +if (!may_alloc && cluster ==EXFAT_EOF_CLUSTER ) { + iomap->type =IOMAP_HOLE ; + iomap->addr =IOMAP_NULL_ADDR ; + iomap->offset = offset; + iomap->length = length; + goto out; + } + cluster_offset =exfat_cluster_offset (sbi, offset); cluster_length =exfat_cluster_to_bytes (sbi, num_clusters);
Signature · 1 line
2.43.0
Re: [PATCH 2/2] exfat: don't map EXFAT_EOF_CLUSTER as a physical cluster
Quoted 40 lines · Youling Tang
From: Youling Tang <[email protected]>
exfat_map_cluster()reports a hole by returning success with *clu ==EXFAT_EOF_CLUSTERfor !create.__exfat_iomap_begin()does not check that sentinel and converts it into a physical address far beyond the device. A writeback that races with the i_size/i_blocks window inexfat_cont_expand()can hit this: it submits a bio past the end of the device, andfsync()ormsync()returns-EIO. If the resulting iomap length is zero,iomap_add_to_ioend()can loop on a zero-length bio and hang a concurrent truncate.When a range starts before the end of the cluster chain but extends past it,
exfat_map_cluster()returns the sentinel for the whole range. Map the valid prefix first and only report the part beyond the chain as a hole. Otherwise a writeback that straddles the end of the chain would skip valid dirty data.Report the remaining range as a hole instead of turning the sentinel into an address, matching the old
exfat_get_block()behavior.Fixes 82a81a7352bc ("exfat: add iomap buffered I/O support")Signed-off-by Youling Tang <[email protected]>--- fs/exfat/iomap.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)fs/exfat/iomap.c +5
-0@@ -74,6 +74,22 @@ static int __exfat_iomap_begin(struct inode *inode, loff_t offset, loff_t length iomap->bdev = inode->i_sb->s_bdev; iomap->offset = offset; +if (!may_alloc) { +unsigned int ondisk_clusters = +exfat_bytes_to_cluster (sbi,exfat_ondisk_size (inode)); +unsigned int clu_offset = +exfat_bytes_to_cluster (sbi, offset);
fs/exfat/iomap.c static int __exfat_iomap_begin(struct inode *inode, loff_t offset, loff_t length · line 91
Quoted 10 lines
fs/exfat/iomap.c +10
-0+ +/* +* exfat_map_cluster() returns the EOF sentinel for the whole +* range if it extends past the cluster chain. Map the valid +* prefix first so the hole only covers the part beyond it. +*/ +if (clu_offset < ondisk_clusters && + num_clusters > ondisk_clusters - clu_offset) + num_clusters = ondisk_clusters - clu_offset; + }
It would be better to put this handling in exfat_map_cluster().
Thanks.
Quoted 24 lines
fs/exfat/iomap.c +14
-0+ err =exfat_map_cluster (inode,exfat_bytes_to_cluster (sbi, offset), &cluster, &num_clusters, may_alloc, &balloc);if (err == -ENOSPC && !(flags &IOMAP_DIRECT )) {@@ -90,6 +106,19 @@ static int __exfat_iomap_begin(struct inode *inode, loff_t offset, loff_t lengthif (err) goto out; +/* +* exfat_map_cluster() reports a hole by returning success with +* EXFAT_EOF_CLUSTER. Return it as a hole instead of turning the +* sentinel into a physical address. +*/ +if (!may_alloc && cluster ==EXFAT_EOF_CLUSTER ) { + iomap->type =IOMAP_HOLE ; + iomap->addr =IOMAP_NULL_ADDR ; + iomap->offset = offset; + iomap->length = length; + goto out; + } + cluster_offset =exfat_cluster_offset (sbi, offset); cluster_length =exfat_cluster_to_bytes (sbi, num_clusters);