Re: [PATCH 12/14] cifs: handle when server stops supporting multichannel

Shyam Prasad N
2023-11-10T06:57:35Z

[-- Attachment #1: Type: text/plain, Size: 3954 bytes --]

On Thu, Nov 9, 2023 at 7:19 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> On Thu, Nov 9, 2023 at 6:58 PM Paulo Alcantara <pc@manguebit.com> wrote:
> >
> > Shyam Prasad N <nspmangalore@gmail.com> writes:
> >
> > > On Thu, Nov 9, 2023 at 1:11 AM Paulo Alcantara <pc@manguebit.com> wrote:
> > >>
> > >> > and leaked server connections
> > >> >
> > >> > Display Internal CIFS Data Structures for Debugging
> > >> > ---------------------------------------------------
> > >> > CIFS Version 2.46
> > >> > Features: DFS,FSCACHE,STATS2,DEBUG2,ALLOW_INSECURE_LEGACY,CIFS_POSIX,UPCALL(SPNEGO),XATTR,ACL,WITNESS
> > >> > CIFSMaxBufSize: 16384
> > >> > Active VFS Requests: 0
> > >> >
> > >> > Servers:
> > >> > 1) ConnectionId: 0x70e Hostname: w22-root2.gandalf.test
> > >> > ClientGUID: 44DAE383-3E91-3042-85FE-87D6F17298B7
> > >> > Number of credits: 1,1,1 Dialect 0x210
> > >> > Server capabilities: 0x300007
> > >> > TCP status: 4 Instance: 77
> > >> > Local Users To Server: 1 SecMode: 0x1 Req On Wire: 0 Net namespace: 4026531840
> > >> > In Send: 0 In MaxReq Wait: 0
> > >> > DFS leaf full path: \\W22-ROOT1.GANDALF.TEST\dfstest3\link10
> > >> >
> > >> > Sessions:
> > >> > [NONE]
> > >> > 2) ConnectionId: 0x706 Hostname: w22-root2.gandalf.test
> > >> > ClientGUID: C8CF45E4-F70D-DF40-8821-0234A2E20DD4
> > >> > Number of credits: 1,1,1 Dialect 0x210
> > >> > Server capabilities: 0x300007
> > >> > TCP status: 4 Instance: 81
> > >> > Local Users To Server: 1 SecMode: 0x1 Req On Wire: 0 Net namespace: 4026531840
> > >> > In Send: 0 In MaxReq Wait: 0
> > >> > DFS leaf full path: \\W22-ROOT1.GANDALF.TEST\dfstest3\link6
> > >> >
> > >> > Sessions:
> > >> > [NONE]
> > >> > 3) ConnectionId: 0x6ae Hostname: w22-root1.gandalf.test
> > >> > ClientGUID: AB059CDD-12FF-B94D-B30C-9E1928ACBA95
> > >> > Number of credits: 1,1,1 Dialect 0x210
> > >> > Server capabilities: 0x300007
> > >> > TCP status: 4 Instance: 96
> > >> > Local Users To Server: 1 SecMode: 0x1 Req On Wire: 0 Net namespace: 4026531840
> > >> > In Send: 0 In MaxReq Wait: 0
> > >> > DFS leaf full path: \\W22-ROOT1.GANDALF.TEST\dfstest3\link9
> > >> >
> > >> > Sessions:
> > >> > [NONE]
> > >> > ...
> > >>
> > >> I ended up with a simple reproducer for the above
> > >>
> > >> mount.cifs //srv/share /mnt/1 -o ...,echo_interval=10
> > >> iptables -I INPUT -s $srvaddr -j DROP
> > >> stat -f /mnt/1
> > >> # check dmesg for "BUG: sleeping function called from invalid context"
> > >> iptables -I INPUT -s $srvaddr -j ACCEPT
> > >> stat -f /mnt/1
> > >> umount /mnt/1
> > >> # check /proc/fs/cifs/DebugData for leaked server connection
> > >
> > > Ack. I'll try and get a repro locally and debug this one.
> >
> > This should be related to patch 10/14 as you're taking an extra
> > reference of @server over reconnect, and when the client reconnects and
> > umount, that reference don't seem to be put afterwards.
>
> The idea is that the reference is put in the reconnect worker.
> I think the issue is that I don't drop the final reference if
> cancel_delayed_work_sync was able to cancel a work.
This was already taken care of.

> Let me work on the updated patch.
>
> Also, I found the reason for "BUG: sleeping function called from
> invalid context". I've enabled CONFIG_DEBUG_ATOMIC_SLEEP in my test
> config now.
>
> --
> Regards,
> Shyam

Hi Paulo,

Can you please check if the problem is still seen with these updated patches?
I was unable to reproduce the issue with the steps you provided.

--
Regards,
Shyam

[-- Attachment #2: 0007-cifs-handle-when-server-stops-supporting-multichanne.patch --]
[-- Type: application/octet-stream, Size: 9561 bytes --]

From 5106086473124167e9e0bdcd5895c2f48e5eb816 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Fri, 13 Oct 2023 11:40:09 +0000
Subject: [PATCH 7/9] cifs: handle when server stops supporting multichannel

When a server stops supporting multichannel, we will
keep attempting reconnects to the secondary channels today.
Avoid this by freeing extra channels when negotiate
returns no multichannel support.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
fs/smb/client/cifsglob.h | 1 +
fs/smb/client/cifsproto.h | 2 ++
fs/smb/client/connect.c | 10 ++++++
fs/smb/client/sess.c | 64 ++++++++++++++++++++++++++++-----
fs/smb/client/smb2pdu.c | 76 ++++++++++++++++++++++++++++++++++++++-
fs/smb/client/transport.c | 2 +-
6 files changed, 145 insertions(+), 10 deletions(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 81e7a45f413d..62d5a5495bb8 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -650,6 +650,7 @@ struct TCP_Server_Info {
bool noautotune; /* do not autotune send buf sizes */
bool nosharesock;
bool tcp_nodelay;
+ bool terminate;
unsigned int credits; /* send no more requests at once */
unsigned int max_credits; /* can override large 32000 default at mnt */
unsigned int in_flight; /* number of requests on the wire to server */
diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index 00ad41633e26..d87e2c26cce2 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -641,6 +641,8 @@ cifs_chan_needs_reconnect(struct cifs_ses *ses,
bool
cifs_chan_is_iface_active(struct cifs_ses *ses,
struct TCP_Server_Info *server);
+void
+cifs_disable_secondary_channels(struct cifs_ses *ses);
int
cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server);
int
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 1490547ec493..57c2a7df3457 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -219,6 +219,14 @@ cifs_mark_tcp_ses_conns_for_reconnect(struct TCP_Server_Info *server,

spin_lock(&cifs_tcp_ses_lock);
list_for_each_entry_safe(ses, nses, &pserver->smb_ses_list, smb_ses_list) {
+ /*
+ * if channel has been marked for termination, nothing to do
+ * for the channel. in fact, we cannot find the channel for the
+ * server. So safe to exit here
+ */
+ if (server->terminate)
+ break;
+
/* check if iface is still active */
if (!cifs_chan_is_iface_active(ses, server))
cifs_chan_update_iface(ses, server);
@@ -253,6 +261,8 @@ cifs_mark_tcp_ses_conns_for_reconnect(struct TCP_Server_Info *server,
spin_lock(&tcon->tc_lock);
tcon->status = TID_NEED_RECON;
spin_unlock(&tcon->tc_lock);
+
+ cancel_delayed_work(&tcon->query_interfaces);
}
if (ses->tcon_ipc) {
ses->tcon_ipc->need_reconnect = true;
diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
index 1457f81af0cf..26784e338388 100644
--- a/fs/smb/client/sess.c
+++ b/fs/smb/client/sess.c
@@ -289,6 +289,60 @@ int cifs_try_adding_channels(struct cifs_ses *ses)
return new_chan_count - old_chan_count;
}

+/*
+ * called when multichannel is disabled by the server.
+ * this always gets called from smb2_reconnect
+ * and cannot get called in parallel threads.
+ */
+void
+cifs_disable_secondary_channels(struct cifs_ses *ses)
+{
+ int i, chan_count;
+ struct TCP_Server_Info *server;
+ struct cifs_server_iface *iface;
+
+ spin_lock(&ses->chan_lock);
+ chan_count = ses->chan_count;
+ if (chan_count == 1)
+ goto done;
+
+ ses->chan_count = 1;
+
+ /* for all secondary channels reset the need reconnect bit */
+ ses->chans_need_reconnect &= 1;
+
+ for (i = 1; i < chan_count; i++) {
+ iface = ses->chans[i].iface;
+ server = ses->chans[i].server;
+
+ if (iface) {
+ spin_lock(&ses->iface_lock);
+ kref_put(&iface->refcount, release_iface);
+ ses->chans[i].iface = NULL;
+ iface->num_channels--;
+ if (iface->weight_fulfilled)
+ iface->weight_fulfilled--;
+ spin_unlock(&ses->iface_lock);
+ }
+
+ spin_unlock(&ses->chan_lock);
+ if (server && !server->terminate) {
+ server->terminate = true;
+ cifs_signal_cifsd_for_reconnect(server, false);
+ }
+ spin_lock(&ses->chan_lock);
+
+ if (server) {
+ ses->chans[i].server = NULL;
+ cifs_put_tcp_session(server, false);
+ }
+
+ }
+
+done:
+ spin_unlock(&ses->chan_lock);
+}
+
/*
* update the iface for the channel if necessary.
* will return 0 when iface is updated, 1 if removed, 2 otherwise
@@ -586,14 +640,10 @@ cifs_ses_add_channel(struct cifs_ses *ses,

out:
if (rc && chan->server) {
- /*
- * we should avoid race with these delayed works before we
- * remove this channel
- */
- cancel_delayed_work_sync(&chan->server->echo);
- cancel_delayed_work_sync(&chan->server->reconnect);
+ cifs_put_tcp_session(chan->server, 0);

spin_lock(&ses->chan_lock);
+
/* we rely on all bits beyond chan_count to be clear */
cifs_chan_clear_need_reconnect(ses, chan->server);
ses->chan_count--;
@@ -603,8 +653,6 @@ cifs_ses_add_channel(struct cifs_ses *ses,
*/
WARN_ON(ses->chan_count < 1);
spin_unlock(&ses->chan_lock);
-
- cifs_put_tcp_session(chan->server, 0);
}

kfree(ctx->UNC);
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index 0d4351e43069..2eb29fa278c3 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -164,6 +164,8 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
struct nls_table *nls_codepage = NULL;
struct cifs_ses *ses;
int xid;
+ struct TCP_Server_Info *pserver;
+ unsigned int chan_index;

/*
* SMB2s NegProt, SessSetup, Logoff do not have tcon yet so
@@ -224,6 +226,12 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
return -EAGAIN;
}
}
+
+ /* if server is marked for termination, cifsd will cleanup */
+ if (server->terminate) {
+ spin_unlock(&server->srv_lock);
+ return -EHOSTDOWN;
+ }
spin_unlock(&server->srv_lock);

again:
@@ -242,12 +250,24 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
tcon->need_reconnect);

mutex_lock(&ses->session_mutex);
+ /*
+ * if this is called by delayed work, and the channel has been disabled
+ * in parallel, the delayed work can continue to execute in parallel
+ * there's a chance that this channel may not exist anymore
+ */
+ spin_lock(&server->srv_lock);
+ if (server->tcpStatus == CifsExiting) {
+ spin_unlock(&server->srv_lock);
+ mutex_unlock(&ses->session_mutex);
+ rc = -EHOSTDOWN;
+ goto out;
+ }
+
/*
* Recheck after acquire mutex. If another thread is negotiating
* and the server never sends an answer the socket will be closed
* and tcpStatus set to reconnect.
*/
- spin_lock(&server->srv_lock);
if (server->tcpStatus == CifsNeedReconnect) {
spin_unlock(&server->srv_lock);
mutex_unlock(&ses->session_mutex);
@@ -284,6 +304,53 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,

rc = cifs_negotiate_protocol(0, ses, server);
if (!rc) {
+ /*
+ * if server stopped supporting multichannel
+ * and the first channel reconnected, disable all the others.
+ */
+ if (ses->chan_count > 1 &&
+ !(server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
+ if (SERVER_IS_CHAN(server)) {
+ cifs_dbg(VFS, "server %s does not support " \
+ "multichannel anymore. skipping secondary channel\n",
+ ses->server->hostname);
+
+ spin_lock(&ses->chan_lock);
+ chan_index = cifs_ses_get_chan_index(ses, server);
+ if (chan_index == CIFS_INVAL_CHAN_INDEX) {
+ spin_unlock(&ses->chan_lock);
+ goto skip_terminate;
+ }
+
+ ses->chans[chan_index].server = NULL;
+ spin_unlock(&ses->chan_lock);
+
+ /*
+ * the above reference of server by channel
+ * needs to be dropped without holding chan_lock
+ * as cifs_put_tcp_session takes a higher lock
+ * i.e. cifs_tcp_ses_lock
+ */
+ cifs_put_tcp_session(server, 1);
+
+ server->terminate = true;
+ cifs_signal_cifsd_for_reconnect(server, false);
+
+ /* mark primary server as needing reconnect */
+ pserver = server->primary_server;
+ cifs_signal_cifsd_for_reconnect(pserver, false);
+
+skip_terminate:
+ mutex_unlock(&ses->session_mutex);
+ rc = -EHOSTDOWN;
+ goto out;
+ } else {
+ cifs_server_dbg(VFS, "does not support " \
+ "multichannel anymore. disabling all other channels\n");
+ cifs_disable_secondary_channels(ses);
+ }
+ }
+
rc = cifs_setup_session(0, ses, server, nls_codepage);
if ((rc == -EACCES) && !tcon->retry) {
mutex_unlock(&ses->session_mutex);
@@ -3836,6 +3903,13 @@ void smb2_reconnect_server(struct work_struct *work)
/* Prevent simultaneous reconnects that can corrupt tcon->rlist list */
mutex_lock(&pserver->reconnect_mutex);

+ /* if the server is marked for termination, drop the ref count here */
+ if (server->terminate) {
+ cifs_put_tcp_session(server, true);
+ mutex_unlock(&pserver->reconnect_mutex);
+ return;
+ }
+
INIT_LIST_HEAD(&tmp_list);
INIT_LIST_HEAD(&tmp_ses_list);
cifs_dbg(FYI, "Reconnecting tcons and channels\n");
diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
index d553b7a54621..4f717ad7c21b 100644
--- a/fs/smb/client/transport.c
+++ b/fs/smb/client/transport.c
@@ -1023,7 +1023,7 @@ struct TCP_Server_Info *cifs_pick_channel(struct cifs_ses *ses)
spin_lock(&ses->chan_lock);
for (i = 0; i < ses->chan_count; i++) {
server = ses->chans[i].server;
- if (!server)
+ if (!server || server->terminate)
continue;

/*
--
2.34.1

[-- Attachment #3: 0006-cifs-handle-when-server-starts-supporting-multichann.patch --]
[-- Type: application/octet-stream, Size: 3256 bytes --]

From b6f410cb8635d01875938f3991d7b093d83e264d Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Fri, 13 Oct 2023 11:33:21 +0000
Subject: [PATCH 6/9] cifs: handle when server starts supporting multichannel

When the user mounts with multichannel option, but the
server does not support it, there can be a time in future
where it can be supported.

With this change, such a case is handled.

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

diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index eed8dbb6b2fb..00ad41633e26 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -132,6 +132,7 @@ extern int SendReceiveBlockingLock(const unsigned int xid,
struct smb_hdr *in_buf,
struct smb_hdr *out_buf,
int *bytes_returned);
+
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 b514b41cc9f0..1490547ec493 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -132,6 +132,9 @@ static void smb2_query_server_interfaces(struct work_struct *work)
free_xid(xid);

if (rc) {
+ if (rc == -EOPNOTSUPP)
+ return;
+
cifs_dbg(FYI, "%s: failed to query server interfaces: %d\n",
__func__, rc);
}
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index b7665155f4e2..0d4351e43069 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -163,6 +163,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
int rc = 0;
struct nls_table *nls_codepage = NULL;
struct cifs_ses *ses;
+ int xid;

/*
* SMB2s NegProt, SessSetup, Logoff do not have tcon yet so
@@ -307,17 +308,44 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
tcon->need_reopen_files = true;

rc = cifs_tree_connect(0, tcon, nls_codepage);
- mutex_unlock(&ses->session_mutex);

cifs_dbg(FYI, "reconnect tcon rc = %d\n", rc);
if (rc) {
/* If sess reconnected but tcon didn't, something strange ... */
+ mutex_unlock(&ses->session_mutex);
cifs_dbg(VFS, "reconnect tcon failed rc = %d\n", rc);
goto out;
}

+ if (!rc &&
+ (server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
+ mutex_unlock(&ses->session_mutex);
+
+ /*
+ * query server network interfaces, in case they change
+ */
+ xid = get_xid();
+ rc = SMB3_request_interfaces(xid, tcon, false);
+ free_xid(xid);
+
+ if (rc)
+ cifs_dbg(FYI, "%s: failed to query server interfaces: %d\n",
+ __func__, rc);
+
+ if (ses->chan_max > ses->chan_count &&
+ !SERVER_IS_CHAN(server)) {
+ if (ses->chan_count == 1)
+ cifs_server_dbg(VFS, "supports multichannel now\n");
+
+ cifs_try_adding_channels(ses);
+ }
+ } else {
+ mutex_unlock(&ses->session_mutex);
+ }
+
if (smb2_command != SMB2_INTERNAL_CMD)
- mod_delayed_work(cifsiod_wq, &server->reconnect, 0);
+ if (mod_delayed_work(cifsiod_wq, &server->reconnect, 0))
+ cifs_put_tcp_session(server, false);

atomic_inc(&tconInfoReconnectCount);
out:
--
2.34.1