ntfs3: provide block_invalidate_folio to fix memory leak

Commit Message

Mikulas Patocka May 30, 2022, noon UTC

On Mon, 30 May 2022, [e-mail] wrote:

> 
> Hello again,
> 
> When you have time, try moving a large file from a SpadFS partition to
> an NTFS partition mounted with the NTFS3 driver and with a 5.18 kernel,
> and then, move the same file back again, to the SpadFS partition. At
> that very moment is when the size of the file remains permanently in
> the system memory (in my particular case). This does not happen if we
> do it to another Linux file system, nor does it happen if we do it from
> a NTFS partition to another XFS or Ext4 partition.
> 
> So no ccache or anything, I swap files quite often between the SpadFS
> partition and an external hard disk with an NTFS partition. Anyway,
> this problem is really unusual, and it must have some technical
> explanation, because with the ntfs-3g driver this doesn't happen.
> 
> If this information is of any use to you I will be satisfied.
> 
> Regards,
>      
> José Luis Lara Carrascal - Webmaster de Manualinux - GNU/Linux en
> Español (https://manualinux.es)

Hi

SpadFS is innocent here :)

The NTFS3 driver in the kernel 5.18 contains the same bug as SpadFS did - 
missing the invalidate_folio method. This patch adds this method and fixes 
the bug.

Mikulas



Author: Mikulas Patocka <[e-mail]>

The ntfs3 filesystem lacks the 'invalidate_folio' method and it causes
memory leak. If you write to the filesystem and then unmount it, the
cached written data are not freed and they are permanently leaked.

Signed-off-by: Mikulas Patocka <[e-mail]>
Reported-by: José Luis Lara Carrascal <[e-mail]>
Fixes: 7ba13abbd31e ("fs: Turn block_invalidatepage into block_invalidate_folio")
Cc: [e-mail]	# v5.18

---
 fs/ntfs3/inode.c |    1 +
 1 file changed, 1 insertion(+)

Comments

Matthew Wilcox May 30, 2022, 12:18 p.m. UTC | #1

On Mon, May 30, 2022 at 08:00:12AM -0400, Mikulas Patocka wrote:
> The ntfs3 filesystem lacks the 'invalidate_folio' method and it causes
> memory leak. If you write to the filesystem and then unmount it, the
> cached written data are not freed and they are permanently leaked.
> 
> Signed-off-by: Mikulas Patocka <[e-mail]>
> Reported-by: José Luis Lara Carrascal <[e-mail]>
> Fixes: 7ba13abbd31e ("fs: Turn block_invalidatepage into block_invalidate_folio")

That commit is innocent here.  Rather, this should be:

Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")

Yes, trees before 7ba13abbd31e will need to change the patch to add
invalidate_page instead of invalidate_folio, but that's a normal part
of the process.

> Cc: [e-mail]	# v5.18
> 
> ---
>  fs/ntfs3/inode.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> Index: linux-2.6/fs/ntfs3/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/ntfs3/inode.c	2022-05-16 16:57:24.000000000 +0200
> +++ linux-2.6/fs/ntfs3/inode.c	2022-05-30 13:36:45.000000000 +0200
> @@ -1951,6 +1951,7 @@ const struct address_space_operations nt
>  	.direct_IO	= ntfs_direct_IO,
>  	.bmap		= ntfs_bmap,
>  	.dirty_folio	= block_dirty_folio,
> +	.invalidate_folio = block_invalidate_folio,
>  };
>  
>  const struct address_space_operations ntfs_aops_cmpr = {

Mikulas Patocka May 30, 2022, 12:57 p.m. UTC | #2

On Mon, 30 May 2022, Matthew Wilcox wrote:

> On Mon, May 30, 2022 at 08:00:12AM -0400, Mikulas Patocka wrote:
> > The ntfs3 filesystem lacks the 'invalidate_folio' method and it causes
> > memory leak. If you write to the filesystem and then unmount it, the
> > cached written data are not freed and they are permanently leaked.
> > 
> > Signed-off-by: Mikulas Patocka <[e-mail]>
> > Reported-by: José Luis Lara Carrascal <[e-mail]>
> > Fixes: 7ba13abbd31e ("fs: Turn block_invalidatepage into block_invalidate_folio")
> 
> That commit is innocent here.  Rather, this should be:
> 
> Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")

82cae269cfa9 is included in the 5.15 kernel - and this bug only happens in 
5.18. So, how could 82cae269cfa9 cause it?

> Yes, trees before 7ba13abbd31e will need to change the patch to add
> invalidate_page instead of invalidate_folio, but that's a normal part
> of the process.

In the kernel 5.17 and before, if the "invalidatepage" method is NULL, the 
kernel will use block_invalidatepage (see do_invalidatepage). So, we don't 
have to provide explicit "invalidatepage" in 5.17 and before and we don't 
have to backport this bugfix there.

Note that the commit 7ba13abbd31e contains this piece of code:
-#ifdef CONFIG_BLOCK
-	if (!invalidatepage)
-		invalidatepage = block_invalidatepage;
-#endif

So, it explicitly breaks filesystems that have NULL invalidatepage and 
NULL invalidate_folio and that relied on block_invalidatepage being called 
implicitly. So, I believe this commit is the root cause of this bug.

I grepped the kernel for "dirty_folio" and it seems that ntfs3 is the only 
filesystem that provides "dirty_folio" and doesn't provide 
"invalidate_folio".

Mikulas

> > Cc: [e-mail]	# v5.18
> > 
> > ---
> >  fs/ntfs3/inode.c |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > Index: linux-2.6/fs/ntfs3/inode.c
> > ===================================================================
> > --- linux-2.6.orig/fs/ntfs3/inode.c	2022-05-16 16:57:24.000000000 +0200
> > +++ linux-2.6/fs/ntfs3/inode.c	2022-05-30 13:36:45.000000000 +0200
> > @@ -1951,6 +1951,7 @@ const struct address_space_operations nt
> >  	.direct_IO	= ntfs_direct_IO,
> >  	.bmap		= ntfs_bmap,
> >  	.dirty_folio	= block_dirty_folio,
> > +	.invalidate_folio = block_invalidate_folio,
> >  };
> >  
> >  const struct address_space_operations ntfs_aops_cmpr = {
>

Matthew Wilcox May 30, 2022, 1:53 p.m. UTC | #3

On Mon, May 30, 2022 at 08:57:15AM -0400, Mikulas Patocka wrote:
> In the kernel 5.17 and before, if the "invalidatepage" method is NULL, the 
> kernel will use block_invalidatepage (see do_invalidatepage). So, we don't 
> have to provide explicit "invalidatepage" in 5.17 and before and we don't 
> have to backport this bugfix there.
> 
> Note that the commit 7ba13abbd31e contains this piece of code:
> -#ifdef CONFIG_BLOCK
> -	if (!invalidatepage)
> -		invalidatepage = block_invalidatepage;
> -#endif
> 
> So, it explicitly breaks filesystems that have NULL invalidatepage and 
> NULL invalidate_folio and that relied on block_invalidatepage being called 
> implicitly. So, I believe this commit is the root cause of this bug.

Oh, right, I missed ntfs3 in that commit.  Oops.

Acked-by: Matthew Wilcox (Oracle) <[e-mail]>

Namjae Jeon May 30, 2022, 2 p.m. UTC | #4

2022-05-30 21:00 GMT+09:00, Mikulas Patocka <[e-mail]>:
>
>
> On Mon, 30 May 2022, [e-mail] wrote:
>
>>
>> Hello again,
>>
>> When you have time, try moving a large file from a SpadFS partition to
>> an NTFS partition mounted with the NTFS3 driver and with a 5.18 kernel,
>> and then, move the same file back again, to the SpadFS partition. At
>> that very moment is when the size of the file remains permanently in
>> the system memory (in my particular case). This does not happen if we
>> do it to another Linux file system, nor does it happen if we do it from
>> a NTFS partition to another XFS or Ext4 partition.
>>
>> So no ccache or anything, I swap files quite often between the SpadFS
>> partition and an external hard disk with an NTFS partition. Anyway,
>> this problem is really unusual, and it must have some technical
>> explanation, because with the ntfs-3g driver this doesn't happen.
>>
>> If this information is of any use to you I will be satisfied.
>>
>> Regards,
>>
>> José Luis Lara Carrascal - Webmaster de Manualinux - GNU/Linux en
>> Español (https://manualinux.es)
>
> Hi
>
> SpadFS is innocent here :)
>
> The NTFS3 driver in the kernel 5.18 contains the same bug as SpadFS did -
> missing the invalidate_folio method. This patch adds this method and fixes
> the bug.
>
> Mikulas
>
>
>
> Author: Mikulas Patocka <[e-mail]>
>
> The ntfs3 filesystem lacks the 'invalidate_folio' method and it causes
> memory leak. If you write to the filesystem and then unmount it, the
> cached written data are not freed and they are permanently leaked.
>
> Signed-off-by: Mikulas Patocka <[e-mail]>
> Reported-by: José Luis Lara Carrascal <[e-mail]>
> Fixes: 7ba13abbd31e ("fs: Turn block_invalidatepage into
> block_invalidate_folio")
> Cc: [e-mail]	# v5.18

Reviewed-by: Namjae Jeon <[e-mail]>
>
> ---
>  fs/ntfs3/inode.c |    1 +
>  1 file changed, 1 insertion(+)
>
> Index: linux-2.6/fs/ntfs3/inode.c

> ===================================================================
> --- linux-2.6.orig/fs/ntfs3/inode.c	2022-05-16 16:57:24.000000000 +0200
> +++ linux-2.6/fs/ntfs3/inode.c	2022-05-30 13:36:45.000000000 +0200
linux-2.6 ? Probably you will submit the patch again ?

> @@ -1951,6 +1951,7 @@ const struct address_space_operations nt
>  	.direct_IO	= ntfs_direct_IO,
>  	.bmap		= ntfs_bmap,
>  	.dirty_folio	= block_dirty_folio,
> +	.invalidate_folio = block_invalidate_folio,
>  };
>
>  const struct address_space_operations ntfs_aops_cmpr = {

Konstantin Komarov June 1, 2022, 10:35 a.m. UTC | #5

On 5/30/22 15:00, Mikulas Patocka wrote:
> 
> 
> On Mon, 30 May 2022, [e-mail] wrote:
> 
>>
>> Hello again,
>>
>> When you have time, try moving a large file from a SpadFS partition to
>> an NTFS partition mounted with the NTFS3 driver and with a 5.18 kernel,
>> and then, move the same file back again, to the SpadFS partition. At
>> that very moment is when the size of the file remains permanently in
>> the system memory (in my particular case). This does not happen if we
>> do it to another Linux file system, nor does it happen if we do it from
>> a NTFS partition to another XFS or Ext4 partition.
>>
>> So no ccache or anything, I swap files quite often between the SpadFS
>> partition and an external hard disk with an NTFS partition. Anyway,
>> this problem is really unusual, and it must have some technical
>> explanation, because with the ntfs-3g driver this doesn't happen.
>>
>> If this information is of any use to you I will be satisfied.
>>
>> Regards,
>>       
>> José Luis Lara Carrascal - Webmaster de Manualinux - GNU/Linux en
>> Español (https://manualinux.es)
> 
> Hi
> 
> SpadFS is innocent here :)
> 
> The NTFS3 driver in the kernel 5.18 contains the same bug as SpadFS did -
> missing the invalidate_folio method. This patch adds this method and fixes
> the bug.
> 
> Mikulas
> 
> 
> 
> Author: Mikulas Patocka <[e-mail]>
> 
> The ntfs3 filesystem lacks the 'invalidate_folio' method and it causes
> memory leak. If you write to the filesystem and then unmount it, the
> cached written data are not freed and they are permanently leaked.
> 
> Signed-off-by: Mikulas Patocka <[e-mail]>
> Reported-by: José Luis Lara Carrascal <[e-mail]>
> Fixes: 7ba13abbd31e ("fs: Turn block_invalidatepage into block_invalidate_folio")
> Cc: [e-mail]	# v5.18
> 
> ---
>   fs/ntfs3/inode.c |    1 +
>   1 file changed, 1 insertion(+)
> 
> Index: linux-2.6/fs/ntfs3/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/ntfs3/inode.c	2022-05-16 16:57:24.000000000 +0200
> +++ linux-2.6/fs/ntfs3/inode.c	2022-05-30 13:36:45.000000000 +0200
> @@ -1951,6 +1951,7 @@ const struct address_space_operations nt
>   	.direct_IO	= ntfs_direct_IO,
>   	.bmap		= ntfs_bmap,
>   	.dirty_folio	= block_dirty_folio,
> +	.invalidate_folio = block_invalidate_folio,
>   };
>   
>   const struct address_space_operations ntfs_aops_cmpr = {


Thanks for patch, applied!

Patch

Index: linux-2.6/fs/ntfs3/inode.c
===================================================================
--- linux-2.6.orig/fs/ntfs3/inode.c	2022-05-16 16:57:24.000000000 +0200
+++ linux-2.6/fs/ntfs3/inode.c	2022-05-30 13:36:45.000000000 +0200
@@ -1951,6 +1951,7 @@  const struct address_space_operations nt
 	.direct_IO	= ntfs_direct_IO,
 	.bmap		= ntfs_bmap,
 	.dirty_folio	= block_dirty_folio,
+	.invalidate_folio = block_invalidate_folio,
 };
 
 const struct address_space_operations ntfs_aops_cmpr = {