From: Shuhao Fu <sfual@cse.ust.hk>
To: Steve French <smfrench@gmail.com>
Cc: Markus Elfring <Markus.Elfring@web.de>,
linux-cifs@vger.kernel.org, samba-technical@lists.samba.org,
Bharath SM <bharathsm@microsoft.com>,
Paulo Alcantara <pc@manguebit.org>,
Ronnie Sahlberg <ronniesahlberg@gmail.com>,
Shyam Prasad N <sprasad@microsoft.com>,
Steve French <sfrench@samba.org>, Tom Talpey <tom@talpey.com>,
LKML <linux-kernel@vger.kernel.org>,
kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] smb: Fix refcount leak for cifs_sb_tlink
Date: Thu, 16 Oct 2025 02:59:12 +0000 [thread overview]
Message-ID: <aPBfgADPxxoV34tT@chcpu18> (raw)
In-Reply-To: <CAH2r5msK3SDhAM0_monUcNTrf5JCwydD+AJgARaiVziUUo0WmQ@mail.gmail.com>
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
prev parent reply other threads:[~2025-10-16 3:00 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-13 10:14 [PATCH] smb: Fix refcount leak for cifs_sb_tlink Shuhao Fu
2025-10-15 14:52 ` Markus Elfring
2025-10-15 15:52 ` Shuhao Fu
2025-10-15 16:25 ` Markus Elfring
2025-10-15 18:10 ` Steve French
2025-10-15 16:29 ` Steve French
2025-10-16 2:59 ` Shuhao Fu [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aPBfgADPxxoV34tT@chcpu18 \
--to=sfual@cse.ust.hk \
--cc=Markus.Elfring@web.de \
--cc=bharathsm@microsoft.com \
--cc=kernel-janitors@vger.kernel.org \
--cc=linux-cifs@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pc@manguebit.org \
--cc=ronniesahlberg@gmail.com \
--cc=samba-technical@lists.samba.org \
--cc=sfrench@samba.org \
--cc=smfrench@gmail.com \
--cc=sprasad@microsoft.com \
--cc=tom@talpey.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).