fs/smb/client/inode.c | 6 ++++-- fs/smb/client/smb2ops.c | 8 ++++---- 2 files changed, 8 insertions(+), 6 deletions(-)
This patch fixes 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>
---
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 out;
}
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);
}
+out:
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 out;
}
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);
}
+out:
cifs_put_tlink(tlink);
free_xid(xid);
return rc;
--
2.39.5 (Apple Git-154)> 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
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
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
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
> 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
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
© 2016 - 2026 Red Hat, Inc.