[PATCH] shmem: Serialize fallocate progress with writeout

[PATCH] shmem: Serialize fallocate progress with writeout

[Zhu Jun]

shmem_writeout() uses inode->i_lock when reading fallocate progress and
updating nr_unswapped, but shmem_fallocate() accesses the shared progress
without that lock. Holding the current folio lock does not exclude
writeout of a different folio, as the KCSAN report demonstrates.

Use inode->i_lock to sample the unswapped count and publish fallocate
progress. This matches the writeout side and keeps the existing folio
to inode lock ordering.

Fixes: 1aac1400319d ("tmpfs: quit when fallocate fills memory")
Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>
---
 mm/shmem.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 990f1b083779..5769cfab1d71 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3969,6 +3969,11 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
 
 	for (index = start; index < end; ) {
 		struct folio *folio;
+		bool unswapped;
+
+		spin_lock(&inode->i_lock);
+		unswapped = shmem_falloc.nr_unswapped > shmem_falloc.nr_falloced;
+		spin_unlock(&inode->i_lock);
 
 		/*
 		 * Check for fatal signal so that we abort early in OOM
@@ -3979,7 +3984,7 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
 		 */
 		if (fatal_signal_pending(current))
 			error = -EINTR;
-		else if (shmem_falloc.nr_unswapped > shmem_falloc.nr_falloced)
+		else if (unswapped)
 			error = -ENOMEM;
 		else
 			error = shmem_get_folio(inode, index, offset + len,
@@ -4007,11 +4012,13 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
 
 		/*
 		 * Inform shmem_writeout() how far we have reached.
-		 * No need for lock or barrier: we have the page lock.
+		 * Other folios can be written out while this folio is locked.
 		 */
+		spin_lock(&inode->i_lock);
 		if (!folio_test_uptodate(folio))
 			shmem_falloc.nr_falloced += index - shmem_falloc.next;
 		shmem_falloc.next = index;
+		spin_unlock(&inode->i_lock);
 
 		/*
 		 * If !uptodate, leave it that way so that freeable folios
-- 
2.51.0

Re: [PATCH] shmem: Serialize fallocate progress with writeout

[Baolin Wang]

On 11/11/26 4:41 PM, Zhu Jun wrote:
> shmem_writeout() uses inode->i_lock when reading fallocate progress and
> updating nr_unswapped, but shmem_fallocate() accesses the shared progress
> without that lock. Holding the current folio lock does not exclude
> writeout of a different folio, as the KCSAN report demonstrates.

Could you show the KCSAN report?

Also, what's the actual impact? It looks like there's no significant 
effect to me. For this kind of concurrency, I think READ_ONCE/WRITE_ONCE 
should be sufficient?

> Use inode->i_lock to sample the unswapped count and publish fallocate
> progress. This matches the writeout side and keeps the existing folio
> to inode lock ordering.
> 
> Fixes: 1aac1400319d ("tmpfs: quit when fallocate fills memory")
> Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>
> ---
>   mm/shmem.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 990f1b083779..5769cfab1d71 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3969,6 +3969,11 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
>   
>   	for (index = start; index < end; ) {
>   		struct folio *folio;
> +		bool unswapped;
> +
> +		spin_lock(&inode->i_lock);
> +		unswapped = shmem_falloc.nr_unswapped > shmem_falloc.nr_falloced;
> +		spin_unlock(&inode->i_lock);
>   
>   		/*
>   		 * Check for fatal signal so that we abort early in OOM
> @@ -3979,7 +3984,7 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
>   		 */
>   		if (fatal_signal_pending(current))
>   			error = -EINTR;
> -		else if (shmem_falloc.nr_unswapped > shmem_falloc.nr_falloced)
> +		else if (unswapped)
>   			error = -ENOMEM;
>   		else
>   			error = shmem_get_folio(inode, index, offset + len,
> @@ -4007,11 +4012,13 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
>   
>   		/*
>   		 * Inform shmem_writeout() how far we have reached.
> -		 * No need for lock or barrier: we have the page lock.
> +		 * Other folios can be written out while this folio is locked.
>   		 */
> +		spin_lock(&inode->i_lock);
>   		if (!folio_test_uptodate(folio))
>   			shmem_falloc.nr_falloced += index - shmem_falloc.next;
>   		shmem_falloc.next = index;
> +		spin_unlock(&inode->i_lock);
>   
>   		/*
>   		 * If !uptodate, leave it that way so that freeable folios

Re: [PATCH] shmem: Serialize fallocate progress with writeout

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] mm: shmem: unsigned underflow of unmap_end in
  shmem_fallocate() [1]
--

commit 9946039967ae29ddb921f16d447ade65e0f1335b
Author: Zhu Jun <zhujun2@cmss.chinamobile.com>

shmem: Serialize fallocate progress with writeout

This commit takes inode->i_lock in shmem_fallocate() when reading
shmem_falloc.nr_unswapped and updating shmem_falloc progress to synchronize
with shmem_writeout().

> diff --git a/mm/shmem.c b/mm/shmem.c
> index 4686875a3adde..219ac18aef915 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3981,6 +3981,11 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
>  
>  	for (index = start; index < end; ) {
>  		struct folio *folio;
> +		bool unswapped;
> +
> +		spin_lock(&inode->i_lock);
> +		unswapped = shmem_falloc.nr_unswapped > shmem_falloc.nr_falloced;
> +		spin_unlock(&inode->i_lock);

[Severity: Medium]
This problem wasn't introduced by this patch, but can unmap_end underflow
when cast to u64 earlier in shmem_fallocate() [1]?

mm/shmem.c:shmem_fallocate() {
	...
	if (mode & FALLOC_FL_PUNCH_HOLE) {
		struct address_space *mapping = file->f_mapping;
		loff_t unmap_start = round_up(offset, PAGE_SIZE);
		loff_t unmap_end = round_down(offset + len, PAGE_SIZE) - 1;
		...
		if ((u64)unmap_end > (u64)unmap_start)
			unmap_mapping_range(mapping, unmap_start,
					    1 + unmap_end - unmap_start, 0);
	...
}

When punching a hole with offset + len < PAGE_SIZE, round_down(offset +
len, PAGE_SIZE) is 0 and unmap_end becomes -1.  Casting unmap_end to u64
turns it into U64_MAX, so (u64)unmap_end > (u64)unmap_start is true and
unmap_mapping_range() is called with a holelen of 0 (or -PAGE_SIZE), which
unmaps all page table entries from unmap_start to EOF.

[1] https://sashiko.dev/#/bug/linux-d9244c37-209f-42ee-9567-f317e50a1be0

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111084155.80971-1-zhujun2@cmss.chinamobile.com?part=1

Re: [PATCH] shmem: Serialize fallocate progress with writeout

[Zhu Jun]

Hi Baolin,

The KCSAN report I referred to is:
https://syzbot.org/bug?extid=aae8bd56cc1c38d50ec4

Full report:
https://syzbot.org/text?tag=CrashReport&x=16888bf5580000

At the reported revision, the conflicting accesses are the store to
shmem_falloc.next in shmem_fallocate() and the load of that field in
shmem_writeout().

I have not demonstrated a user-visible failure beyond the KCSAN warning,
and I have not reproduced this race locally. The VM tests I ran had no
swap, so they do not validate concurrent swap writeout.

I used i_lock to serialize the accesses, but I have not established that
the algorithm requires a consistent snapshot of the progress counters.
The report alone does not justify adding those locks. I will revisit
READ_ONCE()/WRITE_ONCE() for the shared accesses, while retaining the
existing writeout-side locking for i_private and counter updates.

Thanks for the review,
Zhu Jun