[PATCH 1/2] smb: client: hold server ref while queueing reconnect work

[PATCH 1/2] smb: client: hold server ref while queueing reconnect work

[Joseph Qi]

When query_server_interfaces() returns -EOPNOTSUPP on a secondary
channel, smb2_reconnect() calls smb3_update_ses_channels() which for a
channel runs cifs_chan_skip_or_disable().  That clears the channel's
slot in the session and calls cifs_put_tcp_session(), which can drop
the last reference of the channel and tear it down.

smb2_reconnect() then falls through to skip_add_channels and queues
reconnect work on that server, using a server that may already be
freed.  The send path obtained the channel from cifs_pick_channel(),
which does not take a reference, so the session's channel reference is
the only one.

Take our own reference on the channel before calling
smb3_update_ses_channels() and drop it after the reconnect work has
been queued, so the channel cannot be freed while it is still used.

Fixes: f591062bdbf4 ("cifs: handle servers that still advertise multichannel after disabling")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 fs/smb/client/smb2pdu.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index f85595136ef8a..8e53e9e02df6c 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -253,6 +253,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
 	struct cifs_ses *ses;
 	int xid;
 	int rc = 0;
+	bool hold_server_ref = false;
 
 	/*
 	 * SMB2s NegProt, SessSetup, Logoff do not have tcon yet so
@@ -470,6 +471,20 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
 			 * treat this as server not supporting multichannel
 			 */
 
+			/*
+			 * If server is a secondary channel,
+			 * cifs_chan_skip_or_disable() called below may
+			 * drop its last reference, so hold our own
+			 * reference until the reconnect work below has
+			 * been queued.
+			 */
+			spin_lock(&cifs_tcp_ses_lock);
+			if (server->srv_count) {
+				server->srv_count++;
+				hold_server_ref = true;
+			}
+			spin_unlock(&cifs_tcp_ses_lock);
+
 			rc = smb3_update_ses_channels(ses, server,
 						       from_reconnect,
 						       true /* disable_mchan */);
@@ -498,6 +513,8 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
 
 	if (smb2_command != SMB2_INTERNAL_CMD)
 		cifs_queue_server_reconn(server);
+	if (hold_server_ref)
+		cifs_put_tcp_session(server, from_reconnect);
 
 	atomic_inc(&tconInfoReconnectCount);
 out:
-- 
2.39.3

[PATCH 2/2] smb: client: skip NULL channel in __cifs_put_smb_ses()

[Joseph Qi]

cifs_chan_skip_or_disable() clears a secondary channel's slot in the
session (ses->chans[chan_index].server = NULL) without lowering
chan_count when the server stops supporting multichannel.  When the
session is later released, __cifs_put_smb_ses() walks every slot up to
chan_count and calls cifs_put_tcp_session() on it, which dereferences
the server pointer immediately, so a slot left NULL by the skip path
makes the release crash with a NULL pointer dereference.

Clearing a slot without lowering chan_count is older than
cifs_chan_skip_or_disable(): cifs_chan_update_iface() could already
do the same since b54034a73baf9 ("cifs: during reconnect, update
interface if necessary"), and the release loop has been unguarded
since then.  12d1e301bdfd1 ("cifs: do not let cifs_chan_update_iface
deallocate channels") later removed that trigger, but the crash is
reachable through it on older kernels.

cifs_decrease_secondary_channels() already guards the same put with
if (server).  Add the same guard to the release loop so a skipped
channel slot is left alone.

Fixes: b54034a73baf9 ("cifs: during reconnect, update interface if necessary")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 fs/smb/client/connect.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index e68fffb2ab2ea..6497b0d54fea7 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -2200,7 +2200,8 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
 			kref_put(&ses->chans[i].iface->refcount, release_iface);
 			ses->chans[i].iface = NULL;
 		}
-		cifs_put_tcp_session(ses->chans[i].server, 0);
+		if (ses->chans[i].server)
+			cifs_put_tcp_session(ses->chans[i].server, 0);
 		ses->chans[i].server = NULL;
 	}
 
-- 
2.39.3

Re: [PATCH 2/2] smb: client: skip NULL channel in __cifs_put_smb_ses()

[Paulo Alcantara]

Joseph Qi <joseph.qi@linux.alibaba.com> writes:

> cifs_chan_skip_or_disable() clears a secondary channel's slot in the
> session (ses->chans[chan_index].server = NULL) without lowering
> chan_count when the server stops supporting multichannel.  When the
> session is later released, __cifs_put_smb_ses() walks every slot up to
> chan_count and calls cifs_put_tcp_session() on it, which dereferences
> the server pointer immediately, so a slot left NULL by the skip path
> makes the release crash with a NULL pointer dereference.
>
> Clearing a slot without lowering chan_count is older than
> cifs_chan_skip_or_disable(): cifs_chan_update_iface() could already
> do the same since b54034a73baf9 ("cifs: during reconnect, update
> interface if necessary"), and the release loop has been unguarded
> since then.  12d1e301bdfd1 ("cifs: do not let cifs_chan_update_iface
> deallocate channels") later removed that trigger, but the crash is
> reachable through it on older kernels.
>
> cifs_decrease_secondary_channels() already guards the same put with
> if (server).  Add the same guard to the release loop so a skipped
> channel slot is left alone.
> ...

Applied.

Re: [PATCH 1/2] smb: client: hold server ref while queueing reconnect work

[Paulo Alcantara]

Joseph Qi <joseph.qi@linux.alibaba.com> writes:

> When query_server_interfaces() returns -EOPNOTSUPP on a secondary
> channel, smb2_reconnect() calls smb3_update_ses_channels() which for a
> channel runs cifs_chan_skip_or_disable().  That clears the channel's
> slot in the session and calls cifs_put_tcp_session(), which can drop
> the last reference of the channel and tear it down.
>
> smb2_reconnect() then falls through to skip_add_channels and queues
> reconnect work on that server, using a server that may already be
> freed.  The send path obtained the channel from cifs_pick_channel(),
> which does not take a reference, so the session's channel reference is
> the only one.
>
> Take our own reference on the channel before calling
> smb3_update_ses_channels() and drop it after the reconnect work has
> been queued, so the channel cannot be freed while it is still used.
>
> Fixes: f591062bdbf4 ("cifs: handle servers that still advertise multichannel after disabling")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>

Please check sashiko comments

        https://sashiko.dev/#/patchset/20261103091558.2342761-1-joseph.qi%40linux.alibaba.com

Re: [PATCH 1/2] smb: client: hold server ref while queueing reconnect work

[Joseph Qi]

Hi Paulo,

Thanks for reviewing it.

On 11/8/26 1:28 AM, Paulo Alcantara wrote:
> Joseph Qi <joseph.qi@linux.alibaba.com> writes:
> 
>> When query_server_interfaces() returns -EOPNOTSUPP on a secondary
>> channel, smb2_reconnect() calls smb3_update_ses_channels() which for a
>> channel runs cifs_chan_skip_or_disable().  That clears the channel's
>> slot in the session and calls cifs_put_tcp_session(), which can drop
>> the last reference of the channel and tear it down.
>>
>> smb2_reconnect() then falls through to skip_add_channels and queues
>> reconnect work on that server, using a server that may already be
>> freed.  The send path obtained the channel from cifs_pick_channel(),
>> which does not take a reference, so the session's channel reference is
>> the only one.
>>
>> Take our own reference on the channel before calling
>> smb3_update_ses_channels() and drop it after the reconnect work has
>> been queued, so the channel cannot be freed while it is still used.
>>
>> Fixes: f591062bdbf4 ("cifs: handle servers that still advertise multichannel after disabling")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> 
> Please check sashiko comments
> 
>         https://sashiko.dev/#/patchset/20261103091558.2342761-1-joseph.qi%40linux.alibaba.com

These points are about the multichannel disable path as a whole rather
than about this change, so let me try to answer them together.

What this patch does is narrow.  It holds a reference on the channel
across smb3_update_ses_channels() so that the final cifs_put_tcp_session()
runs after cifs_queue_server_reconn() instead of before it.  Previously
the last reference was dropped inside cifs_chan_skip_or_disable(), which
cancelled server->reconnect, and skip_add_channels then re-armed it on a
server whose srv_count was already zero.

Queueing reconnect work on a just-terminated channel is indeed
pointless, but that comes from f591062bdbf4 ("cifs: handle servers that
still advertise multichannel after disabling"), which added the goto
skip_add_channels into a label that already ended with
mod_delayed_work(cifsiod_wq, &server->reconnect, 0).  With the reference
held, the work is either cancelled by the final put or runs and bails on
server->terminate in smb2_reconnect_server(), before it reaches
smb2_reconnect() and ses->session_mutex.  The send path holds no locks
there, so neither ordering can deadlock.

The srv_count == 0 race is reachable, and smb3_reconfigure() is the
clearer path into cifs_decrease_secondary_channels().  But it is older
than this patch and I don't think it can be closed here.
cifs_pick_channel() returns a channel without a reference, so once
srv_count can drop to zero under us the pointer is already stale on
entry to smb2_reconnect().  Reading server->srv_count to decide whether
to take a reference would itself be the use-after-free.

The guarded increment is the same idiom smb2_reconnect_server() already
uses, and it narrows that window rather than widening it.  Refcounting
the channel selection looks like the real fix, which is a much larger
change.  There is at least no double put, since cifs_ses_get_chan_index()
returns CIFS_INVAL_CHAN_INDEX once terminate is set and
cifs_chan_skip_or_disable() then returns -EHOSTDOWN without putting
again.

So what's your opinion?

Thanks,
Joseph

Re: [PATCH 1/2] smb: client: hold server ref while queueing reconnect work

[Paulo Alcantara]

Joseph Qi <joseph.qi@linux.alibaba.com> writes:

> So what's your opinion?

LGTM.  Thanks for looking into it.

I've applied it.