* [PATCH 1/2] cifs: fix mid leak during reconnection after timeout threshold
@ 2023-07-14 8:56 Shyam Prasad N
2023-07-14 8:56 ` [PATCH 2/2] cifs: is_network_name_deleted should return a bool Shyam Prasad N
0 siblings, 1 reply; 5+ messages in thread
From: Shyam Prasad N @ 2023-07-14 8:56 UTC (permalink / raw)
To: linux-cifs, smfrench, bharathsm.hsk, pc; +Cc: Shyam Prasad N
When the number of responses with status of STATUS_IO_TIMEOUT
exceeds a specified threshold (NUM_STATUS_IO_TIMEOUT), we reconnect
the connection. But we do not return the mid, or the credits
returned for the mid, or reduce the number of in-flight requests.
This bug could result in the server->in_flight count to go bad,
and also cause a leak in the mids.
This change moves the check to a few lines below where the
response is decrypted, even of the response is read from the
transform header. This way, the code for returning the mids
can be reused.
Also, the cifs_reconnect was reconnecting just the transport
connection before. In case of multi-channel, this may not be
what we want to do after several timeouts. Changed that to
reconnect the session and the tree too.
Also renamed NUM_STATUS_IO_TIMEOUT to a more appropriate name
MAX_STATUS_IO_TIMEOUT.
Fixes: 8e670f77c4a5 ("Handle STATUS_IO_TIMEOUT gracefully")
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
fs/smb/client/connect.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 9d16626e7a66..87047bd38485 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -60,7 +60,7 @@ extern bool disable_legacy_dialects;
#define TLINK_IDLE_EXPIRE (600 * HZ)
/* Drop the connection to not overload the server */
-#define NUM_STATUS_IO_TIMEOUT 5
+#define MAX_STATUS_IO_TIMEOUT 5
static int ip_connect(struct TCP_Server_Info *server);
static int generic_ip_connect(struct TCP_Server_Info *server);
@@ -1118,6 +1118,7 @@ cifs_demultiplex_thread(void *p)
struct mid_q_entry *mids[MAX_COMPOUND];
char *bufs[MAX_COMPOUND];
unsigned int noreclaim_flag, num_io_timeout = 0;
+ bool pending_reconnect = false;
noreclaim_flag = memalloc_noreclaim_save();
cifs_dbg(FYI, "Demultiplex PID: %d\n", task_pid_nr(current));
@@ -1157,6 +1158,8 @@ cifs_demultiplex_thread(void *p)
cifs_dbg(FYI, "RFC1002 header 0x%x\n", pdu_length);
if (!is_smb_response(server, buf[0]))
continue;
+
+ pending_reconnect = false;
next_pdu:
server->pdu_size = pdu_length;
@@ -1214,10 +1217,13 @@ cifs_demultiplex_thread(void *p)
if (server->ops->is_status_io_timeout &&
server->ops->is_status_io_timeout(buf)) {
num_io_timeout++;
- if (num_io_timeout > NUM_STATUS_IO_TIMEOUT) {
- cifs_reconnect(server, false);
+ if (num_io_timeout > MAX_STATUS_IO_TIMEOUT) {
+ cifs_server_dbg(VFS,
+ "Number of request timeouts exceeded %d. Reconnecting",
+ MAX_STATUS_IO_TIMEOUT);
+
+ pending_reconnect = true;
num_io_timeout = 0;
- continue;
}
}
@@ -1264,6 +1270,11 @@ cifs_demultiplex_thread(void *p)
buf = server->smallbuf;
goto next_pdu;
}
+
+ /* do this reconnect at the very end after processing all MIDs */
+ if (pending_reconnect)
+ cifs_reconnect(server, true);
+
} /* end while !EXITING */
/* buffer usually freed in free_mid - need to free it here on exit */
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] cifs: is_network_name_deleted should return a bool
2023-07-14 8:56 [PATCH 1/2] cifs: fix mid leak during reconnection after timeout threshold Shyam Prasad N
@ 2023-07-14 8:56 ` Shyam Prasad N
2023-07-14 15:36 ` Steve French
2023-07-14 16:30 ` Steve French
0 siblings, 2 replies; 5+ messages in thread
From: Shyam Prasad N @ 2023-07-14 8:56 UTC (permalink / raw)
To: linux-cifs, smfrench, bharathsm.hsk, pc; +Cc: Shyam Prasad N
Currently, is_network_name_deleted and it's implementations
do not return anything if the network name did get deleted.
So the function doesn't fully achieve what it advertizes.
Changed the function to return a bool instead. It will now
return true if the error returned is STATUS_NETWORK_NAME_DELETED
and the share (tree id) was found to be connected. It returns
false otherwise.
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
fs/smb/client/cifsglob.h | 2 +-
fs/smb/client/connect.c | 11 ++++++++---
fs/smb/client/smb2ops.c | 8 +++++---
3 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index b212a4e16b39..bde9de6665a7 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -532,7 +532,7 @@ struct smb_version_operations {
/* Check for STATUS_IO_TIMEOUT */
bool (*is_status_io_timeout)(char *buf);
/* Check for STATUS_NETWORK_NAME_DELETED */
- void (*is_network_name_deleted)(char *buf, struct TCP_Server_Info *srv);
+ bool (*is_network_name_deleted)(char *buf, struct TCP_Server_Info *srv);
};
struct smb_version_values {
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 87047bd38485..6756ce4ff641 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -1233,9 +1233,14 @@ cifs_demultiplex_thread(void *p)
if (mids[i] != NULL) {
mids[i]->resp_buf_size = server->pdu_size;
- if (bufs[i] && server->ops->is_network_name_deleted)
- server->ops->is_network_name_deleted(bufs[i],
- server);
+ if (bufs[i] != NULL) {
+ if (server->ops->is_network_name_deleted &&
+ server->ops->is_network_name_deleted(bufs[i],
+ server)) {
+ cifs_server_dbg(FYI,
+ "Share deleted. Reconnect needed");
+ }
+ }
if (!mids[i]->multiRsp || mids[i]->multiEnd)
mids[i]->callback(mids[i]);
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 153b300621eb..d32477315abc 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -2391,7 +2391,7 @@ smb2_is_status_io_timeout(char *buf)
return false;
}
-static void
+static bool
smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
{
struct smb2_hdr *shdr = (struct smb2_hdr *)buf;
@@ -2400,7 +2400,7 @@ smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
struct cifs_tcon *tcon;
if (shdr->Status != STATUS_NETWORK_NAME_DELETED)
- return;
+ return false;
/* If server is a channel, select the primary channel */
pserver = CIFS_SERVER_IS_CHAN(server) ? server->primary_server : server;
@@ -2415,11 +2415,13 @@ smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
spin_unlock(&cifs_tcp_ses_lock);
pr_warn_once("Server share %s deleted.\n",
tcon->tree_name);
- return;
+ return true;
}
}
}
spin_unlock(&cifs_tcp_ses_lock);
+
+ return false;
}
static int
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] cifs: is_network_name_deleted should return a bool
2023-07-14 8:56 ` [PATCH 2/2] cifs: is_network_name_deleted should return a bool Shyam Prasad N
@ 2023-07-14 15:36 ` Steve French
2023-07-17 3:13 ` Shyam Prasad N
2023-07-14 16:30 ` Steve French
1 sibling, 1 reply; 5+ messages in thread
From: Steve French @ 2023-07-14 15:36 UTC (permalink / raw)
To: Shyam Prasad N; +Cc: linux-cifs, bharathsm.hsk, pc, Shyam Prasad N
should we have a dynamic trace point for this event as well?
On Fri, Jul 14, 2023 at 3:56 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> Currently, is_network_name_deleted and it's implementations
> do not return anything if the network name did get deleted.
> So the function doesn't fully achieve what it advertizes.
>
> Changed the function to return a bool instead. It will now
> return true if the error returned is STATUS_NETWORK_NAME_DELETED
> and the share (tree id) was found to be connected. It returns
> false otherwise.
>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
> fs/smb/client/cifsglob.h | 2 +-
> fs/smb/client/connect.c | 11 ++++++++---
> fs/smb/client/smb2ops.c | 8 +++++---
> 3 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index b212a4e16b39..bde9de6665a7 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -532,7 +532,7 @@ struct smb_version_operations {
> /* Check for STATUS_IO_TIMEOUT */
> bool (*is_status_io_timeout)(char *buf);
> /* Check for STATUS_NETWORK_NAME_DELETED */
> - void (*is_network_name_deleted)(char *buf, struct TCP_Server_Info *srv);
> + bool (*is_network_name_deleted)(char *buf, struct TCP_Server_Info *srv);
> };
>
> struct smb_version_values {
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index 87047bd38485..6756ce4ff641 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -1233,9 +1233,14 @@ cifs_demultiplex_thread(void *p)
> if (mids[i] != NULL) {
> mids[i]->resp_buf_size = server->pdu_size;
>
> - if (bufs[i] && server->ops->is_network_name_deleted)
> - server->ops->is_network_name_deleted(bufs[i],
> - server);
> + if (bufs[i] != NULL) {
> + if (server->ops->is_network_name_deleted &&
> + server->ops->is_network_name_deleted(bufs[i],
> + server)) {
> + cifs_server_dbg(FYI,
> + "Share deleted. Reconnect needed");
> + }
> + }
>
> if (!mids[i]->multiRsp || mids[i]->multiEnd)
> mids[i]->callback(mids[i]);
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index 153b300621eb..d32477315abc 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -2391,7 +2391,7 @@ smb2_is_status_io_timeout(char *buf)
> return false;
> }
>
> -static void
> +static bool
> smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
> {
> struct smb2_hdr *shdr = (struct smb2_hdr *)buf;
> @@ -2400,7 +2400,7 @@ smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
> struct cifs_tcon *tcon;
>
> if (shdr->Status != STATUS_NETWORK_NAME_DELETED)
> - return;
> + return false;
>
> /* If server is a channel, select the primary channel */
> pserver = CIFS_SERVER_IS_CHAN(server) ? server->primary_server : server;
> @@ -2415,11 +2415,13 @@ smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
> spin_unlock(&cifs_tcp_ses_lock);
> pr_warn_once("Server share %s deleted.\n",
> tcon->tree_name);
> - return;
> + return true;
> }
> }
> }
> spin_unlock(&cifs_tcp_ses_lock);
> +
> + return false;
> }
>
> static int
> --
> 2.34.1
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] cifs: is_network_name_deleted should return a bool
2023-07-14 8:56 ` [PATCH 2/2] cifs: is_network_name_deleted should return a bool Shyam Prasad N
2023-07-14 15:36 ` Steve French
@ 2023-07-14 16:30 ` Steve French
1 sibling, 0 replies; 5+ messages in thread
From: Steve French @ 2023-07-14 16:30 UTC (permalink / raw)
To: Shyam Prasad N; +Cc: linux-cifs, bharathsm.hsk, pc, Shyam Prasad N
tentatively merged into cifs-2.6.git for-next pending testing (e.g.
http://smb311-linux-testing.southcentralus.cloudapp.azure.com/#/builders/1/builds/72)
On Fri, Jul 14, 2023 at 3:56 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> Currently, is_network_name_deleted and it's implementations
> do not return anything if the network name did get deleted.
> So the function doesn't fully achieve what it advertizes.
>
> Changed the function to return a bool instead. It will now
> return true if the error returned is STATUS_NETWORK_NAME_DELETED
> and the share (tree id) was found to be connected. It returns
> false otherwise.
>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
> fs/smb/client/cifsglob.h | 2 +-
> fs/smb/client/connect.c | 11 ++++++++---
> fs/smb/client/smb2ops.c | 8 +++++---
> 3 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index b212a4e16b39..bde9de6665a7 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -532,7 +532,7 @@ struct smb_version_operations {
> /* Check for STATUS_IO_TIMEOUT */
> bool (*is_status_io_timeout)(char *buf);
> /* Check for STATUS_NETWORK_NAME_DELETED */
> - void (*is_network_name_deleted)(char *buf, struct TCP_Server_Info *srv);
> + bool (*is_network_name_deleted)(char *buf, struct TCP_Server_Info *srv);
> };
>
> struct smb_version_values {
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index 87047bd38485..6756ce4ff641 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -1233,9 +1233,14 @@ cifs_demultiplex_thread(void *p)
> if (mids[i] != NULL) {
> mids[i]->resp_buf_size = server->pdu_size;
>
> - if (bufs[i] && server->ops->is_network_name_deleted)
> - server->ops->is_network_name_deleted(bufs[i],
> - server);
> + if (bufs[i] != NULL) {
> + if (server->ops->is_network_name_deleted &&
> + server->ops->is_network_name_deleted(bufs[i],
> + server)) {
> + cifs_server_dbg(FYI,
> + "Share deleted. Reconnect needed");
> + }
> + }
>
> if (!mids[i]->multiRsp || mids[i]->multiEnd)
> mids[i]->callback(mids[i]);
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index 153b300621eb..d32477315abc 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -2391,7 +2391,7 @@ smb2_is_status_io_timeout(char *buf)
> return false;
> }
>
> -static void
> +static bool
> smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
> {
> struct smb2_hdr *shdr = (struct smb2_hdr *)buf;
> @@ -2400,7 +2400,7 @@ smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
> struct cifs_tcon *tcon;
>
> if (shdr->Status != STATUS_NETWORK_NAME_DELETED)
> - return;
> + return false;
>
> /* If server is a channel, select the primary channel */
> pserver = CIFS_SERVER_IS_CHAN(server) ? server->primary_server : server;
> @@ -2415,11 +2415,13 @@ smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
> spin_unlock(&cifs_tcp_ses_lock);
> pr_warn_once("Server share %s deleted.\n",
> tcon->tree_name);
> - return;
> + return true;
> }
> }
> }
> spin_unlock(&cifs_tcp_ses_lock);
> +
> + return false;
> }
>
> static int
> --
> 2.34.1
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] cifs: is_network_name_deleted should return a bool
2023-07-14 15:36 ` Steve French
@ 2023-07-17 3:13 ` Shyam Prasad N
0 siblings, 0 replies; 5+ messages in thread
From: Shyam Prasad N @ 2023-07-17 3:13 UTC (permalink / raw)
To: Steve French; +Cc: linux-cifs, bharathsm.hsk, pc, Shyam Prasad N
On Fri, Jul 14, 2023 at 9:07 PM Steve French <smfrench@gmail.com> wrote:
>
> should we have a dynamic trace point for this event as well?
We do print the status returned from the server in smb3_cmd_err. So
strictly speaking we don't loose this info.
But we can have a tracepoint to make it jump out.
>
> On Fri, Jul 14, 2023 at 3:56 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> >
> > Currently, is_network_name_deleted and it's implementations
> > do not return anything if the network name did get deleted.
> > So the function doesn't fully achieve what it advertizes.
> >
> > Changed the function to return a bool instead. It will now
> > return true if the error returned is STATUS_NETWORK_NAME_DELETED
> > and the share (tree id) was found to be connected. It returns
> > false otherwise.
> >
> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > ---
> > fs/smb/client/cifsglob.h | 2 +-
> > fs/smb/client/connect.c | 11 ++++++++---
> > fs/smb/client/smb2ops.c | 8 +++++---
> > 3 files changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> > index b212a4e16b39..bde9de6665a7 100644
> > --- a/fs/smb/client/cifsglob.h
> > +++ b/fs/smb/client/cifsglob.h
> > @@ -532,7 +532,7 @@ struct smb_version_operations {
> > /* Check for STATUS_IO_TIMEOUT */
> > bool (*is_status_io_timeout)(char *buf);
> > /* Check for STATUS_NETWORK_NAME_DELETED */
> > - void (*is_network_name_deleted)(char *buf, struct TCP_Server_Info *srv);
> > + bool (*is_network_name_deleted)(char *buf, struct TCP_Server_Info *srv);
> > };
> >
> > struct smb_version_values {
> > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> > index 87047bd38485..6756ce4ff641 100644
> > --- a/fs/smb/client/connect.c
> > +++ b/fs/smb/client/connect.c
> > @@ -1233,9 +1233,14 @@ cifs_demultiplex_thread(void *p)
> > if (mids[i] != NULL) {
> > mids[i]->resp_buf_size = server->pdu_size;
> >
> > - if (bufs[i] && server->ops->is_network_name_deleted)
> > - server->ops->is_network_name_deleted(bufs[i],
> > - server);
> > + if (bufs[i] != NULL) {
> > + if (server->ops->is_network_name_deleted &&
> > + server->ops->is_network_name_deleted(bufs[i],
> > + server)) {
> > + cifs_server_dbg(FYI,
> > + "Share deleted. Reconnect needed");
> > + }
> > + }
> >
> > if (!mids[i]->multiRsp || mids[i]->multiEnd)
> > mids[i]->callback(mids[i]);
> > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> > index 153b300621eb..d32477315abc 100644
> > --- a/fs/smb/client/smb2ops.c
> > +++ b/fs/smb/client/smb2ops.c
> > @@ -2391,7 +2391,7 @@ smb2_is_status_io_timeout(char *buf)
> > return false;
> > }
> >
> > -static void
> > +static bool
> > smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
> > {
> > struct smb2_hdr *shdr = (struct smb2_hdr *)buf;
> > @@ -2400,7 +2400,7 @@ smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
> > struct cifs_tcon *tcon;
> >
> > if (shdr->Status != STATUS_NETWORK_NAME_DELETED)
> > - return;
> > + return false;
> >
> > /* If server is a channel, select the primary channel */
> > pserver = CIFS_SERVER_IS_CHAN(server) ? server->primary_server : server;
> > @@ -2415,11 +2415,13 @@ smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
> > spin_unlock(&cifs_tcp_ses_lock);
> > pr_warn_once("Server share %s deleted.\n",
> > tcon->tree_name);
> > - return;
> > + return true;
> > }
> > }
> > }
> > spin_unlock(&cifs_tcp_ses_lock);
> > +
> > + return false;
> > }
> >
> > static int
> > --
> > 2.34.1
> >
>
>
> --
> Thanks,
>
> Steve
--
Regards,
Shyam
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-07-17 3:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-14 8:56 [PATCH 1/2] cifs: fix mid leak during reconnection after timeout threshold Shyam Prasad N
2023-07-14 8:56 ` [PATCH 2/2] cifs: is_network_name_deleted should return a bool Shyam Prasad N
2023-07-14 15:36 ` Steve French
2023-07-17 3:13 ` Shyam Prasad N
2023-07-14 16:30 ` Steve French
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.