| Message ID | CAH2r5mtJA0AO+5YGXUKhnb0rtnezrNufZkpMAAuJ5tEKTibgig@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SMB,CLIENT] fix refcount issue that shutdown related xfstests uncovered | expand |
This does not fix the umount/mount busy errors you see with tests like generic/043 and generic/048 but it does fix the rmmod problem. And FYI there is a workaround for fixing the umount/mount issues in those tests - by simply adding a 1 second delay in umount. We need to continue to debug the generic/043 and generic/048 umount busy errors On Fri, Aug 16, 2024 at 4:56 PM Steve French <smfrench@gmail.com> wrote: > > smb3: fix problem unloading module due to leaked refcount on shutdown > > The shutdown ioctl can leak a refcount on the tlink which can > prevent rmmod (unloading the cifs.ko) module from working. > > Found while debugging xfstest generic/043 > > Fixes: 69ca1f57555f ("smb3: add dynamic tracepoints for shutdown ioctl") > > See attached > > -- > Thanks, > > Steve -- Thanks, Steve
On Sat, Aug 17, 2024 at 10:39 AM Steve French <smfrench@gmail.com> wrote: > > This does not fix the umount/mount busy errors you see with tests like > generic/043 and generic/048 but it does fix the rmmod problem. And > FYI there is a workaround for fixing the umount/mount issues in those > tests - by simply adding a 1 second delay in umount. We need to > continue to debug the generic/043 and generic/048 umount busy errors > > > On Fri, Aug 16, 2024 at 4:56 PM Steve French <smfrench@gmail.com> wrote: > > > > smb3: fix problem unloading module due to leaked refcount on shutdown > > > > The shutdown ioctl can leak a refcount on the tlink which can > > prevent rmmod (unloading the cifs.ko) module from working. > > > > Found while debugging xfstest generic/043 > > > > Fixes: 69ca1f57555f ("smb3: add dynamic tracepoints for shutdown ioctl") > > > > See attached > > > > -- > > Thanks, > > > > Steve > > > > -- > Thanks, > > Steve Looks good to me. Did you do a cursory check to see if we drop references in all other places where we call cifs_sb_tlink? Just for completeness?
On Thu, Aug 22, 2024 at 12:30 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Sat, Aug 17, 2024 at 10:39 AM Steve French <smfrench@gmail.com> wrote: > > > > This does not fix the umount/mount busy errors you see with tests like > > generic/043 and generic/048 but it does fix the rmmod problem. And > > FYI there is a workaround for fixing the umount/mount issues in those > > tests - by simply adding a 1 second delay in umount. We need to > > continue to debug the generic/043 and generic/048 umount busy errors > > > > > > On Fri, Aug 16, 2024 at 4:56 PM Steve French <smfrench@gmail.com> wrote: > > > > > > smb3: fix problem unloading module due to leaked refcount on shutdown > > > > > > The shutdown ioctl can leak a refcount on the tlink which can > > > prevent rmmod (unloading the cifs.ko) module from working. > > > > > > Found while debugging xfstest generic/043 > > > > > > Fixes: 69ca1f57555f ("smb3: add dynamic tracepoints for shutdown ioctl") > > > > > > See attached > > > > > > -- > > > Thanks, > > > > > > Steve > > > > > > > > -- > > Thanks, > > > > Steve > > Looks good to me. > Did you do a cursory check to see if we drop references in all other > places where we call cifs_sb_tlink? Just for completeness? I have checked almost all (46) places we call it last week, but will check the remaining ones today.
From da35d443deaa64e7072969a413e3769332bc5849 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Fri, 16 Aug 2024 16:47:39 -0500
Subject: [PATCH] smb3: fix problem unloading module due to leaked refcount on
shutdown
The shutdown ioctl can leak a refcount on the tlink which can
prevent rmmod (unloading the cifs.ko) module from working.
Found while debugging xfstest generic/043
Fixes: 69ca1f57555f ("smb3: add dynamic tracepoints for shutdown ioctl")
Signed-off-by: Steve French <stfrench@microsoft.com>
---
fs/smb/client/connect.c | 3 +++
fs/smb/client/ioctl.c | 2 ++
fs/smb/client/link.c | 1 +
3 files changed, 6 insertions(+)
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index d2307162a2de..c1c14274930a 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -4194,6 +4194,9 @@ tlink_rb_insert(struct rb_root *root, struct tcon_link *new_tlink)
*
* If one doesn't exist then insert a new tcon_link struct into the tree and
* try to construct a new one.
+ *
+ * REMEMBER to call cifs_put_tlink() after successful calls to cifs_sb_tlink,
+ * to avoid refcount issues
*/
struct tcon_link *
cifs_sb_tlink(struct cifs_sb_info *cifs_sb)
diff --git a/fs/smb/client/ioctl.c b/fs/smb/client/ioctl.c
index 44dbaf9929a4..9bb5c869f4db 100644
--- a/fs/smb/client/ioctl.c
+++ b/fs/smb/client/ioctl.c
@@ -229,9 +229,11 @@ static int cifs_shutdown(struct super_block *sb, unsigned long arg)
shutdown_good:
trace_smb3_shutdown_done(flags, tcon->tid);
+ cifs_put_tlink(tlink);
return 0;
shutdown_out_err:
trace_smb3_shutdown_err(rc, flags, tcon->tid);
+ cifs_put_tlink(tlink);
return rc;
}
diff --git a/fs/smb/client/link.c b/fs/smb/client/link.c
index d86da949a919..80099bbb333b 100644
--- a/fs/smb/client/link.c
+++ b/fs/smb/client/link.c
@@ -588,6 +588,7 @@ cifs_symlink(struct mnt_idmap *idmap, struct inode *inode,
tlink = cifs_sb_tlink(cifs_sb);
if (IS_ERR(tlink)) {
rc = PTR_ERR(tlink);
+ /* BB could be clearer if skipped put_tlink on error here, but harmless */
goto symlink_exit;
}
pTcon = tlink_tcon(tlink);
--
2.43.0
smb3: fix problem unloading module due to leaked refcount on shutdown The shutdown ioctl can leak a refcount on the tlink which can prevent rmmod (unloading the cifs.ko) module from working. Found while debugging xfstest generic/043 Fixes: 69ca1f57555f ("smb3: add dynamic tracepoints for shutdown ioctl") See attached