linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 4.19,v2] VFS: Fix fuseblk memory leak caused by mount concurrency
@ 2021-10-13  9:51 ChenXiaoSong
  2021-10-13 10:11 ` Greg KH
  0 siblings, 1 reply; 7+ messages in thread
From: ChenXiaoSong @ 2021-10-13  9:51 UTC (permalink / raw)
  To: viro, stable, gregkh
  Cc: linux-fsdevel, linux-kernel, dhowells, yukuai3, yi.zhang,
	chenxiaosong2, zhangxiaoxu5

If two processes mount same superblock, memory leak occurs:

CPU0               |  CPU1
do_new_mount       |  do_new_mount
  fs_set_subtype   |    fs_set_subtype
    kstrdup        |
                   |      kstrdup
    memrory leak   |

Fix this by adding a write lock while calling fs_set_subtype.

Linus's tree already have refactoring patchset [1], one of them can fix this bug:
        c30da2e981a7 (fuse: convert to use the new mount API)

Since we did not merge the refactoring patchset in this branch, I create this patch.

[1] https://patchwork.kernel.org/project/linux-fsdevel/patch/20190903113640.7984-3-mszeredi@redhat.com/

Fixes: 79c0b2df79eb (add filesystem subtype support)
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
---
v1: Can not mount sshfs ([PATCH linux-4.19.y] VFS: Fix fuseblk memory leak caused by mount concurrency)
v2: Use write lock while writing superblock

 fs/namespace.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 2f3c6a0350a8..396ff1bcfdad 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2490,9 +2490,12 @@ static int do_new_mount(struct path *path, const char *fstype, int sb_flags,
 		return -ENODEV;
 
 	mnt = vfs_kern_mount(type, sb_flags, name, data);
-	if (!IS_ERR(mnt) && (type->fs_flags & FS_HAS_SUBTYPE) &&
-	    !mnt->mnt_sb->s_subtype)
-		mnt = fs_set_subtype(mnt, fstype);
+	if (!IS_ERR(mnt) && (type->fs_flags & FS_HAS_SUBTYPE)) {
+		down_write(&mnt->mnt_sb->s_umount);
+		if (!mnt->mnt_sb->s_subtype)
+			mnt = fs_set_subtype(mnt, fstype);
+		up_write(&mnt->mnt_sb->s_umount);
+	}
 
 	put_filesystem(type);
 	if (IS_ERR(mnt))
-- 
2.25.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 4.19,v2] VFS: Fix fuseblk memory leak caused by mount concurrency
  2021-10-13  9:51 [PATCH 4.19,v2] VFS: Fix fuseblk memory leak caused by mount concurrency ChenXiaoSong
@ 2021-10-13 10:11 ` Greg KH
  2021-10-13 10:38   ` chenxiaosong (A)
  0 siblings, 1 reply; 7+ messages in thread
From: Greg KH @ 2021-10-13 10:11 UTC (permalink / raw)
  To: ChenXiaoSong
  Cc: viro, stable, linux-fsdevel, linux-kernel, dhowells, yukuai3,
	yi.zhang, zhangxiaoxu5

On Wed, Oct 13, 2021 at 05:51:01PM +0800, ChenXiaoSong wrote:
> If two processes mount same superblock, memory leak occurs:
> 
> CPU0               |  CPU1
> do_new_mount       |  do_new_mount
>   fs_set_subtype   |    fs_set_subtype
>     kstrdup        |
>                    |      kstrdup
>     memrory leak   |
> 
> Fix this by adding a write lock while calling fs_set_subtype.
> 
> Linus's tree already have refactoring patchset [1], one of them can fix this bug:
>         c30da2e981a7 (fuse: convert to use the new mount API)
> 
> Since we did not merge the refactoring patchset in this branch, I create this patch.
> 
> [1] https://patchwork.kernel.org/project/linux-fsdevel/patch/20190903113640.7984-3-mszeredi@redhat.com/
> 
> Fixes: 79c0b2df79eb (add filesystem subtype support)
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
> ---
> v1: Can not mount sshfs ([PATCH linux-4.19.y] VFS: Fix fuseblk memory leak caused by mount concurrency)
> v2: Use write lock while writing superblock
> 
>  fs/namespace.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

As you are referring to a fuse-only patch above, why are you trying to
resolve this issue in the core namespace code instead?

How does fuse have anything to do with this?

confused,

greg k-h

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 4.19,v2] VFS: Fix fuseblk memory leak caused by mount concurrency
  2021-10-13 10:11 ` Greg KH
@ 2021-10-13 10:38   ` chenxiaosong (A)
  2021-10-13 10:49     ` chenxiaosong (A)
  0 siblings, 1 reply; 7+ messages in thread
From: chenxiaosong (A) @ 2021-10-13 10:38 UTC (permalink / raw)
  To: Greg KH
  Cc: viro, stable, linux-fsdevel, linux-kernel, dhowells, yukuai3,
	yi.zhang, zhangxiaoxu5

在 2021/10/13 18:11, Greg KH 写道:
> On Wed, Oct 13, 2021 at 05:51:01PM +0800, ChenXiaoSong wrote:
>> If two processes mount same superblock, memory leak occurs:
>>
>> CPU0               |  CPU1
>> do_new_mount       |  do_new_mount
>>    fs_set_subtype   |    fs_set_subtype
>>      kstrdup        |
>>                     |      kstrdup
>>      memrory leak   |
>>
>> Fix this by adding a write lock while calling fs_set_subtype.
>>
>> Linus's tree already have refactoring patchset [1], one of them can fix this bug:
>>          c30da2e981a7 (fuse: convert to use the new mount API)
>>
>> Since we did not merge the refactoring patchset in this branch, I create this patch.
>>
>> [1] https://patchwork.kernel.org/project/linux-fsdevel/patch/20190903113640.7984-3-mszeredi@redhat.com/
>>
>> Fixes: 79c0b2df79eb (add filesystem subtype support)
>> Cc: David Howells <dhowells@redhat.com>
>> Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
>> ---
>> v1: Can not mount sshfs ([PATCH linux-4.19.y] VFS: Fix fuseblk memory leak caused by mount concurrency)
>> v2: Use write lock while writing superblock
>>
>>   fs/namespace.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> As you are referring to a fuse-only patch above, why are you trying to
> resolve this issue in the core namespace code instead?
> 
> How does fuse have anything to do with this?
> 
> confused,
> 
> greg k-h
> .
> 

Now, only `fuse_fs_type` and `fuseblk_fs_type` has `FS_HAS_SUBTYPE` flag 
in kernel code, but maybe there is a filesystem module(`struct 
file_system_type` has `FS_HAS_SUBTYPE` flag). And only mounting fuseblk 
filesystem(e.g. ntfs) will occur memory leak now.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 4.19,v2] VFS: Fix fuseblk memory leak caused by mount concurrency
  2021-10-13 10:38   ` chenxiaosong (A)
@ 2021-10-13 10:49     ` chenxiaosong (A)
  2021-10-20 12:30       ` Greg KH
  0 siblings, 1 reply; 7+ messages in thread
From: chenxiaosong (A) @ 2021-10-13 10:49 UTC (permalink / raw)
  To: Greg KH
  Cc: viro, stable, linux-fsdevel, linux-kernel, dhowells, yukuai3,
	yi.zhang, zhangxiaoxu5

在 2021/10/13 18:38, chenxiaosong (A) 写道:
> 在 2021/10/13 18:11, Greg KH 写道:
>> On Wed, Oct 13, 2021 at 05:51:01PM +0800, ChenXiaoSong wrote:
>>> If two processes mount same superblock, memory leak occurs:
>>>
>>> CPU0               |  CPU1
>>> do_new_mount       |  do_new_mount
>>>    fs_set_subtype   |    fs_set_subtype
>>>      kstrdup        |
>>>                     |      kstrdup
>>>      memrory leak   |
>>>
>>> Fix this by adding a write lock while calling fs_set_subtype.
>>>
>>> Linus's tree already have refactoring patchset [1], one of them can 
>>> fix this bug:
>>>          c30da2e981a7 (fuse: convert to use the new mount API)
>>>
>>> Since we did not merge the refactoring patchset in this branch, I 
>>> create this patch.
>>>
>>> [1] 
>>> https://patchwork.kernel.org/project/linux-fsdevel/patch/20190903113640.7984-3-mszeredi@redhat.com/ 
>>>
>>>
>>> Fixes: 79c0b2df79eb (add filesystem subtype support)
>>> Cc: David Howells <dhowells@redhat.com>
>>> Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
>>> ---
>>> v1: Can not mount sshfs ([PATCH linux-4.19.y] VFS: Fix fuseblk memory 
>>> leak caused by mount concurrency)
>>> v2: Use write lock while writing superblock
>>>
>>>   fs/namespace.c | 9 ++++++---
>>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> As you are referring to a fuse-only patch above, why are you trying to
>> resolve this issue in the core namespace code instead?
>>
>> How does fuse have anything to do with this?
>>
>> confused,
>>
>> greg k-h
>> .
>>
> 
> Now, only `fuse_fs_type` and `fuseblk_fs_type` has `FS_HAS_SUBTYPE` flag 
> in kernel code, but maybe there is a filesystem module(`struct 
> file_system_type` has `FS_HAS_SUBTYPE` flag). And only mounting fuseblk 
> filesystem(e.g. ntfs) will occur memory leak now.

How about updating the subject as: VFS: Fix memory leak caused by 
mounting fs with subtype concurrency?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 4.19,v2] VFS: Fix fuseblk memory leak caused by mount concurrency
  2021-10-13 10:49     ` chenxiaosong (A)
@ 2021-10-20 12:30       ` Greg KH
  2021-10-26  2:18         ` chenxiaosong (A)
  0 siblings, 1 reply; 7+ messages in thread
From: Greg KH @ 2021-10-20 12:30 UTC (permalink / raw)
  To: chenxiaosong (A)
  Cc: viro, stable, linux-fsdevel, linux-kernel, dhowells, yukuai3,
	yi.zhang, zhangxiaoxu5

On Wed, Oct 13, 2021 at 06:49:06PM +0800, chenxiaosong (A) wrote:
> 在 2021/10/13 18:38, chenxiaosong (A) 写道:
> > 在 2021/10/13 18:11, Greg KH 写道:
> > > On Wed, Oct 13, 2021 at 05:51:01PM +0800, ChenXiaoSong wrote:
> > > > If two processes mount same superblock, memory leak occurs:
> > > > 
> > > > CPU0               |  CPU1
> > > > do_new_mount       |  do_new_mount
> > > >    fs_set_subtype   |    fs_set_subtype
> > > >      kstrdup        |
> > > >                     |      kstrdup
> > > >      memrory leak   |
> > > > 
> > > > Fix this by adding a write lock while calling fs_set_subtype.
> > > > 
> > > > Linus's tree already have refactoring patchset [1], one of them
> > > > can fix this bug:
> > > >          c30da2e981a7 (fuse: convert to use the new mount API)
> > > > 
> > > > Since we did not merge the refactoring patchset in this branch,
> > > > I create this patch.
> > > > 
> > > > [1] https://patchwork.kernel.org/project/linux-fsdevel/patch/20190903113640.7984-3-mszeredi@redhat.com/
> > > > 
> > > > 
> > > > Fixes: 79c0b2df79eb (add filesystem subtype support)
> > > > Cc: David Howells <dhowells@redhat.com>
> > > > Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
> > > > ---
> > > > v1: Can not mount sshfs ([PATCH linux-4.19.y] VFS: Fix fuseblk
> > > > memory leak caused by mount concurrency)
> > > > v2: Use write lock while writing superblock
> > > > 
> > > >   fs/namespace.c | 9 ++++++---
> > > >   1 file changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > As you are referring to a fuse-only patch above, why are you trying to
> > > resolve this issue in the core namespace code instead?
> > > 
> > > How does fuse have anything to do with this?
> > > 
> > > confused,
> > > 
> > > greg k-h
> > > .
> > > 
> > 
> > Now, only `fuse_fs_type` and `fuseblk_fs_type` has `FS_HAS_SUBTYPE` flag
> > in kernel code, but maybe there is a filesystem module(`struct
> > file_system_type` has `FS_HAS_SUBTYPE` flag). And only mounting fuseblk
> > filesystem(e.g. ntfs) will occur memory leak now.
> 
> How about updating the subject as: VFS: Fix memory leak caused by mounting
> fs with subtype concurrency?

That would be a better idea, but still, this is not obvious that this is
the correct fix at all...

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 4.19,v2] VFS: Fix fuseblk memory leak caused by mount concurrency
  2021-10-20 12:30       ` Greg KH
@ 2021-10-26  2:18         ` chenxiaosong (A)
  2021-10-26  5:45           ` Greg KH
  0 siblings, 1 reply; 7+ messages in thread
From: chenxiaosong (A) @ 2021-10-26  2:18 UTC (permalink / raw)
  To: Greg KH
  Cc: viro, stable, linux-fsdevel, linux-kernel, dhowells, yukuai3,
	yi.zhang, zhangxiaoxu5



在 2021/10/20 20:30, Greg KH 写道:
> On Wed, Oct 13, 2021 at 06:49:06PM +0800, chenxiaosong (A) wrote:
>> 在 2021/10/13 18:38, chenxiaosong (A) 写道:
>>> 在 2021/10/13 18:11, Greg KH 写道:
>>>> On Wed, Oct 13, 2021 at 05:51:01PM +0800, ChenXiaoSong wrote:
>>>>> If two processes mount same superblock, memory leak occurs:
>>>>>
>>>>> CPU0               |  CPU1
>>>>> do_new_mount       |  do_new_mount
>>>>>     fs_set_subtype   |    fs_set_subtype
>>>>>       kstrdup        |
>>>>>                      |      kstrdup
>>>>>       memrory leak   |
>>>>>
>>>>> Fix this by adding a write lock while calling fs_set_subtype.
>>>>>
>>>>> Linus's tree already have refactoring patchset [1], one of them
>>>>> can fix this bug:
>>>>>           c30da2e981a7 (fuse: convert to use the new mount API)
>>>>>
>>>>> Since we did not merge the refactoring patchset in this branch,
>>>>> I create this patch.
>>>>>
>>>>> [1] https://patchwork.kernel.org/project/linux-fsdevel/patch/20190903113640.7984-3-mszeredi@redhat.com/
>>>>>
>>>>>
>>>>> Fixes: 79c0b2df79eb (add filesystem subtype support)
>>>>> Cc: David Howells <dhowells@redhat.com>
>>>>> Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
>>>>> ---
>>>>> v1: Can not mount sshfs ([PATCH linux-4.19.y] VFS: Fix fuseblk
>>>>> memory leak caused by mount concurrency)
>>>>> v2: Use write lock while writing superblock
>>>>>
>>>>>    fs/namespace.c | 9 ++++++---
>>>>>    1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> As you are referring to a fuse-only patch above, why are you trying to
>>>> resolve this issue in the core namespace code instead?
>>>>
>>>> How does fuse have anything to do with this?
>>>>
>>>> confused,
>>>>
>>>> greg k-h
>>>> .
>>>>
>>>
>>> Now, only `fuse_fs_type` and `fuseblk_fs_type` has `FS_HAS_SUBTYPE` flag
>>> in kernel code, but maybe there is a filesystem module(`struct
>>> file_system_type` has `FS_HAS_SUBTYPE` flag). And only mounting fuseblk
>>> filesystem(e.g. ntfs) will occur memory leak now.
>>
>> How about updating the subject as: VFS: Fix memory leak caused by mounting
>> fs with subtype concurrency?
> 
> That would be a better idea, but still, this is not obvious that this is
> the correct fix at all...
> .
> 
Why is this patch not correct? Can you tell me more about it? Thanks.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 4.19,v2] VFS: Fix fuseblk memory leak caused by mount concurrency
  2021-10-26  2:18         ` chenxiaosong (A)
@ 2021-10-26  5:45           ` Greg KH
  0 siblings, 0 replies; 7+ messages in thread
From: Greg KH @ 2021-10-26  5:45 UTC (permalink / raw)
  To: chenxiaosong (A)
  Cc: viro, stable, linux-fsdevel, linux-kernel, dhowells, yukuai3,
	yi.zhang, zhangxiaoxu5

On Tue, Oct 26, 2021 at 10:18:11AM +0800, chenxiaosong (A) wrote:
> 
> 
> 在 2021/10/20 20:30, Greg KH 写道:
> > On Wed, Oct 13, 2021 at 06:49:06PM +0800, chenxiaosong (A) wrote:
> > > 在 2021/10/13 18:38, chenxiaosong (A) 写道:
> > > > 在 2021/10/13 18:11, Greg KH 写道:
> > > > > On Wed, Oct 13, 2021 at 05:51:01PM +0800, ChenXiaoSong wrote:
> > > > > > If two processes mount same superblock, memory leak occurs:
> > > > > > 
> > > > > > CPU0               |  CPU1
> > > > > > do_new_mount       |  do_new_mount
> > > > > >     fs_set_subtype   |    fs_set_subtype
> > > > > >       kstrdup        |
> > > > > >                      |      kstrdup
> > > > > >       memrory leak   |
> > > > > > 
> > > > > > Fix this by adding a write lock while calling fs_set_subtype.
> > > > > > 
> > > > > > Linus's tree already have refactoring patchset [1], one of them
> > > > > > can fix this bug:
> > > > > >           c30da2e981a7 (fuse: convert to use the new mount API)
> > > > > > 
> > > > > > Since we did not merge the refactoring patchset in this branch,
> > > > > > I create this patch.
> > > > > > 
> > > > > > [1] https://patchwork.kernel.org/project/linux-fsdevel/patch/20190903113640.7984-3-mszeredi@redhat.com/
> > > > > > 
> > > > > > 
> > > > > > Fixes: 79c0b2df79eb (add filesystem subtype support)
> > > > > > Cc: David Howells <dhowells@redhat.com>
> > > > > > Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
> > > > > > ---
> > > > > > v1: Can not mount sshfs ([PATCH linux-4.19.y] VFS: Fix fuseblk
> > > > > > memory leak caused by mount concurrency)
> > > > > > v2: Use write lock while writing superblock
> > > > > > 
> > > > > >    fs/namespace.c | 9 ++++++---
> > > > > >    1 file changed, 6 insertions(+), 3 deletions(-)
> > > > > 
> > > > > As you are referring to a fuse-only patch above, why are you trying to
> > > > > resolve this issue in the core namespace code instead?
> > > > > 
> > > > > How does fuse have anything to do with this?
> > > > > 
> > > > > confused,
> > > > > 
> > > > > greg k-h
> > > > > .
> > > > > 
> > > > 
> > > > Now, only `fuse_fs_type` and `fuseblk_fs_type` has `FS_HAS_SUBTYPE` flag
> > > > in kernel code, but maybe there is a filesystem module(`struct
> > > > file_system_type` has `FS_HAS_SUBTYPE` flag). And only mounting fuseblk
> > > > filesystem(e.g. ntfs) will occur memory leak now.
> > > 
> > > How about updating the subject as: VFS: Fix memory leak caused by mounting
> > > fs with subtype concurrency?
> > 
> > That would be a better idea, but still, this is not obvious that this is
> > the correct fix at all...
> > .
> > 
> Why is this patch not correct? Can you tell me more about it? Thanks.

You need to prove that it is correct, and you need to get maintainers to
approve it.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-10-26  5:45 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-13  9:51 [PATCH 4.19,v2] VFS: Fix fuseblk memory leak caused by mount concurrency ChenXiaoSong
2021-10-13 10:11 ` Greg KH
2021-10-13 10:38   ` chenxiaosong (A)
2021-10-13 10:49     ` chenxiaosong (A)
2021-10-20 12:30       ` Greg KH
2021-10-26  2:18         ` chenxiaosong (A)
2021-10-26  5:45           ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).