Skip to contentSkip to conversation

smb: client: avoid leaking refcount when cifs_sb_tlink() fails

PATCH Applied Bjoern Doebel 3 participants 3 messages Sep 4 – Sep 5

Subsystems (3)
Stable branch SMB client (CIFS) Kernel SMB3 server (ksmbd)
lore
Share & tools
lore

Bjoern Doebel

author
#0 lore raw

PATCH 1/1 smb: client: avoid leaking refcount when cifs_sb_tlink() fails

cifs_oplock_break() takes over the reference that cifs_queue_oplock_break() acquired when it queued the work, and drops it with _cifsFileInfo_put() once the break has been processed.

Only in setups with "-o multiuser", cifs_sb_tlink() may fail, at which point cifs_oplock_break() returns without putting the file reference, mirroring the reference leak we already fixed in the companion patch to cifs_queue_oplock_break().

This would trigger a crash due to busy inodes on the next unmount:

  BUG: Dentry ... still in use (1) [unmount of cifs cifs]
  VFS: Busy inodes after unmount of cifs (cifs)

Drop the reference on that path as well. Doing so before the out label mirrors the normal path, which also puts the reference before cifs_done_oplock_break().

Found by Sashiko code review. The failure path was not exercised at runtime.

Fixes e8f5f849ffce2 ("cifs: fix potential oops in cifs_oplock_break")
Cc [email protected]
Assisted-by Kiro:claude-opus-5
Signed-off-by Bjoern Doebel <[email protected]>
---
 fs/smb/client/file.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/smb/client/file.c +4 -1
index d7b0a9512dfa3..27b58d9072034 100644
@@ -3348,8 +3348,11 @@ void cifs_oplock_break(struct work_struct *work)
 			TASK_UNINTERRUPTIBLE);
 
 	tlink = cifs_sb_tlink(cifs_sb);
-	if (IS_ERR(tlink))
+	if (IS_ERR(tlink)) {
+		/* drop the reference taken when the break was queued */
+		_cifsFileInfo_put(cfile, false /* do not wait for ourself */, false);
 		goto out;
+	}
 	tcon = tlink_tcon(tlink);
 	server = tcon->ses->server;
Signature · 1 line
2.50.1
maintainer Reviewed-by on v1 1/1
#1 lore raw
Quoted 19 lines · Bjoern Doebel

cifs_oplock_break() takes over the reference that cifs_queue_oplock_break() acquired when it queued the work, and drops it with _cifsFileInfo_put() once the break has been processed.

Only in setups with "-o multiuser", cifs_sb_tlink() may fail, at which point cifs_oplock_break() returns without putting the file reference, mirroring the reference leak we already fixed in the companion patch to cifs_queue_oplock_break().

This would trigger a crash due to busy inodes on the next unmount:

  BUG: Dentry ... still in use (1) [unmount of cifs cifs]
  VFS: Busy inodes after unmount of cifs (cifs)

Drop the reference on that path as well. Doing so before the out label mirrors the normal path, which also puts the reference before cifs_done_oplock_break().

Quoted 7 lines

Found by Sashiko code review. The failure path was not exercised at runtime.

Fixes e8f5f849ffce2 ("cifs: fix potential oops in cifs_oplock_break")
Cc [email protected]
Assisted-by Kiro:claude-opus-5
Signed-off-by Bjoern Doebel <[email protected]>
Reviewed-by Namjae Jeon <[email protected]>

Thanks!

maintainer Applied
#2 lore raw
In reply to Bjoern Doebel
Quoted 11 lines · Bjoern Doebel

cifs_oplock_break() takes over the reference that cifs_queue_oplock_break() acquired when it queued the work, and drops it with _cifsFileInfo_put() once the break has been processed.

Only in setups with "-o multiuser", cifs_sb_tlink() may fail, at which point cifs_oplock_break() returns without putting the file reference, mirroring the reference leak we already fixed in the companion patch to cifs_queue_oplock_break().

This would trigger a crash due to busy inodes on the next unmount:

Quoted 7 lines
  BUG: Dentry ... still in use (1) [unmount of cifs cifs]
  VFS: Busy inodes after unmount of cifs (cifs)

Drop the reference on that path as well. Doing so before the out label mirrors the normal path, which also puts the reference before cifs_done_oplock_break().
...

Applied.