smb: client: pin DFS superblock in iterator callback

[PATCH] smb: client: pin DFS superblock in iterator callback

Posted by Karl Mehltretter 3 weeks, 2 days ago

tcon_super_cb() stores a raw superblock pointer, but __cifs_get_super()
takes its active reference only after iterate_supers_type() has dropped
s_umount and its passive reference. Concurrent DFS automount expiry can
therefore free the superblock before cifs_sb_active() uses it.

A deterministic KASAN test reproduces the race as:

  BUG: KASAN: slab-use-after-free in cifs_sb_active+0x77/0x80

The same test passes with this change applied.

Take the active reference in the callback while iterate_supers_type()
still holds s_umount shared. cifs_put_tcp_super() remains the matching
release.

Fixes: bacd704a95ad ("cifs: handle prefix paths in reconnect")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
Testing: x86_64 QEMU KASAN A/B with deterministic KUnit synchronization
around the lifetime gap and real VFS superblock allocation/teardown.
Baseline reports a slab-use-after-free in cifs_sb_active(); fixed passes.

 fs/smb/client/misc.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
index 46e1382e8e04b..d4db3f91a91fa 100644
--- a/fs/smb/client/misc.c
+++ b/fs/smb/client/misc.c
@@ -891,8 +891,14 @@ static void tcon_super_cb(struct super_block *sb, void *arg)
 	     t1->ses->dfs_root_ses == t2->ses->dfs_root_ses) &&
 	    t1->ses->server == t2->ses->server &&
 	    t2->origin_fullpath &&
-	    dfs_src_pathname_equal(t2->origin_fullpath, t1->origin_fullpath))
+	    dfs_src_pathname_equal(t2->origin_fullpath, t1->origin_fullpath)) {
+		/*
+		 * Take the active reference while iterate_supers_type() still
+		 * holds s_umount shared.
+		 */
+		cifs_sb_active(sb);
 		sd->sb = sb;
+	}
 	spin_unlock(&t2->tc_lock);
 }
 
@@ -909,15 +915,8 @@ static struct super_block *__cifs_get_super(void (*f)(struct super_block *, void
 
 	for (; *fs_type; fs_type++) {
 		iterate_supers_type(*fs_type, f, &sd);
-		if (sd.sb) {
-			/*
-			 * Grab an active reference in order to prevent automounts (DFS links)
-			 * of expiring and then freeing up our cifs superblock pointer while
-			 * we're doing failover.
-			 */
-			cifs_sb_active(sd.sb);
+		if (sd.sb)
 			return sd.sb;
-		}
 	}
 	pr_warn_once("%s: could not find dfs superblock\n", __func__);
 	return ERR_PTR(-EINVAL);
-- 
2.53.0

Re: [PATCH] smb: client: pin DFS superblock in iterator callback

Posted by Paulo Alcantara 3 weeks, 1 day ago

Karl Mehltretter <kmehltretter@gmail.com> writes:

> tcon_super_cb() stores a raw superblock pointer, but __cifs_get_super()
> takes its active reference only after iterate_supers_type() has dropped
> s_umount and its passive reference. Concurrent DFS automount expiry can
> therefore free the superblock before cifs_sb_active() uses it.
>
> A deterministic KASAN test reproduces the race as:
>
>   BUG: KASAN: slab-use-after-free in cifs_sb_active+0x77/0x80
>
> The same test passes with this change applied.
>
> Take the active reference in the callback while iterate_supers_type()
> still holds s_umount shared. cifs_put_tcp_super() remains the matching
> release.
> ...

Applied.

Re: [PATCH] smb: client: pin DFS superblock in iterator callback

Posted by Shyam Prasad N 2 weeks, 5 days ago

On Thu, Sep 3, 2026 at 10:27 PM Paulo Alcantara <pc@manguebit.org> wrote:
>
> Karl Mehltretter <kmehltretter@gmail.com> writes:
>
> > tcon_super_cb() stores a raw superblock pointer, but __cifs_get_super()
> > takes its active reference only after iterate_supers_type() has dropped
> > s_umount and its passive reference. Concurrent DFS automount expiry can
> > therefore free the superblock before cifs_sb_active() uses it.
> >
> > A deterministic KASAN test reproduces the race as:
> >
> >   BUG: KASAN: slab-use-after-free in cifs_sb_active+0x77/0x80
> >
> > The same test passes with this change applied.
> >
> > Take the active reference in the callback while iterate_supers_type()
> > still holds s_umount shared. cifs_put_tcp_super() remains the matching
> > release.
> > ...
>
> Applied.
>

I'm not sure that taking a ref on the superblock is a good idea.
Can we find a way to do this in a synchronous way in cifs_sb_kill?

-- 
Regards,
Shyam