smb: client: Fix refcount leak for cifs_sb_tlink

[PATCH v2] smb: client: Fix refcount leak for cifs_sb_tlink

Posted by Shuhao Fu 3 months, 3 weeks ago

Fix three refcount inconsistency issues related to `cifs_sb_tlink`. 

Comments for `cifs_sb_tlink` state that `cifs_put_tlink()` needs to be 
called after successful calls to `cifs_sb_tlink()`. Three calls fail to
update refcount accordingly, leading to possible resource leaks.

Fixes: 8ceb98437946 ("CIFS: Move rename to ops struct")
Fixes: 2f1afe25997f ("cifs: Use smb 2 - 3 and cifsacl mount options getacl functions")
Fixes: 366ed846df60 ("cifs: Use smb 2 - 3 and cifsacl mount options setacl function")
Signed-off-by: Shuhao Fu <sfual@cse.ust.hk>
---
Change in v2:
1. improved patch wording
2. nicer goto label naming

Link to v1: https://lore.kernel.org/linux-cifs/aOzRF9JB9VkBKapw@osx.local/
---
 fs/smb/client/inode.c   | 6 ++++--
 fs/smb/client/smb2ops.c | 8 ++++----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
index 239dd84a3..098a79b7a 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -2431,8 +2431,10 @@ cifs_do_rename(const unsigned int xid, struct dentry *from_dentry,
 	tcon = tlink_tcon(tlink);
 	server = tcon->ses->server;
 
-	if (!server->ops->rename)
-		return -ENOSYS;
+	if (!server->ops->rename) {
+		rc = -ENOSYS;
+		goto do_rename_exit;
+	}
 
 	/* try path-based rename first */
 	rc = server->ops->rename(xid, tcon, from_dentry,
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 7c392cf59..00b3f769e 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -3212,8 +3212,7 @@ get_smb2_acl_by_path(struct cifs_sb_info *cifs_sb,
 	utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
 	if (!utf16_path) {
 		rc = -ENOMEM;
-		free_xid(xid);
-		return ERR_PTR(rc);
+		goto put_tlink;
 	}
 
 	oparms = (struct cifs_open_parms) {
@@ -3245,6 +3244,7 @@ get_smb2_acl_by_path(struct cifs_sb_info *cifs_sb,
 		SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid);
 	}
 
+put_tlink:
 	cifs_put_tlink(tlink);
 	free_xid(xid);
 
@@ -3285,8 +3285,7 @@ set_smb2_acl(struct smb_ntsd *pnntsd, __u32 acllen,
 	utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
 	if (!utf16_path) {
 		rc = -ENOMEM;
-		free_xid(xid);
-		return rc;
+		goto put_tlink;
 	}
 
 	oparms = (struct cifs_open_parms) {
@@ -3307,6 +3306,7 @@ set_smb2_acl(struct smb_ntsd *pnntsd, __u32 acllen,
 		SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid);
 	}
 
+put_tlink:
 	cifs_put_tlink(tlink);
 	free_xid(xid);
 	return rc;
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2] smb: client: Fix refcount leak for cifs_sb_tlink

Posted by Markus Elfring 3 months, 3 weeks ago

> Fix three refcount inconsistency issues related to `cifs_sb_tlink`.

I find such an introduction sentence not so relevant here.


> Comments for `cifs_sb_tlink` state that `cifs_put_tlink()` needs to be 
> called after successful calls to `cifs_sb_tlink()`. Three calls fail to
> update refcount accordingly, leading to possible resource leaks.

* Can it be preferred to refer to the term “reference count”?

* Would you find a description of corresponding case distinctions more helpful?

* May resource leaks be indicated also in the summary phrase?

* Would it be helpful to append parentheses to function names at more places?

* Is there a need to mention change steps more individually?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.17#n94

* Will development interests grow for the application of scope-based resource management?


Regards,
Markus

Re: [PATCH v2] smb: client: Fix refcount leak for cifs_sb_tlink

Posted by Steve French 3 months, 3 weeks ago

The patch looks fine.  More important is focusing on the fixes (and
missing features) - minor wording of description can be helpful but
MUCH more important is focusing on xfstests, new test scenarios,
automated analysis to find places where use after frees possible etc,
fuzzing (like the great scripts Dr. Morris created for us to show some
potential security issues), fixing the various known bugs, adding the
missing features etc

On Thu, Oct 16, 2025 at 2:22 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > Fix three refcount inconsistency issues related to `cifs_sb_tlink`.
>
> I find such an introduction sentence not so relevant here.
>
>
> > Comments for `cifs_sb_tlink` state that `cifs_put_tlink()` needs to be
> > called after successful calls to `cifs_sb_tlink()`. Three calls fail to
> > update refcount accordingly, leading to possible resource leaks.
>
> * Can it be preferred to refer to the term “reference count”?
>
> * Would you find a description of corresponding case distinctions more helpful?
>
> * May resource leaks be indicated also in the summary phrase?
>
> * Would it be helpful to append parentheses to function names at more places?
>
> * Is there a need to mention change steps more individually?
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.17#n94
>
> * Will development interests grow for the application of scope-based resource management?
>
>
> Regards,
> Markus
>


-- 
Thanks,

Steve

Re: [PATCH v2] smb: client: Fix refcount leak for cifs_sb_tlink

Posted by Steve French 3 months, 3 weeks ago

updated the patch in cifs-2.6.git for-next

On Wed, Oct 15, 2025 at 9:53 PM Shuhao Fu <sfual@cse.ust.hk> wrote:
>
> Fix three refcount inconsistency issues related to `cifs_sb_tlink`.
>
> Comments for `cifs_sb_tlink` state that `cifs_put_tlink()` needs to be
> called after successful calls to `cifs_sb_tlink()`. Three calls fail to
> update refcount accordingly, leading to possible resource leaks.
>
> Fixes: 8ceb98437946 ("CIFS: Move rename to ops struct")
> Fixes: 2f1afe25997f ("cifs: Use smb 2 - 3 and cifsacl mount options getacl functions")
> Fixes: 366ed846df60 ("cifs: Use smb 2 - 3 and cifsacl mount options setacl function")
> Signed-off-by: Shuhao Fu <sfual@cse.ust.hk>
> ---
> Change in v2:
> 1. improved patch wording
> 2. nicer goto label naming
>
> Link to v1: https://lore.kernel.org/linux-cifs/aOzRF9JB9VkBKapw@osx.local/
> ---
>  fs/smb/client/inode.c   | 6 ++++--
>  fs/smb/client/smb2ops.c | 8 ++++----
>  2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> index 239dd84a3..098a79b7a 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -2431,8 +2431,10 @@ cifs_do_rename(const unsigned int xid, struct dentry *from_dentry,
>         tcon = tlink_tcon(tlink);
>         server = tcon->ses->server;
>
> -       if (!server->ops->rename)
> -               return -ENOSYS;
> +       if (!server->ops->rename) {
> +               rc = -ENOSYS;
> +               goto do_rename_exit;
> +       }
>
>         /* try path-based rename first */
>         rc = server->ops->rename(xid, tcon, from_dentry,
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index 7c392cf59..00b3f769e 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -3212,8 +3212,7 @@ get_smb2_acl_by_path(struct cifs_sb_info *cifs_sb,
>         utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
>         if (!utf16_path) {
>                 rc = -ENOMEM;
> -               free_xid(xid);
> -               return ERR_PTR(rc);
> +               goto put_tlink;
>         }
>
>         oparms = (struct cifs_open_parms) {
> @@ -3245,6 +3244,7 @@ get_smb2_acl_by_path(struct cifs_sb_info *cifs_sb,
>                 SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid);
>         }
>
> +put_tlink:
>         cifs_put_tlink(tlink);
>         free_xid(xid);
>
> @@ -3285,8 +3285,7 @@ set_smb2_acl(struct smb_ntsd *pnntsd, __u32 acllen,
>         utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
>         if (!utf16_path) {
>                 rc = -ENOMEM;
> -               free_xid(xid);
> -               return rc;
> +               goto put_tlink;
>         }
>
>         oparms = (struct cifs_open_parms) {
> @@ -3307,6 +3306,7 @@ set_smb2_acl(struct smb_ntsd *pnntsd, __u32 acllen,
>                 SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid);
>         }
>
> +put_tlink:
>         cifs_put_tlink(tlink);
>         free_xid(xid);
>         return rc;
> --
> 2.39.5 (Apple Git-154)
>
>


-- 
Thanks,

Steve