[PATCH] cifs: Fix connections leak when tlink setup failed

[PATCH] cifs: Fix connections leak when tlink setup failed

[Zhang Xiaoxu]

If the tlink setup failed, lost to put the connections, then
the module refcnt leak since the cifsd kthread not exit.

Also leak the fscache info, and for next mount with fsc,it will
print the follow errors:
  CIFS: Cache volume key already in use (cifs,127.0.0.1:445,TEST)

Let's check the result of tlink setup, and put the connection when
error happened.

Fixes: 56c762eb9bee ("cifs: Refactor out cifs_mount()")
Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
---
 fs/cifs/connect.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 1cc47dd3b4d6..e699e45e70c4 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3855,14 +3855,19 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, struct smb3_fs_context *ctx)
 	uuid_copy(&cifs_sb->dfs_mount_id, &mnt_ctx.mount_id);
 
 out:
-	free_xid(mnt_ctx.xid);
 	cifs_try_adding_channels(cifs_sb, mnt_ctx.ses);
-	return mount_setup_tlink(cifs_sb, mnt_ctx.ses, mnt_ctx.tcon);
+	rc = mount_setup_tlink(cifs_sb, mnt_ctx.ses, mnt_ctx.tcon);
+	if (rc)
+		goto put_conns;
+
+	free_xid(mnt_ctx.xid);
+	return rc;
 
 error:
 	dfs_cache_put_refsrv_sessions(&mnt_ctx.mount_id);
 	kfree(mnt_ctx.origin_fullpath);
 	kfree(mnt_ctx.leaf_fullpath);
+put_conns:
 	mount_put_conns(&mnt_ctx);
 	return rc;
 }
@@ -3884,8 +3889,12 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, struct smb3_fs_context *ctx)
 			goto error;
 	}
 
+	rc = mount_setup_tlink(cifs_sb, mnt_ctx.ses, mnt_ctx.tcon);
+	if (rc)
+		goto error;
+
 	free_xid(mnt_ctx.xid);
-	return mount_setup_tlink(cifs_sb, mnt_ctx.ses, mnt_ctx.tcon);
+	return rc;
 
 error:
 	mount_put_conns(&mnt_ctx);
-- 
2.31.1

Re: [PATCH] cifs: Fix connections leak when tlink setup failed

[Paulo Alcantara]

Zhang Xiaoxu <zhangxiaoxu5@huawei.com> writes:

> If the tlink setup failed, lost to put the connections, then
> the module refcnt leak since the cifsd kthread not exit.
>
> Also leak the fscache info, and for next mount with fsc,it will
> print the follow errors:
>   CIFS: Cache volume key already in use (cifs,127.0.0.1:445,TEST)
>
> Let's check the result of tlink setup, and put the connection when
> error happened.
>
> Fixes: 56c762eb9bee ("cifs: Refactor out cifs_mount()")
> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
> ---
>  fs/cifs/connect.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 1cc47dd3b4d6..e699e45e70c4 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -3855,14 +3855,19 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, struct smb3_fs_context *ctx)
>  	uuid_copy(&cifs_sb->dfs_mount_id, &mnt_ctx.mount_id);
>  
>  out:
> -	free_xid(mnt_ctx.xid);
>  	cifs_try_adding_channels(cifs_sb, mnt_ctx.ses);
> -	return mount_setup_tlink(cifs_sb, mnt_ctx.ses, mnt_ctx.tcon);
> +	rc = mount_setup_tlink(cifs_sb, mnt_ctx.ses, mnt_ctx.tcon);
> +	if (rc)
> +		goto put_conns;

Good catch.  However, this would partially fix the leaked connections as
you must still call dfs_cache_put_refsrv_sessions() to put all other
connections that were used for chasing DFS referrals.  For non-DFS
mounts, it wouldn't be a problem, though.

What about something like below

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 1cc47dd3b4d6..083ba70f3c1a 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3855,9 +3855,13 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, struct smb3_fs_context *ctx)
 	uuid_copy(&cifs_sb->dfs_mount_id, &mnt_ctx.mount_id);
 
 out:
-	free_xid(mnt_ctx.xid);
 	cifs_try_adding_channels(cifs_sb, mnt_ctx.ses);
-	return mount_setup_tlink(cifs_sb, mnt_ctx.ses, mnt_ctx.tcon);
+	rc =  mount_setup_tlink(cifs_sb, mnt_ctx.ses, mnt_ctx.tcon);
+	if (rc)
+		goto error;
+
+	free_xid(mnt_ctx.xid);
+	return rc;
 
 error:
 	dfs_cache_put_refsrv_sessions(&mnt_ctx.mount_id);

Re: [PATCH] cifs: Fix connections leak when tlink setup failed

[zhangxiaoxu (A)]

On 2022/11/11 5:29, Paulo Alcantara wrote:
> Zhang Xiaoxu <zhangxiaoxu5@huawei.com> writes:
> 
>> If the tlink setup failed, lost to put the connections, then
>> the module refcnt leak since the cifsd kthread not exit.
>>
>> Also leak the fscache info, and for next mount with fsc,it will
>> print the follow errors:
>>    CIFS: Cache volume key already in use (cifs,127.0.0.1:445,TEST)
>>
>> Let's check the result of tlink setup, and put the connection when
>> error happened.
>>
>> Fixes: 56c762eb9bee ("cifs: Refactor out cifs_mount()")
>> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
>> ---
>>   fs/cifs/connect.c | 15 ++++++++++++---
>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index 1cc47dd3b4d6..e699e45e70c4 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -3855,14 +3855,19 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, struct smb3_fs_context *ctx)
>>   	uuid_copy(&cifs_sb->dfs_mount_id, &mnt_ctx.mount_id);
>>   
>>   out:
>> -	free_xid(mnt_ctx.xid);
>>   	cifs_try_adding_channels(cifs_sb, mnt_ctx.ses);
>> -	return mount_setup_tlink(cifs_sb, mnt_ctx.ses, mnt_ctx.tcon);
>> +	rc = mount_setup_tlink(cifs_sb, mnt_ctx.ses, mnt_ctx.tcon);
>> +	if (rc)
>> +		goto put_conns;
> 
> Good catch.  However, this would partially fix the leaked connections as
> you must still call dfs_cache_put_refsrv_sessions() to put all other
> connections that were used for chasing DFS referrals.  For non-DFS
> mounts, it wouldn't be a problem, though.
> 
> What about something like below
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 1cc47dd3b4d6..083ba70f3c1a 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -3855,9 +3855,13 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, struct smb3_fs_context *ctx)
>   	uuid_copy(&cifs_sb->dfs_mount_id, &mnt_ctx.mount_id);
>   
>   out:
> -	free_xid(mnt_ctx.xid);
>   	cifs_try_adding_channels(cifs_sb, mnt_ctx.ses);
> -	return mount_setup_tlink(cifs_sb, mnt_ctx.ses, mnt_ctx.tcon);
> +	rc =  mount_setup_tlink(cifs_sb, mnt_ctx.ses, mnt_ctx.tcon);
> +	if (rc)
> +		goto error;
> +
> +	free_xid(mnt_ctx.xid);
> +	return rc;
>   
>   error:
>   	dfs_cache_put_refsrv_sessions(&mnt_ctx.mount_id);
Thanks Paulo, I will check this point and send v2.