[PATCH] cifs: open files should not hold ref on superblock

[PATCH] cifs: open files should not hold ref on superblock

[nspmangalore]

From: Shyam Prasad N <sprasad@microsoft.com>

Today whenever we deal with a file, in addition to holding
a reference on the dentry, we also get a reference on the
superblock. This happens in two cases:
1. when a new cinode is allocated
2. when an oplock break is being processed

The reasoning for holding the superblock ref was to make sure
that when umount happens, if there are users of inodes and
dentries, it does not try to clean them up and wait for the
last ref to superblock to be dropped by last of such users.

But the side effect of doing that is that umount silently drops
a ref on the superblock and we could have deferred closes and
lease breaks still holding these refs.

Ideally, we should ensure that all of these users of inodes and
dentries are cleaned up at the time of umount, which is what this
code is doing.

This code change allows these code paths to use a ref on the
dentry (and hence the inode). That way, umount is
ensured to clean up SMB client resources when it's the last
ref on the superblock (For ex: when same objects are shared).

The code change also moves the call to close all the files in
deferred close list to the umount code path. It also waits for
oplock_break workers to be flushed before calling
kill_anon_super (which eventually frees up those objects).

Fixes: 24261fc23db9 ("cifs: delay super block destruction until all cifsFileInfo objects are gone")
Fixes: 705c79101ccf ("smb: client: fix use-after-free in cifs_oplock_break")
Cc: <stable@vger.kernel.org>
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/cifsfs.c    |  7 +++++--
 fs/smb/client/cifsproto.h |  1 +
 fs/smb/client/file.c      | 11 ----------
 fs/smb/client/misc.c      | 42 +++++++++++++++++++++++++++++++++++++++
 fs/smb/client/trace.h     |  2 ++
 5 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
index 99b04234a08e6..fcc56481d6cf2 100644
--- a/fs/smb/client/cifsfs.c
+++ b/fs/smb/client/cifsfs.c
@@ -330,10 +330,14 @@ static void cifs_kill_sb(struct super_block *sb)
 
 	/*
 	 * We need to release all dentries for the cached directories
-	 * before we kill the sb.
+	 * and close all deferred file handles before we kill the sb.
 	 */
 	if (cifs_sb->root) {
 		close_all_cached_dirs(cifs_sb);
+		cifs_close_all_deferred_files_sb(cifs_sb);
+
+		/* Wait for all pending oplock breaks to complete */
+		flush_workqueue(cifsoplockd_wq);
 
 		/* finally release root dentry */
 		dput(cifs_sb->root);
@@ -864,7 +868,6 @@ static void cifs_umount_begin(struct super_block *sb)
 	spin_unlock(&tcon->tc_lock);
 	spin_unlock(&cifs_tcp_ses_lock);
 
-	cifs_close_all_deferred_files(tcon);
 	/* cancel_brl_requests(tcon); */ /* BB mark all brl mids as exiting */
 	/* cancel_notify_requests(tcon); */
 	if (tcon->ses && tcon->ses->server) {
diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index 96d6b5325aa33..800a7e418c326 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -261,6 +261,7 @@ void cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode);
 
 void cifs_close_all_deferred_files(struct cifs_tcon *tcon);
 
+void cifs_close_all_deferred_files_sb(struct cifs_sb_info *cifs_sb);
 void cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon,
 					   struct dentry *dentry);
 
diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index 18f31d4eb98de..fb4f9aafe1386 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -711,8 +711,6 @@ struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
 	mutex_init(&cfile->fh_mutex);
 	spin_lock_init(&cfile->file_info_lock);
 
-	cifs_sb_active(inode->i_sb);
-
 	/*
 	 * If the server returned a read oplock and we have mandatory brlocks,
 	 * set oplock level to None.
@@ -767,7 +765,6 @@ static void cifsFileInfo_put_final(struct cifsFileInfo *cifs_file)
 	struct inode *inode = d_inode(cifs_file->dentry);
 	struct cifsInodeInfo *cifsi = CIFS_I(inode);
 	struct cifsLockInfo *li, *tmp;
-	struct super_block *sb = inode->i_sb;
 
 	/*
 	 * Delete any outstanding lock records. We'll lose them when the file
@@ -785,7 +782,6 @@ static void cifsFileInfo_put_final(struct cifsFileInfo *cifs_file)
 
 	cifs_put_tlink(cifs_file->tlink);
 	dput(cifs_file->dentry);
-	cifs_sb_deactive(sb);
 	kfree(cifs_file->symlink_target);
 	kfree(cifs_file);
 }
@@ -3165,12 +3161,6 @@ void cifs_oplock_break(struct work_struct *work)
 	__u64 persistent_fid, volatile_fid;
 	__u16 net_fid;
 
-	/*
-	 * Hold a reference to the superblock to prevent it and its inodes from
-	 * being freed while we are accessing cinode. Otherwise, _cifsFileInfo_put()
-	 * may release the last reference to the sb and trigger inode eviction.
-	 */
-	cifs_sb_active(sb);
 	wait_on_bit(&cinode->flags, CIFS_INODE_PENDING_WRITERS,
 			TASK_UNINTERRUPTIBLE);
 
@@ -3255,7 +3245,6 @@ void cifs_oplock_break(struct work_struct *work)
 	cifs_put_tlink(tlink);
 out:
 	cifs_done_oplock_break(cinode);
-	cifs_sb_deactive(sb);
 }
 
 static int cifs_swap_activate(struct swap_info_struct *sis,
diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
index 22cde46309fe0..318533210648d 100644
--- a/fs/smb/client/misc.c
+++ b/fs/smb/client/misc.c
@@ -28,6 +28,11 @@
 #include "fs_context.h"
 #include "cached_dir.h"
 
+struct tcon_list {
+	struct list_head entry;
+	struct cifs_tcon *tcon;
+};
+
 /* The xid serves as a useful identifier for each incoming vfs request,
    in a similar way to the mid which is useful to track each sent smb,
    and CurrentXid can also provide a running counter (although it
@@ -550,6 +555,43 @@ cifs_close_all_deferred_files(struct cifs_tcon *tcon)
 	}
 }
 
+void cifs_close_all_deferred_files_sb(struct cifs_sb_info *cifs_sb)
+{
+	struct rb_root *root = &cifs_sb->tlink_tree;
+	struct rb_node *node;
+	struct cifs_tcon *tcon;
+	struct tcon_link *tlink;
+	struct tcon_list *tmp_list, *q;
+	LIST_HEAD(tcon_head);
+
+	spin_lock(&cifs_sb->tlink_tree_lock);
+	for (node = rb_first(root); node; node = rb_next(node)) {
+		tlink = rb_entry(node, struct tcon_link, tl_rbnode);
+		tcon = tlink_tcon(tlink);
+		if (IS_ERR(tcon))
+			continue;
+		tmp_list = kmalloc_obj(struct tcon_list, GFP_ATOMIC);
+		if (tmp_list == NULL)
+			break;
+		tmp_list->tcon = tcon;
+		/* Take a reference on tcon to prevent it from being freed */
+		spin_lock(&tcon->tc_lock);
+		++tcon->tc_count;
+		trace_smb3_tcon_ref(tcon->debug_id, tcon->tc_count,
+				    netfs_trace_tcon_ref_get_close_defer_files);
+		spin_unlock(&tcon->tc_lock);
+		list_add_tail(&tmp_list->entry, &tcon_head);
+	}
+	spin_unlock(&cifs_sb->tlink_tree_lock);
+
+	list_for_each_entry_safe(tmp_list, q, &tcon_head, entry) {
+		cifs_close_all_deferred_files(tmp_list->tcon);
+		list_del(&tmp_list->entry);
+		cifs_put_tcon(tmp_list->tcon, netfs_trace_tcon_ref_put_close_defer_files);
+		kfree(tmp_list);
+	}
+}
+
 void cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon,
 					   struct dentry *dentry)
 {
diff --git a/fs/smb/client/trace.h b/fs/smb/client/trace.h
index 9228f95cae2bd..acfbb63086ea2 100644
--- a/fs/smb/client/trace.h
+++ b/fs/smb/client/trace.h
@@ -176,6 +176,7 @@
 	EM(netfs_trace_tcon_ref_get_cached_laundromat,	"GET Ch-Lau") \
 	EM(netfs_trace_tcon_ref_get_cached_lease_break,	"GET Ch-Lea") \
 	EM(netfs_trace_tcon_ref_get_cancelled_close,	"GET Cn-Cls") \
+	EM(netfs_trace_tcon_ref_get_close_defer_files,	"GET Cl-Def") \
 	EM(netfs_trace_tcon_ref_get_dfs_refer,		"GET DfsRef") \
 	EM(netfs_trace_tcon_ref_get_find,		"GET Find  ") \
 	EM(netfs_trace_tcon_ref_get_find_sess_tcon,	"GET FndSes") \
@@ -187,6 +188,7 @@
 	EM(netfs_trace_tcon_ref_put_cancelled_close,	"PUT Cn-Cls") \
 	EM(netfs_trace_tcon_ref_put_cancelled_close_fid, "PUT Cn-Fid") \
 	EM(netfs_trace_tcon_ref_put_cancelled_mid,	"PUT Cn-Mid") \
+	EM(netfs_trace_tcon_ref_put_close_defer_files,	"PUT Cl-Def") \
 	EM(netfs_trace_tcon_ref_put_mnt_ctx,		"PUT MntCtx") \
 	EM(netfs_trace_tcon_ref_put_dfs_refer,		"PUT DfsRfr") \
 	EM(netfs_trace_tcon_ref_put_reconnect_server,	"PUT Reconn") \
-- 
2.43.0

Re: [PATCH] cifs: open files should not hold ref on superblock

[Steve French]

merged into cifs-2.6.git for-next pending more review and testing

On Wed, Mar 4, 2026 at 6:46 AM <nspmangalore@gmail.com> wrote:
>
> From: Shyam Prasad N <sprasad@microsoft.com>
>
> Today whenever we deal with a file, in addition to holding
> a reference on the dentry, we also get a reference on the
> superblock. This happens in two cases:
> 1. when a new cinode is allocated
> 2. when an oplock break is being processed
>
> The reasoning for holding the superblock ref was to make sure
> that when umount happens, if there are users of inodes and
> dentries, it does not try to clean them up and wait for the
> last ref to superblock to be dropped by last of such users.
>
> But the side effect of doing that is that umount silently drops
> a ref on the superblock and we could have deferred closes and
> lease breaks still holding these refs.
>
> Ideally, we should ensure that all of these users of inodes and
> dentries are cleaned up at the time of umount, which is what this
> code is doing.
>
> This code change allows these code paths to use a ref on the
> dentry (and hence the inode). That way, umount is
> ensured to clean up SMB client resources when it's the last
> ref on the superblock (For ex: when same objects are shared).
>
> The code change also moves the call to close all the files in
> deferred close list to the umount code path. It also waits for
> oplock_break workers to be flushed before calling
> kill_anon_super (which eventually frees up those objects).
>
> Fixes: 24261fc23db9 ("cifs: delay super block destruction until all cifsFileInfo objects are gone")
> Fixes: 705c79101ccf ("smb: client: fix use-after-free in cifs_oplock_break")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/smb/client/cifsfs.c    |  7 +++++--
>  fs/smb/client/cifsproto.h |  1 +
>  fs/smb/client/file.c      | 11 ----------
>  fs/smb/client/misc.c      | 42 +++++++++++++++++++++++++++++++++++++++
>  fs/smb/client/trace.h     |  2 ++
>  5 files changed, 50 insertions(+), 13 deletions(-)
>
> diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> index 99b04234a08e6..fcc56481d6cf2 100644
> --- a/fs/smb/client/cifsfs.c
> +++ b/fs/smb/client/cifsfs.c
> @@ -330,10 +330,14 @@ static void cifs_kill_sb(struct super_block *sb)
>
>         /*
>          * We need to release all dentries for the cached directories
> -        * before we kill the sb.
> +        * and close all deferred file handles before we kill the sb.
>          */
>         if (cifs_sb->root) {
>                 close_all_cached_dirs(cifs_sb);
> +               cifs_close_all_deferred_files_sb(cifs_sb);
> +
> +               /* Wait for all pending oplock breaks to complete */
> +               flush_workqueue(cifsoplockd_wq);
>
>                 /* finally release root dentry */
>                 dput(cifs_sb->root);
> @@ -864,7 +868,6 @@ static void cifs_umount_begin(struct super_block *sb)
>         spin_unlock(&tcon->tc_lock);
>         spin_unlock(&cifs_tcp_ses_lock);
>
> -       cifs_close_all_deferred_files(tcon);
>         /* cancel_brl_requests(tcon); */ /* BB mark all brl mids as exiting */
>         /* cancel_notify_requests(tcon); */
>         if (tcon->ses && tcon->ses->server) {
> diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> index 96d6b5325aa33..800a7e418c326 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -261,6 +261,7 @@ void cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode);
>
>  void cifs_close_all_deferred_files(struct cifs_tcon *tcon);
>
> +void cifs_close_all_deferred_files_sb(struct cifs_sb_info *cifs_sb);
>  void cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon,
>                                            struct dentry *dentry);
>
> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> index 18f31d4eb98de..fb4f9aafe1386 100644
> --- a/fs/smb/client/file.c
> +++ b/fs/smb/client/file.c
> @@ -711,8 +711,6 @@ struct cifsFileInfo *cifs_new_fileinfo(struct cifs_fid *fid, struct file *file,
>         mutex_init(&cfile->fh_mutex);
>         spin_lock_init(&cfile->file_info_lock);
>
> -       cifs_sb_active(inode->i_sb);
> -
>         /*
>          * If the server returned a read oplock and we have mandatory brlocks,
>          * set oplock level to None.
> @@ -767,7 +765,6 @@ static void cifsFileInfo_put_final(struct cifsFileInfo *cifs_file)
>         struct inode *inode = d_inode(cifs_file->dentry);
>         struct cifsInodeInfo *cifsi = CIFS_I(inode);
>         struct cifsLockInfo *li, *tmp;
> -       struct super_block *sb = inode->i_sb;
>
>         /*
>          * Delete any outstanding lock records. We'll lose them when the file
> @@ -785,7 +782,6 @@ static void cifsFileInfo_put_final(struct cifsFileInfo *cifs_file)
>
>         cifs_put_tlink(cifs_file->tlink);
>         dput(cifs_file->dentry);
> -       cifs_sb_deactive(sb);
>         kfree(cifs_file->symlink_target);
>         kfree(cifs_file);
>  }
> @@ -3165,12 +3161,6 @@ void cifs_oplock_break(struct work_struct *work)
>         __u64 persistent_fid, volatile_fid;
>         __u16 net_fid;
>
> -       /*
> -        * Hold a reference to the superblock to prevent it and its inodes from
> -        * being freed while we are accessing cinode. Otherwise, _cifsFileInfo_put()
> -        * may release the last reference to the sb and trigger inode eviction.
> -        */
> -       cifs_sb_active(sb);
>         wait_on_bit(&cinode->flags, CIFS_INODE_PENDING_WRITERS,
>                         TASK_UNINTERRUPTIBLE);
>
> @@ -3255,7 +3245,6 @@ void cifs_oplock_break(struct work_struct *work)
>         cifs_put_tlink(tlink);
>  out:
>         cifs_done_oplock_break(cinode);
> -       cifs_sb_deactive(sb);
>  }
>
>  static int cifs_swap_activate(struct swap_info_struct *sis,
> diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
> index 22cde46309fe0..318533210648d 100644
> --- a/fs/smb/client/misc.c
> +++ b/fs/smb/client/misc.c
> @@ -28,6 +28,11 @@
>  #include "fs_context.h"
>  #include "cached_dir.h"
>
> +struct tcon_list {
> +       struct list_head entry;
> +       struct cifs_tcon *tcon;
> +};
> +
>  /* The xid serves as a useful identifier for each incoming vfs request,
>     in a similar way to the mid which is useful to track each sent smb,
>     and CurrentXid can also provide a running counter (although it
> @@ -550,6 +555,43 @@ cifs_close_all_deferred_files(struct cifs_tcon *tcon)
>         }
>  }
>
> +void cifs_close_all_deferred_files_sb(struct cifs_sb_info *cifs_sb)
> +{
> +       struct rb_root *root = &cifs_sb->tlink_tree;
> +       struct rb_node *node;
> +       struct cifs_tcon *tcon;
> +       struct tcon_link *tlink;
> +       struct tcon_list *tmp_list, *q;
> +       LIST_HEAD(tcon_head);
> +
> +       spin_lock(&cifs_sb->tlink_tree_lock);
> +       for (node = rb_first(root); node; node = rb_next(node)) {
> +               tlink = rb_entry(node, struct tcon_link, tl_rbnode);
> +               tcon = tlink_tcon(tlink);
> +               if (IS_ERR(tcon))
> +                       continue;
> +               tmp_list = kmalloc_obj(struct tcon_list, GFP_ATOMIC);
> +               if (tmp_list == NULL)
> +                       break;
> +               tmp_list->tcon = tcon;
> +               /* Take a reference on tcon to prevent it from being freed */
> +               spin_lock(&tcon->tc_lock);
> +               ++tcon->tc_count;
> +               trace_smb3_tcon_ref(tcon->debug_id, tcon->tc_count,
> +                                   netfs_trace_tcon_ref_get_close_defer_files);
> +               spin_unlock(&tcon->tc_lock);
> +               list_add_tail(&tmp_list->entry, &tcon_head);
> +       }
> +       spin_unlock(&cifs_sb->tlink_tree_lock);
> +
> +       list_for_each_entry_safe(tmp_list, q, &tcon_head, entry) {
> +               cifs_close_all_deferred_files(tmp_list->tcon);
> +               list_del(&tmp_list->entry);
> +               cifs_put_tcon(tmp_list->tcon, netfs_trace_tcon_ref_put_close_defer_files);
> +               kfree(tmp_list);
> +       }
> +}
> +
>  void cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon,
>                                            struct dentry *dentry)
>  {
> diff --git a/fs/smb/client/trace.h b/fs/smb/client/trace.h
> index 9228f95cae2bd..acfbb63086ea2 100644
> --- a/fs/smb/client/trace.h
> +++ b/fs/smb/client/trace.h
> @@ -176,6 +176,7 @@
>         EM(netfs_trace_tcon_ref_get_cached_laundromat,  "GET Ch-Lau") \
>         EM(netfs_trace_tcon_ref_get_cached_lease_break, "GET Ch-Lea") \
>         EM(netfs_trace_tcon_ref_get_cancelled_close,    "GET Cn-Cls") \
> +       EM(netfs_trace_tcon_ref_get_close_defer_files,  "GET Cl-Def") \
>         EM(netfs_trace_tcon_ref_get_dfs_refer,          "GET DfsRef") \
>         EM(netfs_trace_tcon_ref_get_find,               "GET Find  ") \
>         EM(netfs_trace_tcon_ref_get_find_sess_tcon,     "GET FndSes") \
> @@ -187,6 +188,7 @@
>         EM(netfs_trace_tcon_ref_put_cancelled_close,    "PUT Cn-Cls") \
>         EM(netfs_trace_tcon_ref_put_cancelled_close_fid, "PUT Cn-Fid") \
>         EM(netfs_trace_tcon_ref_put_cancelled_mid,      "PUT Cn-Mid") \
> +       EM(netfs_trace_tcon_ref_put_close_defer_files,  "PUT Cl-Def") \
>         EM(netfs_trace_tcon_ref_put_mnt_ctx,            "PUT MntCtx") \
>         EM(netfs_trace_tcon_ref_put_dfs_refer,          "PUT DfsRfr") \
>         EM(netfs_trace_tcon_ref_put_reconnect_server,   "PUT Reconn") \
> --
> 2.43.0
>


-- 
Thanks,

Steve

Re: [PATCH] cifs: open files should not hold ref on superblock

[Henrique Carvalho]

On Wed, Mar 04, 2026 at 06:15:53PM +0530, nspmangalore@gmail.com wrote:
> From: Shyam Prasad N <sprasad@microsoft.com>
> 
> Today whenever we deal with a file, in addition to holding
> a reference on the dentry, we also get a reference on the
> superblock. This happens in two cases:
> 1. when a new cinode is allocated
> 2. when an oplock break is being processed
> 
> The reasoning for holding the superblock ref was to make sure
> that when umount happens, if there are users of inodes and
> dentries, it does not try to clean them up and wait for the
> last ref to superblock to be dropped by last of such users.
> 
> But the side effect of doing that is that umount silently drops
> a ref on the superblock and we could have deferred closes and
> lease breaks still holding these refs.
> 
> Ideally, we should ensure that all of these users of inodes and
> dentries are cleaned up at the time of umount, which is what this
> code is doing.
> 
> This code change allows these code paths to use a ref on the
> dentry (and hence the inode). That way, umount is
> ensured to clean up SMB client resources when it's the last
> ref on the superblock (For ex: when same objects are shared).
> 
> The code change also moves the call to close all the files in
> deferred close list to the umount code path. It also waits for
> oplock_break workers to be flushed before calling
> kill_anon_super (which eventually frees up those objects).
> 
> Fixes: 24261fc23db9 ("cifs: delay super block destruction until all cifsFileInfo objects are gone")
> Fixes: 705c79101ccf ("smb: client: fix use-after-free in cifs_oplock_break")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---

Hi Shyam,

So the side effect of the previous code is that the umount hangs until
all the files are closed?

Thanks,

-- 
Henrique
SUSE Labs

Re: [PATCH] cifs: open files should not hold ref on superblock

[Shyam Prasad N]

On Fri, Mar 13, 2026 at 1:28 AM Henrique Carvalho
<henrique.carvalho@suse.com> wrote:
>
> On Wed, Mar 04, 2026 at 06:15:53PM +0530, nspmangalore@gmail.com wrote:
> > From: Shyam Prasad N <sprasad@microsoft.com>
> >
> > Today whenever we deal with a file, in addition to holding
> > a reference on the dentry, we also get a reference on the
> > superblock. This happens in two cases:
> > 1. when a new cinode is allocated
> > 2. when an oplock break is being processed
> >
> > The reasoning for holding the superblock ref was to make sure
> > that when umount happens, if there are users of inodes and
> > dentries, it does not try to clean them up and wait for the
> > last ref to superblock to be dropped by last of such users.
> >
> > But the side effect of doing that is that umount silently drops
> > a ref on the superblock and we could have deferred closes and
> > lease breaks still holding these refs.
> >
> > Ideally, we should ensure that all of these users of inodes and
> > dentries are cleaned up at the time of umount, which is what this
> > code is doing.
> >
> > This code change allows these code paths to use a ref on the
> > dentry (and hence the inode). That way, umount is
> > ensured to clean up SMB client resources when it's the last
> > ref on the superblock (For ex: when same objects are shared).
> >
> > The code change also moves the call to close all the files in
> > deferred close list to the umount code path. It also waits for
> > oplock_break workers to be flushed before calling
> > kill_anon_super (which eventually frees up those objects).
> >
> > Fixes: 24261fc23db9 ("cifs: delay super block destruction until all cifsFileInfo objects are gone")
> > Fixes: 705c79101ccf ("smb: client: fix use-after-free in cifs_oplock_break")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > ---
>
> Hi Shyam,
>
> So the side effect of the previous code is that the umount hangs until
> all the files are closed?

Hi Henrique
Umount works. All it does is decrement refcount on sb.
When the last file is closed (or when the last cifs_oplock_break
processing completes) that's when cifs_kill_sb would get called.
Before that if there's another mount of the same share, it will reuse
the same session, tcon and open handles. As a result, an attempt to
delete files on the mount point may fail (which is one of first things
done by many xfstests).

>
> Thanks,
>
> --
> Henrique
> SUSE Labs



-- 
Regards,
Shyam

Re: [PATCH] cifs: open files should not hold ref on superblock

[Henrique Carvalho]

On Fri, Mar 13, 2026 at 10:57:42AM +0530, Shyam Prasad N wrote:
> On Fri, Mar 13, 2026 at 1:28 AM Henrique Carvalho
> <henrique.carvalho@suse.com> wrote:
> >
> > On Wed, Mar 04, 2026 at 06:15:53PM +0530, nspmangalore@gmail.com wrote:
> > > From: Shyam Prasad N <sprasad@microsoft.com>
> > >
> > > Today whenever we deal with a file, in addition to holding
> > > a reference on the dentry, we also get a reference on the
> > > superblock. This happens in two cases:
> > > 1. when a new cinode is allocated
> > > 2. when an oplock break is being processed
> > >
> > > The reasoning for holding the superblock ref was to make sure
> > > that when umount happens, if there are users of inodes and
> > > dentries, it does not try to clean them up and wait for the
> > > last ref to superblock to be dropped by last of such users.
> > >
> > > But the side effect of doing that is that umount silently drops
> > > a ref on the superblock and we could have deferred closes and
> > > lease breaks still holding these refs.
> > >
> > > Ideally, we should ensure that all of these users of inodes and
> > > dentries are cleaned up at the time of umount, which is what this
> > > code is doing.
> > >
> > > This code change allows these code paths to use a ref on the
> > > dentry (and hence the inode). That way, umount is
> > > ensured to clean up SMB client resources when it's the last
> > > ref on the superblock (For ex: when same objects are shared).
> > >
> > > The code change also moves the call to close all the files in
> > > deferred close list to the umount code path. It also waits for
> > > oplock_break workers to be flushed before calling
> > > kill_anon_super (which eventually frees up those objects).
> > >
> > > Fixes: 24261fc23db9 ("cifs: delay super block destruction until all cifsFileInfo objects are gone")
> > > Fixes: 705c79101ccf ("smb: client: fix use-after-free in cifs_oplock_break")
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > > ---
> >
> > Hi Shyam,
> >
> > So the side effect of the previous code is that the umount hangs until
> > all the files are closed?
> 
> Hi Henrique
> Umount works. All it does is decrement refcount on sb.
> When the last file is closed (or when the last cifs_oplock_break
> processing completes) that's when cifs_kill_sb would get called.
> Before that if there's another mount of the same share, it will reuse
> the same session, tcon and open handles. As a result, an attempt to
> delete files on the mount point may fail (which is one of first things
> done by many xfstests).
>

Thank you for the explanation.

I will wait for your v2.

Re: [PATCH] cifs: open files should not hold ref on superblock

[Shyam Prasad N]

On Sat, Mar 14, 2026 at 1:47 AM Henrique Carvalho
<henrique.carvalho@suse.com> wrote:
>
> On Fri, Mar 13, 2026 at 10:57:42AM +0530, Shyam Prasad N wrote:
> > On Fri, Mar 13, 2026 at 1:28 AM Henrique Carvalho
> > <henrique.carvalho@suse.com> wrote:
> > >
> > > On Wed, Mar 04, 2026 at 06:15:53PM +0530, nspmangalore@gmail.com wrote:
> > > > From: Shyam Prasad N <sprasad@microsoft.com>
> > > >
> > > > Today whenever we deal with a file, in addition to holding
> > > > a reference on the dentry, we also get a reference on the
> > > > superblock. This happens in two cases:
> > > > 1. when a new cinode is allocated
> > > > 2. when an oplock break is being processed
> > > >
> > > > The reasoning for holding the superblock ref was to make sure
> > > > that when umount happens, if there are users of inodes and
> > > > dentries, it does not try to clean them up and wait for the
> > > > last ref to superblock to be dropped by last of such users.
> > > >
> > > > But the side effect of doing that is that umount silently drops
> > > > a ref on the superblock and we could have deferred closes and
> > > > lease breaks still holding these refs.
> > > >
> > > > Ideally, we should ensure that all of these users of inodes and
> > > > dentries are cleaned up at the time of umount, which is what this
> > > > code is doing.
> > > >
> > > > This code change allows these code paths to use a ref on the
> > > > dentry (and hence the inode). That way, umount is
> > > > ensured to clean up SMB client resources when it's the last
> > > > ref on the superblock (For ex: when same objects are shared).
> > > >
> > > > The code change also moves the call to close all the files in
> > > > deferred close list to the umount code path. It also waits for
> > > > oplock_break workers to be flushed before calling
> > > > kill_anon_super (which eventually frees up those objects).
> > > >
> > > > Fixes: 24261fc23db9 ("cifs: delay super block destruction until all cifsFileInfo objects are gone")
> > > > Fixes: 705c79101ccf ("smb: client: fix use-after-free in cifs_oplock_break")
> > > > Cc: <stable@vger.kernel.org>
> > > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > > > ---
> > >
> > > Hi Shyam,
> > >
> > > So the side effect of the previous code is that the umount hangs until
> > > all the files are closed?
> >
> > Hi Henrique
> > Umount works. All it does is decrement refcount on sb.
> > When the last file is closed (or when the last cifs_oplock_break
> > processing completes) that's when cifs_kill_sb would get called.
> > Before that if there's another mount of the same share, it will reuse
> > the same session, tcon and open handles. As a result, an attempt to
> > delete files on the mount point may fail (which is one of first things
> > done by many xfstests).
> >
>
> Thank you for the explanation.
>
> I will wait for your v2.

Hi Steve,

I ran generic/694 to understand why it is failing with this change.
I think that this fix has just exposed a problem rather than caused it.

The test does the following:
1. either fallocates a file to 4G or pwrites to it
2. calls sync
3. runs stat to get number of blocks allocated for the file
4. umounts the share
5. mounts the share again
6. runs stat to get number of blocks allocated for the file
7. compares output of steps 3 and 6

Without this change, both step 3 and 6 would return 0, since even
through umount/mount, the same file would remain open (since
superblocks will be shared).
With this change, step 3 would return 0. Step 6 would return the right value.

If you use nosharesock even after reverting this change, you'll see
the test failing.
Or even with this change if actimeo=0, then this test passes.

The real question to ask is why aren't we updating i_blocks even after
sync succeeds?
My guess is that this has something to do with attribute caching when
the handle is kept open.

-- 
Regards,
Shyam

Re: [PATCH] cifs: open files should not hold ref on superblock

[Steve French]

On Sat, Mar 14, 2026 at 3:38 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> On Sat, Mar 14, 2026 at 1:47 AM Henrique Carvalho
> <henrique.carvalho@suse.com> wrote:
> >
> > On Fri, Mar 13, 2026 at 10:57:42AM +0530, Shyam Prasad N wrote:
> > > On Fri, Mar 13, 2026 at 1:28 AM Henrique Carvalho
> > > <henrique.carvalho@suse.com> wrote:
> > > >
> > > > On Wed, Mar 04, 2026 at 06:15:53PM +0530, nspmangalore@gmail.com wrote:
> > > > > From: Shyam Prasad N <sprasad@microsoft.com>
> > > > >
> > > > > Today whenever we deal with a file, in addition to holding
> > > > > a reference on the dentry, we also get a reference on the
> > > > > superblock. This happens in two cases:
> > > > > 1. when a new cinode is allocated
> > > > > 2. when an oplock break is being processed
> > > > >
> > > > > The reasoning for holding the superblock ref was to make sure
> > > > > that when umount happens, if there are users of inodes and
> > > > > dentries, it does not try to clean them up and wait for the
> > > > > last ref to superblock to be dropped by last of such users.
> > > > >
> > > > > But the side effect of doing that is that umount silently drops
> > > > > a ref on the superblock and we could have deferred closes and
> > > > > lease breaks still holding these refs.
> > > > >
> > > > > Ideally, we should ensure that all of these users of inodes and
> > > > > dentries are cleaned up at the time of umount, which is what this
> > > > > code is doing.
> > > > >
> > > > > This code change allows these code paths to use a ref on the
> > > > > dentry (and hence the inode). That way, umount is
> > > > > ensured to clean up SMB client resources when it's the last
> > > > > ref on the superblock (For ex: when same objects are shared).
> > > > >
> > > > > The code change also moves the call to close all the files in
> > > > > deferred close list to the umount code path. It also waits for
> > > > > oplock_break workers to be flushed before calling
> > > > > kill_anon_super (which eventually frees up those objects).
> > > > >
> > > > > Fixes: 24261fc23db9 ("cifs: delay super block destruction until all cifsFileInfo objects are gone")
> > > > > Fixes: 705c79101ccf ("smb: client: fix use-after-free in cifs_oplock_break")
> > > > > Cc: <stable@vger.kernel.org>
> > > > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > > > > ---
> > > >
> > > > Hi Shyam,
> > > >
> > > > So the side effect of the previous code is that the umount hangs until
> > > > all the files are closed?
> > >
> > > Hi Henrique
> > > Umount works. All it does is decrement refcount on sb.
> > > When the last file is closed (or when the last cifs_oplock_break
> > > processing completes) that's when cifs_kill_sb would get called.
> > > Before that if there's another mount of the same share, it will reuse
> > > the same session, tcon and open handles. As a result, an attempt to
> > > delete files on the mount point may fail (which is one of first things
> > > done by many xfstests).
> > >
> >
> > Thank you for the explanation.
> >
> > I will wait for your v2.
>
> Hi Steve,
>
> I ran generic/694 to understand why it is failing with this change.
> I think that this fix has just exposed a problem rather than caused it.
>
> The test does the following:
> 1. either fallocates a file to 4G or pwrites to it
> 2. calls sync
> 3. runs stat to get number of blocks allocated for the file
> 4. umounts the share
> 5. mounts the share again
> 6. runs stat to get number of blocks allocated for the file
> 7. compares output of steps 3 and 6

Any chance of creating a small repro script for this that is easier to
debug han the xfstest was?

> Without this change, both step 3 and 6 would return 0, since even
> through umount/mount, the same file would remain open (since
> superblocks will be shared).
> With this change, step 3 would return 0. Step 6 would return the right value.
>
> If you use nosharesock even after reverting this change, you'll see
> the test failing.
> Or even with this change if actimeo=0, then this test passes.
>
> The real question to ask is why aren't we updating i_blocks even after
> sync succeeds?
> My guess is that this has something to do with attribute caching when
> the handle is kept open.

That sounds an important bug to fix.  Glad this test showed it.



-- 
Thanks,

Steve