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

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

[Markus Elfring]

> This patch fixes …

* Will another imperative wording approach become more helpful for an improved
  change description?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.17#n94

* 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

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

[Shuhao Fu]

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

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

[Markus Elfring]

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

I suggest to omit this introduction.


> Comments for `cifs_sb_tlink` state that `cifs_put_tlink()` needs to be

                             ()?


> called after successful calls to `cifs_sb_tlink`. Three callsites fail

                                                          call sites?


> to update refcount accordingly, leading to possible resource leaks.

* Do we prefer the term “reference count”?

* Is the word “possible” really relevant here?
  (Would you find corresponding case distinctions more helpful?)

* How do you think about to increase the application of scope-based resource management?


Regards,
Markus

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

[Steve French]

I agree that "callsites" is incorrect, it should be "calls" e.g. but
the others are very minor and I think the existing wording is fine for
the others

On Wed, Oct 15, 2025 at 11:25 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > Fix three refcount inconsistency issues related to `cifs_sb_tlink`.
>
> I suggest to omit this introduction.
>
>
> > Comments for `cifs_sb_tlink` state that `cifs_put_tlink()` needs to be
>
>                              ()?
>
>
> > called after successful calls to `cifs_sb_tlink`. Three callsites fail
>
>                                                           call sites?
>
>
> > to update refcount accordingly, leading to possible resource leaks.
>
> * Do we prefer the term “reference count”?
>
> * Is the word “possible” really relevant here?
>   (Would you find corresponding case distinctions more helpful?)
>
> * How do you think about to increase the application of scope-based resource management?
>
>
> Regards,
> Markus



-- 
Thanks,

Steve

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

[Steve French]

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.

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

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

[Shuhao Fu]

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