[PATCH] cifs: all initializations for tcon should happen in tcon_info_alloc

[PATCH] cifs: all initializations for tcon should happen in tcon_info_alloc

[nspmangalore]

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

Today, a few work structs inside tcon are initialized inside
cifs_get_tcon and not in tcon_info_alloc. As a result, if a tcon
is obtained from tcon_info_alloc, but not called as a part of
cifs_get_tcon, we may trip over.

Cc: <stable@vger.kernel.org>
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/cifsproto.h | 1 +
 fs/smb/client/connect.c   | 8 +-------
 fs/smb/client/misc.c      | 6 ++++++
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index 66093fa78aed..045227ed4efc 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -136,6 +136,7 @@ extern int SendReceiveBlockingLock(const unsigned int xid,
 			struct smb_hdr *out_buf,
 			int *bytes_returned);
 
+void smb2_query_server_interfaces(struct work_struct *work);
 void
 cifs_signal_cifsd_for_reconnect(struct TCP_Server_Info *server,
 				      bool all_channels);
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index c48869c29e15..16c4f7fa1f34 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -97,7 +97,7 @@ static int reconn_set_ipaddr_from_hostname(struct TCP_Server_Info *server)
 	return rc;
 }
 
-static void smb2_query_server_interfaces(struct work_struct *work)
+void smb2_query_server_interfaces(struct work_struct *work)
 {
 	int rc;
 	int xid;
@@ -2866,20 +2866,14 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb3_fs_context *ctx)
 	tcon->max_cached_dirs = ctx->max_cached_dirs;
 	tcon->nodelete = ctx->nodelete;
 	tcon->local_lease = ctx->local_lease;
-	INIT_LIST_HEAD(&tcon->pending_opens);
 	tcon->status = TID_GOOD;
 
-	INIT_DELAYED_WORK(&tcon->query_interfaces,
-			  smb2_query_server_interfaces);
 	if (ses->server->dialect >= SMB30_PROT_ID &&
 	    (ses->server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
 		/* schedule query interfaces poll */
 		queue_delayed_work(cifsiod_wq, &tcon->query_interfaces,
 				   (SMB_INTERFACE_POLL_INTERVAL * HZ));
 	}
-#ifdef CONFIG_CIFS_DFS_UPCALL
-	INIT_DELAYED_WORK(&tcon->dfs_cache_work, dfs_cache_refresh);
-#endif
 	spin_lock(&cifs_tcp_ses_lock);
 	list_add(&tcon->tcon_list, &ses->tcon_list);
 	spin_unlock(&cifs_tcp_ses_lock);
diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
index e77017f47084..da23cc12a52c 100644
--- a/fs/smb/client/misc.c
+++ b/fs/smb/client/misc.c
@@ -151,6 +151,12 @@ tcon_info_alloc(bool dir_leases_enabled, enum smb3_tcon_ref_trace trace)
 #ifdef CONFIG_CIFS_DFS_UPCALL
 	INIT_LIST_HEAD(&ret_buf->dfs_ses_list);
 #endif
+	INIT_LIST_HEAD(&ret_buf->pending_opens);
+	INIT_DELAYED_WORK(&ret_buf->query_interfaces,
+			  smb2_query_server_interfaces);
+#ifdef CONFIG_CIFS_DFS_UPCALL
+	INIT_DELAYED_WORK(&ret_buf->dfs_cache_work, dfs_cache_refresh);
+#endif
 
 	return ret_buf;
 }
-- 
2.43.0

Re: [PATCH] cifs: all initializations for tcon should happen in tcon_info_alloc

[Steve French]

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

On Mon, Jun 30, 2025 at 12:40 PM <nspmangalore@gmail.com> wrote:
>
> From: Shyam Prasad N <sprasad@microsoft.com>
>
> Today, a few work structs inside tcon are initialized inside
> cifs_get_tcon and not in tcon_info_alloc. As a result, if a tcon
> is obtained from tcon_info_alloc, but not called as a part of
> cifs_get_tcon, we may trip over.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/smb/client/cifsproto.h | 1 +
>  fs/smb/client/connect.c   | 8 +-------
>  fs/smb/client/misc.c      | 6 ++++++
>  3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> index 66093fa78aed..045227ed4efc 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -136,6 +136,7 @@ extern int SendReceiveBlockingLock(const unsigned int xid,
>                         struct smb_hdr *out_buf,
>                         int *bytes_returned);
>
> +void smb2_query_server_interfaces(struct work_struct *work);
>  void
>  cifs_signal_cifsd_for_reconnect(struct TCP_Server_Info *server,
>                                       bool all_channels);
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index c48869c29e15..16c4f7fa1f34 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -97,7 +97,7 @@ static int reconn_set_ipaddr_from_hostname(struct TCP_Server_Info *server)
>         return rc;
>  }
>
> -static void smb2_query_server_interfaces(struct work_struct *work)
> +void smb2_query_server_interfaces(struct work_struct *work)
>  {
>         int rc;
>         int xid;
> @@ -2866,20 +2866,14 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb3_fs_context *ctx)
>         tcon->max_cached_dirs = ctx->max_cached_dirs;
>         tcon->nodelete = ctx->nodelete;
>         tcon->local_lease = ctx->local_lease;
> -       INIT_LIST_HEAD(&tcon->pending_opens);
>         tcon->status = TID_GOOD;
>
> -       INIT_DELAYED_WORK(&tcon->query_interfaces,
> -                         smb2_query_server_interfaces);
>         if (ses->server->dialect >= SMB30_PROT_ID &&
>             (ses->server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
>                 /* schedule query interfaces poll */
>                 queue_delayed_work(cifsiod_wq, &tcon->query_interfaces,
>                                    (SMB_INTERFACE_POLL_INTERVAL * HZ));
>         }
> -#ifdef CONFIG_CIFS_DFS_UPCALL
> -       INIT_DELAYED_WORK(&tcon->dfs_cache_work, dfs_cache_refresh);
> -#endif
>         spin_lock(&cifs_tcp_ses_lock);
>         list_add(&tcon->tcon_list, &ses->tcon_list);
>         spin_unlock(&cifs_tcp_ses_lock);
> diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
> index e77017f47084..da23cc12a52c 100644
> --- a/fs/smb/client/misc.c
> +++ b/fs/smb/client/misc.c
> @@ -151,6 +151,12 @@ tcon_info_alloc(bool dir_leases_enabled, enum smb3_tcon_ref_trace trace)
>  #ifdef CONFIG_CIFS_DFS_UPCALL
>         INIT_LIST_HEAD(&ret_buf->dfs_ses_list);
>  #endif
> +       INIT_LIST_HEAD(&ret_buf->pending_opens);
> +       INIT_DELAYED_WORK(&ret_buf->query_interfaces,
> +                         smb2_query_server_interfaces);
> +#ifdef CONFIG_CIFS_DFS_UPCALL
> +       INIT_DELAYED_WORK(&ret_buf->dfs_cache_work, dfs_cache_refresh);
> +#endif
>
>         return ret_buf;
>  }
> --
> 2.43.0
>


-- 
Thanks,

Steve

Re: [PATCH] cifs: all initializations for tcon should happen in tcon_info_alloc

[Paulo Alcantara]

nspmangalore@gmail.com writes:

> From: Shyam Prasad N <sprasad@microsoft.com>
>
> Today, a few work structs inside tcon are initialized inside
> cifs_get_tcon and not in tcon_info_alloc. As a result, if a tcon
> is obtained from tcon_info_alloc, but not called as a part of
> cifs_get_tcon, we may trip over.
>
> Cc: <stable@vger.kernel.org>

stable?  Makes no sense.

> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/smb/client/cifsproto.h | 1 +
>  fs/smb/client/connect.c   | 8 +-------
>  fs/smb/client/misc.c      | 6 ++++++
>  3 files changed, 8 insertions(+), 7 deletions(-)

Otherwise, looks good:

Reviewed-by: Paulo Alcantara (Red Hat) <pc@manguebit.org>

Re: [PATCH] cifs: all initializations for tcon should happen in tcon_info_alloc

[Shyam Prasad N]

On Tue, Jul 1, 2025 at 1:06 AM Paulo Alcantara <pc@manguebit.org> wrote:
>
> nspmangalore@gmail.com writes:
>
> > From: Shyam Prasad N <sprasad@microsoft.com>
> >
> > Today, a few work structs inside tcon are initialized inside
> > cifs_get_tcon and not in tcon_info_alloc. As a result, if a tcon
> > is obtained from tcon_info_alloc, but not called as a part of
> > cifs_get_tcon, we may trip over.
> >
> > Cc: <stable@vger.kernel.org>
>
> stable?  Makes no sense.

I feel this is a serious one. If some code were to use
tcon_info_alloc, they'd expect that it's fully initialized, but they'd
end up with the problem that you and David saw.
I feel that this is the correct fix to that problem (although that
addresses the problem of unnecessary scheduling of work).

>
> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > ---
> >  fs/smb/client/cifsproto.h | 1 +
> >  fs/smb/client/connect.c   | 8 +-------
> >  fs/smb/client/misc.c      | 6 ++++++
> >  3 files changed, 8 insertions(+), 7 deletions(-)
>
> Otherwise, looks good:
>
> Reviewed-by: Paulo Alcantara (Red Hat) <pc@manguebit.org>

Thanks.

-- 
Regards,
Shyam

Re: [PATCH] cifs: all initializations for tcon should happen in tcon_info_alloc

[Paulo Alcantara]

Shyam Prasad N <nspmangalore@gmail.com> writes:

> On Tue, Jul 1, 2025 at 1:06 AM Paulo Alcantara <pc@manguebit.org> wrote:
>>
>> nspmangalore@gmail.com writes:
>>
>> > From: Shyam Prasad N <sprasad@microsoft.com>
>> >
>> > Today, a few work structs inside tcon are initialized inside
>> > cifs_get_tcon and not in tcon_info_alloc. As a result, if a tcon
>> > is obtained from tcon_info_alloc, but not called as a part of
>> > cifs_get_tcon, we may trip over.
>> >
>> > Cc: <stable@vger.kernel.org>
>>
>> stable?  Makes no sense.
>
> I feel this is a serious one. If some code were to use
> tcon_info_alloc, they'd expect that it's fully initialized, but they'd
> end up with the problem that you and David saw.

Yes, I understand you want to be safe.  But you're not fixing any
existing problem with this patch, hence Cc stable didn't make sense to
me.

> I feel that this is the correct fix to that problem (although that
> addresses the problem of unnecessary scheduling of work).

You'd just mask the real problem with this.  Without the WARN_ON() on
the uninitialized delayed worker we wouldn't have found the actual bug,
though.