| Message ID | 20230714085634.10808-2-sprasad@microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] cifs: fix mid leak during reconnection after timeout threshold | expand |
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 >
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 >
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
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
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(-)