| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAH2r5msNho7x0aQ9XZ=ra8hw=z4P-U74iS6SzcL+pDbp5R21UQ@mail.gmail.com>
Date: Thu, 21 Aug 2025 11:12:23 -0500
From: Steve French <smfrench@...il.com>
To: Wang Zhaolong <wangzhaolong@...weicloud.com>
Cc: pc@...guebit.org, linux-cifs@...r.kernel.org,
samba-technical@...ts.samba.org, linux-kernel@...r.kernel.org,
chengzhihao1@...wei.com, yi.zhang@...wei.com, yangerkun@...wei.com
Subject: Re: [PATCH V5] smb: client: Fix mount deadlock by avoiding super
block iteration in DFS reconnect
Does this patch deal with the regression with multiuser mounts that
Paulo mentioned in the earlier email?
On Wed, Aug 20, 2025 at 6:43 AM Wang Zhaolong
<wangzhaolong@...weicloud.com> wrote:
>
> An AA deadlock occurs when network interruption during mount triggers
> DFS reconnection logic that calls iterate_supers_type().
>
> The detailed call process is as follows:
>
> mount.cifs
> -------------------------
> path_mount
> do_new_mount
> vfs_get_tree
> smb3_get_tree
> cifs_smb3_do_mount
> sget
> alloc_super
> down_write_nested(&s->s_umount, ..); // Hold lock
> cifs_root_iget
> cifs_get_inode_info
> smb2_query_path_info
> smb2_compound_op
> SMB2_open_init
> smb2_plain_req_init
> smb2_reconnect // Trigger reconnection
> cifs_tree_connect
> cifs_get_dfs_tcon_super
> __cifs_get_super
> iterate_supers_type
> down_read(&sb->s_umount); // Deadlock
> do_new_mount_fc
> up_write(&sb->s_umount); // Release lock
>
> During mount phase, if reconnection is triggered, the foreground mount
> process may enter smb2_reconnect prior to the reconnect worker being
> scheduled, leading to a deadlock when subsequent DFS tree connect
> attempts reacquire the s_umount lock.
>
> The issue stems from cifs_get_dfs_tcon_super() using iterate_supers_type()
> which reacquires the s_umount lock that's already held by the mount
> process.
>
> However, after commit a091d9711bde ("smb:client: smb: client: Add reverse
> mapping from tcon to superblocks"), we have a more direct way to access
> associated superblocks through tcon->cifs_sb_list, which was originally
> introduced to update I/O sizes after reconnection.
>
> This patch leverages the existing tcon->cifs_sb_list infrastructure to
> directly update DFS mount prepaths without needing to search through all
> superblocks.
>
> The key changes are:
> - Add update_tcon_super_prepaths() to update all related superblocks
> - Remove now-unused cifs_get_dfs_tcon_super() and related callback code
> - Update tree_connect_dfs_target() to use the new direct approach
>
> Fixes: 3ae872de4107 ("smb: client: fix shared DFS root mounts with different prefixes")
> Signed-off-by: Wang Zhaolong <wangzhaolong@...weicloud.com>
> ---
>
> V5:
> - Extract update logic into update_tcon_super_prepaths() function
> - Add error logging for prepath update failures
> - Leverage existing tcon->cifs_sb_list infrastructure instead of iterate_supers_type()
> - Remove now-unused cifs_get_dfs_tcon_super() and related callback code
>
> V4:
> - Perform a null pointer check on the return value of cifs_get_dfs_tcon_super()
> to prevent NULL ptr dereference with DFS multiuser mount
>
> V3:
> - Adjust the trace diagram for the super_lock_shared() section to align with
> the latest mainline call flow.
>
> V2:
> - Adjust the trace diagram in the commit message to indicate when the lock
> is released
>
> fs/smb/client/cifsproto.h | 2 --
> fs/smb/client/dfs.c | 32 +++++++++++------
> fs/smb/client/misc.c | 76 ---------------------------------------
> 3 files changed, 21 insertions(+), 89 deletions(-)
>
> diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> index c34c533b2efa..6b55582b427a 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -677,12 +677,10 @@ void extract_unc_hostname(const char *unc, const char **h, size_t *len);
> int copy_path_name(char *dst, const char *src);
> int smb2_parse_query_directory(struct cifs_tcon *tcon, struct kvec *rsp_iov,
> int resp_buftype,
> struct cifs_search_info *srch_inf);
>
> -struct super_block *cifs_get_dfs_tcon_super(struct cifs_tcon *tcon);
> -void cifs_put_tcp_super(struct super_block *sb);
> int cifs_update_super_prepath(struct cifs_sb_info *cifs_sb, char *prefix);
> char *extract_hostname(const char *unc);
> char *extract_sharename(const char *unc);
> int parse_reparse_point(struct reparse_data_buffer *buf,
> u32 plen, struct cifs_sb_info *cifs_sb,
> diff --git a/fs/smb/client/dfs.c b/fs/smb/client/dfs.c
> index f65a8a90ba27..cb0532e3868f 100644
> --- a/fs/smb/client/dfs.c
> +++ b/fs/smb/client/dfs.c
> @@ -331,13 +331,30 @@ static int target_share_matches_server(struct TCP_Server_Info *server, char *sha
> }
> cifs_server_unlock(server);
> return rc;
> }
>
> +static int update_tcon_super_prepaths(struct cifs_tcon *tcon, const char *prefix)
> +{
> + struct cifs_sb_info *cifs_sb;
> + int rc = 0;
> +
> + spin_lock(&tcon->sb_list_lock);
> + list_for_each_entry(cifs_sb, &tcon->cifs_sb_list, tcon_sb_link) {
> + rc = cifs_update_super_prepath(cifs_sb, (char *)prefix);
> + if (rc) {
> + cifs_dbg(VFS, "Failed to update prepath for superblock: %d\n", rc);
> + break;
> + }
> + }
> + spin_unlock(&tcon->sb_list_lock);
> +
> + return rc;
> +}
> +
> static int tree_connect_dfs_target(const unsigned int xid,
> struct cifs_tcon *tcon,
> - struct cifs_sb_info *cifs_sb,
> char *tree, bool islink,
> struct dfs_cache_tgt_list *tl)
> {
> const struct smb_version_operations *ops = tcon->ses->server->ops;
> struct TCP_Server_Info *server = tcon->ses->server;
> @@ -370,12 +387,12 @@ static int tree_connect_dfs_target(const unsigned int xid,
>
> dfs_cache_noreq_update_tgthint(server->leaf_fullpath + 1, tit);
> scnprintf(tree, MAX_TREE_SIZE, "\\%s", share);
> rc = ops->tree_connect(xid, tcon->ses, tree,
> tcon, tcon->ses->local_nls);
> - if (islink && !rc && cifs_sb)
> - rc = cifs_update_super_prepath(cifs_sb, prefix);
> + if (islink && !rc && READ_ONCE(tcon->origin_fullpath))
> + rc = update_tcon_super_prepaths(tcon, prefix);
> break;
> }
>
> kfree(share);
> kfree(prefix);
> @@ -387,12 +404,10 @@ int cifs_tree_connect(const unsigned int xid, struct cifs_tcon *tcon)
> {
> int rc;
> struct TCP_Server_Info *server = tcon->ses->server;
> const struct smb_version_operations *ops = server->ops;
> DFS_CACHE_TGT_LIST(tl);
> - struct cifs_sb_info *cifs_sb = NULL;
> - struct super_block *sb = NULL;
> struct dfs_info3_param ref = {0};
> char *tree;
>
> /* only send once per connect */
> spin_lock(&tcon->tc_lock);
> @@ -428,29 +443,24 @@ int cifs_tree_connect(const unsigned int xid, struct cifs_tcon *tcon)
> rc = ops->tree_connect(xid, tcon->ses, tree,
> tcon, tcon->ses->local_nls);
> goto out;
> }
>
> - sb = cifs_get_dfs_tcon_super(tcon);
> - if (!IS_ERR(sb))
> - cifs_sb = CIFS_SB(sb);
> -
> /* Tree connect to last share in @tcon->tree_name if no DFS referral */
> if (!server->leaf_fullpath ||
> dfs_cache_noreq_find(server->leaf_fullpath + 1, &ref, &tl)) {
> rc = ops->tree_connect(xid, tcon->ses, tcon->tree_name,
> tcon, tcon->ses->local_nls);
> goto out;
> }
>
> - rc = tree_connect_dfs_target(xid, tcon, cifs_sb, tree, ref.server_type == DFS_TYPE_LINK,
> + rc = tree_connect_dfs_target(xid, tcon, tree, ref.server_type == DFS_TYPE_LINK,
> &tl);
> free_dfs_info_param(&ref);
>
> out:
> kfree(tree);
> - cifs_put_tcp_super(sb);
>
> if (rc) {
> spin_lock(&tcon->tc_lock);
> if (tcon->status == TID_IN_TCON)
> tcon->status = TID_NEED_TCON;
> diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
> index da23cc12a52c..3eedcca0d7f9 100644
> --- a/fs/smb/client/misc.c
> +++ b/fs/smb/client/misc.c
> @@ -1108,86 +1108,10 @@ int copy_path_name(char *dst, const char *src)
> /* we count the trailing nul */
> name_len++;
> return name_len;
> }
>
> -struct super_cb_data {
> - void *data;
> - struct super_block *sb;
> -};
> -
> -static void tcon_super_cb(struct super_block *sb, void *arg)
> -{
> - struct super_cb_data *sd = arg;
> - struct cifs_sb_info *cifs_sb;
> - struct cifs_tcon *t1 = sd->data, *t2;
> -
> - if (sd->sb)
> - return;
> -
> - cifs_sb = CIFS_SB(sb);
> - t2 = cifs_sb_master_tcon(cifs_sb);
> -
> - spin_lock(&t2->tc_lock);
> - if ((t1->ses == t2->ses ||
> - 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))
> - sd->sb = sb;
> - spin_unlock(&t2->tc_lock);
> -}
> -
> -static struct super_block *__cifs_get_super(void (*f)(struct super_block *, void *),
> - void *data)
> -{
> - struct super_cb_data sd = {
> - .data = data,
> - .sb = NULL,
> - };
> - struct file_system_type **fs_type = (struct file_system_type *[]) {
> - &cifs_fs_type, &smb3_fs_type, NULL,
> - };
> -
> - 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);
> - return sd.sb;
> - }
> - }
> - pr_warn_once("%s: could not find dfs superblock\n", __func__);
> - return ERR_PTR(-EINVAL);
> -}
> -
> -static void __cifs_put_super(struct super_block *sb)
> -{
> - if (!IS_ERR_OR_NULL(sb))
> - cifs_sb_deactive(sb);
> -}
> -
> -struct super_block *cifs_get_dfs_tcon_super(struct cifs_tcon *tcon)
> -{
> - spin_lock(&tcon->tc_lock);
> - if (!tcon->origin_fullpath) {
> - spin_unlock(&tcon->tc_lock);
> - return ERR_PTR(-ENOENT);
> - }
> - spin_unlock(&tcon->tc_lock);
> - return __cifs_get_super(tcon_super_cb, tcon);
> -}
> -
> -void cifs_put_tcp_super(struct super_block *sb)
> -{
> - __cifs_put_super(sb);
> -}
> -
> #ifdef CONFIG_CIFS_DFS_UPCALL
> int match_target_ip(struct TCP_Server_Info *server,
> const char *host, size_t hostlen,
> bool *result)
> {
> --
> 2.39.2
>
>
--
Thanks,
Steve
Powered by blists - more mailing lists