linux-kernel - Re: [PATCH] smb: Fix refcount leak for cifs_sb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aO/DLq/OtAjvkgcY@chcpu18>
Date: Wed, 15 Oct 2025 15:52:14 +0000
From: Shuhao Fu <sfual@....ust.hk>
To: Markus Elfring <Markus.Elfring@....de>
Cc: linux-cifs@...r.kernel.org, samba-technical@...ts.samba.org,
        Bharath SM <bharathsm@...rosoft.com>,
        Paulo Alcantara <pc@...guebit.org>,
        Ronnie Sahlberg <ronniesahlberg@...il.com>,
        Shyam Prasad N <sprasad@...rosoft.com>,
        Steve French <sfrench@...ba.org>, Steve French <smfrench@...il.com>,
        Tom Talpey <tom@...pey.com>, LKML <linux-kernel@...r.kernel.org>,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] smb: Fix refcount leak for cifs_sb_tlink

On Wed, Oct 15, 2025 at 04:52:23PM +0200, Markus Elfring wrote:
> > This patch fixes …
> 
> * Will another imperative wording approach become more helpful for an improved
>   change description?
>   https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2FDocumentation%2Fprocess%2Fsubmitting-patches.rst%3Fh%3Dv6.17%23n94&data=05%7C02%7Csfual%40connect.ust.hk%7Caffcb410915f4b4bc8f308de0bfa853c%7C6c1d415239d044ca88d9b8d6ddca0708%7C1%7C0%7C638961367775911255%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=bThHSbvjokcDU6hNpnYxt4%2BlVyzlyxHl1JopGmCLY%2FQ%3D&reserved=0
> 
> * Would it be more helpful to use the label “put_tlink” instead of “out”?
> 
> * Can a subject like “smb: client: Complete reference counting in three functions”
>   be nicer?
> 
> 
> Regards,
> Markus

Hi,

Thanks for the suggestions. My apologies for the inapproriate wording. 
Here's my updates. Please do let me know if it still needs improvement.
I will definitely address these issues in patch v2.

1. An improved patch description

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 callsites 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@....ust.hk>

2. New subject: [PATCH v2] smb: client: Complete reference counting in three functions

3. Labels are changed accordingly

@@ -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;

Thanks,
Shuhao