[PATCH v4] cifs: fix pagecache leak when do writepages

[PATCH v4] cifs: fix pagecache leak when do writepages

[Yang Erkun]

After commit f3dc1bdb6b0b("cifs: Fix writeback data corruption"), the
writepages for cifs will find all folio needed writepage with two phase.
The first folio will be found in cifs_writepages_begin, and the latter
various folios will be found in cifs_extend_writeback.

All those will first get folio, and for normal case, once we set page
writeback and after do really write, we should put the reference, folio
found in cifs_extend_writeback do this with folio_batch_release. But the
folio found in cifs_writepages_begin never get the chance do it. And
every writepages call, we will leak a folio(found this problem while do
xfstests over cifs, the latter show that we will leak about 600M+ every
we run generic/074).

echo 3 > /proc/sys/vm/drop_caches ; cat /proc/meminfo | grep file
Active(file):      34092 kB
Inactive(file):   176192 kB
./check generic/074 (smb v1)
...
generic/074 50s ...  53s
Ran: generic/074
Passed all 1 tests

echo 3 > /proc/sys/vm/drop_caches ; cat /proc/meminfo | grep file
Active(file):      35036 kB
Inactive(file):   854708 kB

Besides, the exist path seem never handle this folio correctly, fix it too
with this patch. All issue does not occur in the mainline because the
writepages path for CIFS was changed to netfs (commit 3ee1a1fc3981,
titled "cifs: Cut over to using netfslib") as part of a major refactor.
After discussing with the CIFS maintainer, we believe that this single
patch is safer for the stable branch [1].

Steve said:
"""
David and I discussed this today and this patch is MUCH safer than
backporting the later (6.10) netfs changes which would be much larger
and riskier to include (and presumably could affect code outside
cifs.ko as well where this patch is narrowly targeted).

I am fine with this patch.from Yang for 6.6 stable
"""

David said:
"""
Backporting the massive amount of changes to netfslib, fscache, cifs,
afs, 9p, ceph and nfs would kind of diminish the notion that this is a
stable kernel;-).
"""

Fixes: f3dc1bdb6b0b ("cifs: Fix writeback data corruption")
Cc: stable@kernel.org # v6.6~v6.9
Link: https://lore.kernel.org/all/20250911030120.1076413-1-yangerkun@huawei.com/ [1]
Signed-off-by: Yang Erkun <yangerkun@huawei.com>
---
 fs/smb/client/file.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

v3->v4:
1. delay folio_put after folio_unlock
2. document the reason why we choose this single patch instead of
backport

diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index 7a2b81fbd9cf..1058066913dd 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -2884,17 +2884,21 @@ static ssize_t cifs_write_back_from_locked_folio(struct address_space *mapping,
 	rc = cifs_get_writable_file(CIFS_I(inode), FIND_WR_ANY, &cfile);
 	if (rc) {
 		cifs_dbg(VFS, "No writable handle in writepages rc=%d\n", rc);
+		folio_unlock(folio);
 		goto err_xid;
 	}
 
 	rc = server->ops->wait_mtu_credits(server, cifs_sb->ctx->wsize,
 					   &wsize, credits);
-	if (rc != 0)
+	if (rc != 0) {
+		folio_unlock(folio);
 		goto err_close;
+	}
 
 	wdata = cifs_writedata_alloc(cifs_writev_complete);
 	if (!wdata) {
 		rc = -ENOMEM;
+		folio_unlock(folio);
 		goto err_uncredit;
 	}
 
@@ -3041,17 +3045,22 @@ static ssize_t cifs_writepages_begin(struct address_space *mapping,
 lock_again:
 	if (wbc->sync_mode != WB_SYNC_NONE) {
 		ret = folio_lock_killable(folio);
-		if (ret < 0)
+		if (ret < 0) {
+			folio_put(folio);
 			return ret;
+		}
 	} else {
-		if (!folio_trylock(folio))
+		if (!folio_trylock(folio)) {
+			folio_put(folio);
 			goto search_again;
+		}
 	}
 
 	if (folio->mapping != mapping ||
 	    !folio_test_dirty(folio)) {
 		start += folio_size(folio);
 		folio_unlock(folio);
+		folio_put(folio);
 		goto search_again;
 	}
 
@@ -3081,6 +3090,7 @@ static ssize_t cifs_writepages_begin(struct address_space *mapping,
 out:
 	if (ret > 0)
 		*_start = start + ret;
+	folio_put(folio);
 	return ret;
 }
 
-- 
2.39.2

Re: [PATCH v4] cifs: fix pagecache leak when do writepages

[Steve French]

Acked-by:  Steve French <stfrench@microsoft.com>

On Thu, Sep 11, 2025 at 9:11 PM Yang Erkun <yangerkun@huawei.com> wrote:
>
> After commit f3dc1bdb6b0b("cifs: Fix writeback data corruption"), the
> writepages for cifs will find all folio needed writepage with two phase.
> The first folio will be found in cifs_writepages_begin, and the latter
> various folios will be found in cifs_extend_writeback.
>
> All those will first get folio, and for normal case, once we set page
> writeback and after do really write, we should put the reference, folio
> found in cifs_extend_writeback do this with folio_batch_release. But the
> folio found in cifs_writepages_begin never get the chance do it. And
> every writepages call, we will leak a folio(found this problem while do
> xfstests over cifs, the latter show that we will leak about 600M+ every
> we run generic/074).
>
> echo 3 > /proc/sys/vm/drop_caches ; cat /proc/meminfo | grep file
> Active(file):      34092 kB
> Inactive(file):   176192 kB
> ./check generic/074 (smb v1)
> ...
> generic/074 50s ...  53s
> Ran: generic/074
> Passed all 1 tests
>
> echo 3 > /proc/sys/vm/drop_caches ; cat /proc/meminfo | grep file
> Active(file):      35036 kB
> Inactive(file):   854708 kB
>
> Besides, the exist path seem never handle this folio correctly, fix it too
> with this patch. All issue does not occur in the mainline because the
> writepages path for CIFS was changed to netfs (commit 3ee1a1fc3981,
> titled "cifs: Cut over to using netfslib") as part of a major refactor.
> After discussing with the CIFS maintainer, we believe that this single
> patch is safer for the stable branch [1].
>
> Steve said:
> """
> David and I discussed this today and this patch is MUCH safer than
> backporting the later (6.10) netfs changes which would be much larger
> and riskier to include (and presumably could affect code outside
> cifs.ko as well where this patch is narrowly targeted).
>
> I am fine with this patch.from Yang for 6.6 stable
> """
>
> David said:
> """
> Backporting the massive amount of changes to netfslib, fscache, cifs,
> afs, 9p, ceph and nfs would kind of diminish the notion that this is a
> stable kernel;-).
> """
>
> Fixes: f3dc1bdb6b0b ("cifs: Fix writeback data corruption")
> Cc: stable@kernel.org # v6.6~v6.9
> Link: https://lore.kernel.org/all/20250911030120.1076413-1-yangerkun@huawei.com/ [1]
> Signed-off-by: Yang Erkun <yangerkun@huawei.com>
> ---
>  fs/smb/client/file.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> v3->v4:
> 1. delay folio_put after folio_unlock
> 2. document the reason why we choose this single patch instead of
> backport
>
> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> index 7a2b81fbd9cf..1058066913dd 100644
> --- a/fs/smb/client/file.c
> +++ b/fs/smb/client/file.c
> @@ -2884,17 +2884,21 @@ static ssize_t cifs_write_back_from_locked_folio(struct address_space *mapping,
>         rc = cifs_get_writable_file(CIFS_I(inode), FIND_WR_ANY, &cfile);
>         if (rc) {
>                 cifs_dbg(VFS, "No writable handle in writepages rc=%d\n", rc);
> +               folio_unlock(folio);
>                 goto err_xid;
>         }
>
>         rc = server->ops->wait_mtu_credits(server, cifs_sb->ctx->wsize,
>                                            &wsize, credits);
> -       if (rc != 0)
> +       if (rc != 0) {
> +               folio_unlock(folio);
>                 goto err_close;
> +       }
>
>         wdata = cifs_writedata_alloc(cifs_writev_complete);
>         if (!wdata) {
>                 rc = -ENOMEM;
> +               folio_unlock(folio);
>                 goto err_uncredit;
>         }
>
> @@ -3041,17 +3045,22 @@ static ssize_t cifs_writepages_begin(struct address_space *mapping,
>  lock_again:
>         if (wbc->sync_mode != WB_SYNC_NONE) {
>                 ret = folio_lock_killable(folio);
> -               if (ret < 0)
> +               if (ret < 0) {
> +                       folio_put(folio);
>                         return ret;
> +               }
>         } else {
> -               if (!folio_trylock(folio))
> +               if (!folio_trylock(folio)) {
> +                       folio_put(folio);
>                         goto search_again;
> +               }
>         }
>
>         if (folio->mapping != mapping ||
>             !folio_test_dirty(folio)) {
>                 start += folio_size(folio);
>                 folio_unlock(folio);
> +               folio_put(folio);
>                 goto search_again;
>         }
>
> @@ -3081,6 +3090,7 @@ static ssize_t cifs_writepages_begin(struct address_space *mapping,
>  out:
>         if (ret > 0)
>                 *_start = start + ret;
> +       folio_put(folio);
>         return ret;
>  }
>
> --
> 2.39.2
>
>


-- 
Thanks,

Steve

Re: [PATCH v4] cifs: fix pagecache leak when do writepages

[David Howells]

Steve French <smfrench@gmail.com> wrote:

> On Thu, Sep 11, 2025 at 9:11 PM Yang Erkun <yangerkun@huawei.com> wrote:
> >
> > After commit f3dc1bdb6b0b("cifs: Fix writeback data corruption"), the
> > writepages for cifs will find all folio needed writepage with two phase.
> > The first folio will be found in cifs_writepages_begin, and the latter
> > various folios will be found in cifs_extend_writeback.
> >
> > All those will first get folio, and for normal case, once we set page
> > writeback and after do really write, we should put the reference, folio
> > found in cifs_extend_writeback do this with folio_batch_release. But the
> > folio found in cifs_writepages_begin never get the chance do it. And
> > every writepages call, we will leak a folio(found this problem while do
> > xfstests over cifs, the latter show that we will leak about 600M+ every
> > we run generic/074).
> >
> > echo 3 > /proc/sys/vm/drop_caches ; cat /proc/meminfo | grep file
> > Active(file):      34092 kB
> > Inactive(file):   176192 kB
> > ./check generic/074 (smb v1)
> > ...
> > generic/074 50s ...  53s
> > Ran: generic/074
> > Passed all 1 tests
> >
> > echo 3 > /proc/sys/vm/drop_caches ; cat /proc/meminfo | grep file
> > Active(file):      35036 kB
> > Inactive(file):   854708 kB
> >
> > Besides, the exist path seem never handle this folio correctly, fix it too
> > with this patch. All issue does not occur in the mainline because the
> > writepages path for CIFS was changed to netfs (commit 3ee1a1fc3981,
> > titled "cifs: Cut over to using netfslib") as part of a major refactor.
> > After discussing with the CIFS maintainer, we believe that this single
> > patch is safer for the stable branch [1].
> >
> > Steve said:
> > """
> > David and I discussed this today and this patch is MUCH safer than
> > backporting the later (6.10) netfs changes which would be much larger
> > and riskier to include (and presumably could affect code outside
> > cifs.ko as well where this patch is narrowly targeted).
> >
> > I am fine with this patch.from Yang for 6.6 stable
> > """
> >
> > David said:
> > """
> > Backporting the massive amount of changes to netfslib, fscache, cifs,
> > afs, 9p, ceph and nfs would kind of diminish the notion that this is a
> > stable kernel;-).
> > """
> >
> > Fixes: f3dc1bdb6b0b ("cifs: Fix writeback data corruption")
> > Cc: stable@kernel.org # v6.6~v6.9
> > Link: https://lore.kernel.org/all/20250911030120.1076413-1-yangerkun@huawei.com/ [1]
> > Signed-off-by: Yang Erkun <yangerkun@huawei.com>

Reviewed-by: David Howells <dhowells@redhat.com>