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

[Shuhao Fu <sfual@cse.ust.hk>]

On Wed, Oct 15, 2025 at 11:29:46AM -0500, Steve French wrote:
> I don't think the title needs to be changed, it seems clear enough.
> The other changes are minor (changing goto label) and also probably
> not needed but ok if you have to update it for other reasons.
> 

Thank you for your comments. I did some minor changes to this patch, 
including better wording and nicer goto labels.

Link to v2: https://lore.kernel.org/linux-cifs/aPBeBxTQLeyFl4mx@chcpu18/

Thanks,
Shuhao

> On Wed, Oct 15, 2025 at 10:52 AM Shuhao Fu <sfual@cse.ust.hk> wrote:
> >
> > 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%7Cf73c7723b8104879a14408de0c0818dd%7C6c1d415239d044ca88d9b8d6ddca0708%7C1%7C0%7C638961426106746132%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=qBAe0Dgw57Ff%2BjixeyPqtA86BO8922uMmi9sldpDlbY%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@cse.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
> 
> 
> 
> -- 
> Thanks,
> 
> Steve