Re: TCON reconnect during STATUS_NETWORK_NAME_DELETED

Re: TCON reconnect during STATUS_NETWORK_NAME_DELETED

[Pavel Shilovsky]

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 50fcb65920e8..fe99fb7307b2 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -504,6 +504,8 @@ struct smb_version_operations {
  loff_t (*llseek)(struct file *, struct cifs_tcon *, loff_t, int);
  /* Check for STATUS_IO_TIMEOUT */
  bool (*is_status_io_timeout)(char *buf);
+ /* Check for STATUS_NETWORK_NAME_DELETED */
+ void (*mark_tcon_reconnect) (char *buf, struct TCP_Server_Info *);
              ^^^
Let's follow the above style and name it

is_network_name_deleted()

 };

 struct smb_version_values {
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 10fe6d6d2dee..d4ffe2564e07 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -993,6 +993,8 @@ cifs_demultiplex_thread(void *p)
  if (mids[i] != NULL) {
  mids[i]->resp_buf_size = server->pdu_size;

+ server->ops->mark_tcon_reconnect(bufs[i],
+                  server);

How about SMB1? It doesn't have the callback, so it may fail here.

Btw, I think is_status_io timeout should be called in this loop for
every mid not outside the loop (sorry, missed that in the original
review). Could you please fix that separately?


  if (!mids[i]->multiRsp || mids[i]->multiEnd)
  mids[i]->callback(mids[i]);

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index f19274857292..23d238c0017a 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -2418,6 +2418,37 @@ smb2_is_status_io_timeout(char *buf)
  return false;
 }

+static void
+smb2_mark_tcon_reconnect(char *buf, struct TCP_Server_Info *server)
+{
+ struct smb2_sync_hdr *shdr = (struct smb2_sync_hdr *)buf;
+ struct list_head *tmp, *tmp1;
+ struct cifs_ses *ses;
+ struct cifs_tcon *tcon;
+ bool   find_tcon = false;
+
+ spin_lock(&cifs_tcp_ses_lock);
+ list_for_each(tmp, &server->smb_ses_list) {

Why looking for the TCON if in most cases Status is not
STATUS_NETWORK_NAME_DELETED?
If it is not, then the function should become a no-op and exit immediately.

+ ses = list_entry(tmp, struct cifs_ses, smb_ses_list);
+ list_for_each(tmp1, &ses->tcon_list) {
+ tcon = list_entry(tmp1, struct cifs_tcon, tcon_list);
+ if (tcon->tid == shdr->TreeId) {
+ find_tcon = true;
+ spin_unlock(&cifs_tcp_ses_lock);
+ goto reconnect;
+ }
+ }
+ }
+ spin_unlock(&cifs_tcp_ses_lock);
+
+reconnect:
+ if (find_tcon && (shdr->Status == STATUS_NETWORK_NAME_DELETED)) {
+ tcon->need_reconnect = true;

this should happen under the locks - there is no guarantee that tcon
is still valid outside the loop unless you take a reference which is
not needed here.


+ pr_warn_once("Server share %s deleted.\n",
+      tcon->treeName);

so, just printing logs should happen outside the locks.

+ }
+}
+
 static int
 smb2_oplock_response(struct cifs_tcon *tcon, struct cifs_fid *fid,
       struct cifsInodeInfo *cinode)
@@ -4605,6 +4636,8 @@ static void smb2_decrypt_offload(struct work_struct *work)
 #ifdef CONFIG_CIFS_STATS2
  mid->when_received = jiffies;
 #endif
+ dw->server->ops->mark_tcon_reconnect(dw->buf,
+                      dw->server);
  mid->callback(mid);
  } else {
  spin_lock(&GlobalMid_Lock);
@@ -5072,6 +5105,7 @@ struct smb_version_operations smb20_operations = {
  .fiemap = smb3_fiemap,
  .llseek = smb3_llseek,
  .is_status_io_timeout = smb2_is_status_io_timeout,
+ .mark_tcon_reconnect = smb2_mark_tcon_reconnect,
 };

 struct smb_version_operations smb21_operations = {
@@ -5173,6 +5207,7 @@ struct smb_version_operations smb21_operations = {
  .fiemap = smb3_fiemap,
  .llseek = smb3_llseek,
  .is_status_io_timeout = smb2_is_status_io_timeout,
+ .mark_tcon_reconnect = smb2_mark_tcon_reconnect,
 };

 struct smb_version_operations smb30_operations = {
@@ -5286,6 +5321,7 @@ struct smb_version_operations smb30_operations = {
  .fiemap = smb3_fiemap,
  .llseek = smb3_llseek,
  .is_status_io_timeout = smb2_is_status_io_timeout,
+ .mark_tcon_reconnect = smb2_mark_tcon_reconnect,
 };

 struct smb_version_operations smb311_operations = {
@@ -5399,6 +5435,7 @@ struct smb_version_operations smb311_operations = {
  .fiemap = smb3_fiemap,
  .llseek = smb3_llseek,
  .is_status_io_timeout = smb2_is_status_io_timeout,
+ .mark_tcon_reconnect = smb2_mark_tcon_reconnect,
 };

 struct smb_version_values smb20_values = {
-- 
2.25.1

--
Best regards,
Pavel Shilovsky

вт, 16 февр. 2021 г. в 02:54, Rohith <rohiths.msft@gmail.com>:
>
> Hi All,
>
>
>
> During migration or soft delete/undelete, Application doing recursive IO’s will fail until tcon is reconnected or session is established again.
>
>
>
> Attached patch addresses above mentioned issue. So, applications will recover automatically once share is undeleted.
>
>
>
> Regards,
>
> Rohith
>
>
>
> Sent from Mail for Windows 10
>
>

Re: TCON reconnect during STATUS_NETWORK_NAME_DELETED

[Pavel Shilovsky]

чт, 18 февр. 2021 г. в 08:10, Rohith <rohiths.msft@gmail.com>:
>
> Hi Pavel,
>
>
>
> Addressed review comments. Can you please take a look.
>
>
>
> >> Btw, I think is_status_io timeout should be called in this loop for
>
> >> every mid not outside the loop (sorry, missed that in the original
>
> >> review). Could you please fix that separately?
>
>
>
> Yes, will send out different patch for status_io_timeout.

Thanks!

The patch looks good. Please find my minor comments below:

1.
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 10fe6d6d2dee..b2f51546c2ef 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -993,6 +993,10 @@ cifs_demultiplex_thread(void *p)
  if (mids[i] != NULL) {
  mids[i]->resp_buf_size = server->pdu_size;

+ if (server->ops->is_network_name_deleted) {
+         server->ops->is_network_name_deleted(bufs[i],
+                              server);
+ }

^^^
remove extra {}

2.
+static void
+smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
+{
+ struct smb2_sync_hdr *shdr = (struct smb2_sync_hdr *)buf;

+ struct list_head *tmp, *tmp1;
+ struct cifs_ses *ses;
+ struct cifs_tcon *tcon;
+
+ if (shdr->Status == STATUS_NETWORK_NAME_DELETED) {

^^^
let's do the opposite check: if status is not
STATUS_NETWORK_NAME_DELETED then return immediately from the function.
This will remove overall indentation for the nested for loops.

3.
@@ -4605,6 +4632,10 @@ static void smb2_decrypt_offload(struct
work_struct *work)
 #ifdef CONFIG_CIFS_STATS2
  mid->when_received = jiffies;
 #endif
+ if (dw->server->ops->is_network_name_deleted) {
+         dw->server->ops->is_network_name_deleted(dw->buf,
+                                  dw->server);
+ }

^^^
remove extra {}

--
Best regards,
Pavel Shilovsky

Re: TCON reconnect during STATUS_NETWORK_NAME_DELETED

[Pavel Shilovsky]

Both patches look good. Thanks!

Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
--
Best regards,
Pavel Shilovsky

вс, 21 февр. 2021 г. в 22:44, Rohith <rohiths.msft@gmail.com>:
>
> Thanks Steve. Will make sure to run checkpatch for further patches.
>
>
>
> Regards,
>
> Rohith
>
>
>
> Sent from Mail for Windows 10
>
>
>
> From: Steve French
> Sent: Sunday, February 21, 2021 6:26 AM
> To: Pavel Shilovsky
> Cc: Rohith; linux-cifs@vger.kernel.org; Shyam Prasad N; sribhat.msa@outlook.com
> Subject: Re: TCON reconnect during STATUS_NETWORK_NAME_DELETED
>
>
>
> The remaining suggestion of Pavel's is included in this trivial cleanup.
>
>
>
> Trivial change to clarify code in smb2_is_network_name_deleted
>
> Suggested-by: Pavel Shilovsky <pshilov@microsoft.com>
> Signed-off-by: Steve French <stfrench@microsoft.com>
> ---
>  fs/cifs/smb2ops.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index aee9da88a333..b2191babce26 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -2459,23 +2459,24 @@ smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
>   struct cifs_ses *ses;
>   struct cifs_tcon *tcon;
>
> - if (shdr->Status == STATUS_NETWORK_NAME_DELETED) {
> - spin_lock(&cifs_tcp_ses_lock);
> - list_for_each(tmp, &server->smb_ses_list) {
> - ses = list_entry(tmp, struct cifs_ses, smb_ses_list);
> - list_for_each(tmp1, &ses->tcon_list) {
> - tcon = list_entry(tmp1, struct cifs_tcon, tcon_list);
> - if (tcon->tid == shdr->TreeId) {
> - tcon->need_reconnect = true;
> - spin_unlock(&cifs_tcp_ses_lock);
> - pr_warn_once("Server share %s deleted.\n",
> -     tcon->treeName);
> - return;
> - }
> + if (shdr->Status != STATUS_NETWORK_NAME_DELETED)
> + return;
> +
> + spin_lock(&cifs_tcp_ses_lock);
> + list_for_each(tmp, &server->smb_ses_list) {
> + ses = list_entry(tmp, struct cifs_ses, smb_ses_list);
> + list_for_each(tmp1, &ses->tcon_list) {
> + tcon = list_entry(tmp1, struct cifs_tcon, tcon_list);
> + if (tcon->tid == shdr->TreeId) {
> + tcon->need_reconnect = true;
> + spin_unlock(&cifs_tcp_ses_lock);
> + pr_warn_once("Server share %s deleted.\n",
> +     tcon->treeName);
> + return;
>   }
>   }
> - spin_unlock(&cifs_tcp_ses_lock);
>   }
> + spin_unlock(&cifs_tcp_ses_lock);
>  }
>
>  static int
>
>
>
> On Sat, Feb 20, 2021 at 5:38 PM Steve French <smfrench@gmail.com> wrote:
>
> I corrected various checkpatch errors - including two of those Pavel noted, and tentatively merged into cifs-2.6.git for-next.  See attached updated patch.
>
>
>
> Will fix the indentation next.
>
>
>
> On Sat, Feb 20, 2021 at 12:18 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> чт, 18 февр. 2021 г. в 08:10, Rohith <rohiths.msft@gmail.com>:
> >
> > Hi Pavel,
> >
> >
> >
> > Addressed review comments. Can you please take a look.
> >
> >
> >
> > >> Btw, I think is_status_io timeout should be called in this loop for
> >
> > >> every mid not outside the loop (sorry, missed that in the original
> >
> > >> review). Could you please fix that separately?
> >
> >
> >
> > Yes, will send out different patch for status_io_timeout.
>
> Thanks!
>
> The patch looks good. Please find my minor comments below:
>
> 1.
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 10fe6d6d2dee..b2f51546c2ef 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -993,6 +993,10 @@ cifs_demultiplex_thread(void *p)
>   if (mids[i] != NULL) {
>   mids[i]->resp_buf_size = server->pdu_size;
>
> + if (server->ops->is_network_name_deleted) {
> +         server->ops->is_network_name_deleted(bufs[i],
> +                              server);
> + }
>
> ^^^
> remove extra {}
>
> 2.
> +static void
> +smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
> +{
> + struct smb2_sync_hdr *shdr = (struct smb2_sync_hdr *)buf;
>
> + struct list_head *tmp, *tmp1;
> + struct cifs_ses *ses;
> + struct cifs_tcon *tcon;
> +
> + if (shdr->Status == STATUS_NETWORK_NAME_DELETED) {
>
> ^^^
> let's do the opposite check: if status is not
> STATUS_NETWORK_NAME_DELETED then return immediately from the function.
> This will remove overall indentation for the nested for loops.
>
> 3.
> @@ -4605,6 +4632,10 @@ static void smb2_decrypt_offload(struct
> work_struct *work)
>  #ifdef CONFIG_CIFS_STATS2
>   mid->when_received = jiffies;
>  #endif
> + if (dw->server->ops->is_network_name_deleted) {
> +         dw->server->ops->is_network_name_deleted(dw->buf,
> +                                  dw->server);
> + }
>
> ^^^
> remove extra {}
>
> --
> Best regards,
> Pavel Shilovsky
>
>
>
>
> --
>
> Thanks,
>
> Steve
>
>
>
>
> --
>
> Thanks,
>
> Steve
>
>

Re: TCON reconnect during STATUS_NETWORK_NAME_DELETED

[Shyam Prasad N]

Looks good to me.
Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>

On Mon, Feb 22, 2021 at 9:21 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> Both patches look good. Thanks!
>
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> --
> Best regards,
> Pavel Shilovsky
>
> вс, 21 февр. 2021 г. в 22:44, Rohith <rohiths.msft@gmail.com>:
> >
> > Thanks Steve. Will make sure to run checkpatch for further patches.
> >
> >
> >
> > Regards,
> >
> > Rohith
> >
> >
> >
> > Sent from Mail for Windows 10
> >
> >
> >
> > From: Steve French
> > Sent: Sunday, February 21, 2021 6:26 AM
> > To: Pavel Shilovsky
> > Cc: Rohith; linux-cifs@vger.kernel.org; Shyam Prasad N; sribhat.msa@outlook.com
> > Subject: Re: TCON reconnect during STATUS_NETWORK_NAME_DELETED
> >
> >
> >
> > The remaining suggestion of Pavel's is included in this trivial cleanup.
> >
> >
> >
> > Trivial change to clarify code in smb2_is_network_name_deleted
> >
> > Suggested-by: Pavel Shilovsky <pshilov@microsoft.com>
> > Signed-off-by: Steve French <stfrench@microsoft.com>
> > ---
> >  fs/cifs/smb2ops.c | 29 +++++++++++++++--------------
> >  1 file changed, 15 insertions(+), 14 deletions(-)
> >
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index aee9da88a333..b2191babce26 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -2459,23 +2459,24 @@ smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
> >   struct cifs_ses *ses;
> >   struct cifs_tcon *tcon;
> >
> > - if (shdr->Status == STATUS_NETWORK_NAME_DELETED) {
> > - spin_lock(&cifs_tcp_ses_lock);
> > - list_for_each(tmp, &server->smb_ses_list) {
> > - ses = list_entry(tmp, struct cifs_ses, smb_ses_list);
> > - list_for_each(tmp1, &ses->tcon_list) {
> > - tcon = list_entry(tmp1, struct cifs_tcon, tcon_list);
> > - if (tcon->tid == shdr->TreeId) {
> > - tcon->need_reconnect = true;
> > - spin_unlock(&cifs_tcp_ses_lock);
> > - pr_warn_once("Server share %s deleted.\n",
> > -     tcon->treeName);
> > - return;
> > - }
> > + if (shdr->Status != STATUS_NETWORK_NAME_DELETED)
> > + return;
> > +
> > + spin_lock(&cifs_tcp_ses_lock);
> > + list_for_each(tmp, &server->smb_ses_list) {
> > + ses = list_entry(tmp, struct cifs_ses, smb_ses_list);
> > + list_for_each(tmp1, &ses->tcon_list) {
> > + tcon = list_entry(tmp1, struct cifs_tcon, tcon_list);
> > + if (tcon->tid == shdr->TreeId) {
> > + tcon->need_reconnect = true;
> > + spin_unlock(&cifs_tcp_ses_lock);
> > + pr_warn_once("Server share %s deleted.\n",
> > +     tcon->treeName);
> > + return;
> >   }
> >   }
> > - spin_unlock(&cifs_tcp_ses_lock);
> >   }
> > + spin_unlock(&cifs_tcp_ses_lock);
> >  }
> >
> >  static int
> >
> >
> >
> > On Sat, Feb 20, 2021 at 5:38 PM Steve French <smfrench@gmail.com> wrote:
> >
> > I corrected various checkpatch errors - including two of those Pavel noted, and tentatively merged into cifs-2.6.git for-next.  See attached updated patch.
> >
> >
> >
> > Will fix the indentation next.
> >
> >
> >
> > On Sat, Feb 20, 2021 at 12:18 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
> > чт, 18 февр. 2021 г. в 08:10, Rohith <rohiths.msft@gmail.com>:
> > >
> > > Hi Pavel,
> > >
> > >
> > >
> > > Addressed review comments. Can you please take a look.
> > >
> > >
> > >
> > > >> Btw, I think is_status_io timeout should be called in this loop for
> > >
> > > >> every mid not outside the loop (sorry, missed that in the original
> > >
> > > >> review). Could you please fix that separately?
> > >
> > >
> > >
> > > Yes, will send out different patch for status_io_timeout.
> >
> > Thanks!
> >
> > The patch looks good. Please find my minor comments below:
> >
> > 1.
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index 10fe6d6d2dee..b2f51546c2ef 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -993,6 +993,10 @@ cifs_demultiplex_thread(void *p)
> >   if (mids[i] != NULL) {
> >   mids[i]->resp_buf_size = server->pdu_size;
> >
> > + if (server->ops->is_network_name_deleted) {
> > +         server->ops->is_network_name_deleted(bufs[i],
> > +                              server);
> > + }
> >
> > ^^^
> > remove extra {}
> >
> > 2.
> > +static void
> > +smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
> > +{
> > + struct smb2_sync_hdr *shdr = (struct smb2_sync_hdr *)buf;
> >
> > + struct list_head *tmp, *tmp1;
> > + struct cifs_ses *ses;
> > + struct cifs_tcon *tcon;
> > +
> > + if (shdr->Status == STATUS_NETWORK_NAME_DELETED) {
> >
> > ^^^
> > let's do the opposite check: if status is not
> > STATUS_NETWORK_NAME_DELETED then return immediately from the function.
> > This will remove overall indentation for the nested for loops.
> >
> > 3.
> > @@ -4605,6 +4632,10 @@ static void smb2_decrypt_offload(struct
> > work_struct *work)
> >  #ifdef CONFIG_CIFS_STATS2
> >   mid->when_received = jiffies;
> >  #endif
> > + if (dw->server->ops->is_network_name_deleted) {
> > +         dw->server->ops->is_network_name_deleted(dw->buf,
> > +                                  dw->server);
> > + }
> >
> > ^^^
> > remove extra {}
> >
> > --
> > Best regards,
> > Pavel Shilovsky
> >
> >
> >
> >
> > --
> >
> > Thanks,
> >
> > Steve
> >
> >
> >
> >
> > --
> >
> > Thanks,
> >
> > Steve
> >
> >



-- 
Regards,
Shyam

Re: TCON reconnect during STATUS_NETWORK_NAME_DELETED

[Steve French]

merged this patch in with the one it fixes and pushed to cifs-2.6.git for-next

Running buildbot on them now:

http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/509

On Tue, Feb 23, 2021 at 3:49 AM Rohith <rohiths.msft@gmail.com> wrote:
>
> Hi Steve,
>
>
>
> Please find the attached patch which will fix the regression caused by previous patch.
>
> During read path when pdu length is greater than CIFSMaxBufSize, “bufs” pointer is not updated during “receive_transform”. So, oops occurred while accessing “bufs” pointer.
>
>
>
> I ran cifs/100 test on my local machine:
>
> dd if=/dev/zero of=$TEST_DIR/$$/big-file bs=4M count=1
> dd if=$TEST_DIR/$$/big-file of=/dev/null bs=4M count=1
>
>
>
>
>
> Regards,
>
> Rohith
>
>
>
> Sent from Mail for Windows 10
>
>
>
> From: Shyam Prasad N
> Sent: Monday, February 22, 2021 10:53 PM
> To: Pavel Shilovsky
> Cc: Rohith; Steve French; linux-cifs@vger.kernel.org; sribhat.msa@outlook.com
> Subject: Re: TCON reconnect during STATUS_NETWORK_NAME_DELETED
>
>
>
> Looks good to me.
>
> Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
>
>
>
> On Mon, Feb 22, 2021 at 9:21 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> >
>
> > Both patches look good. Thanks!
>
> >
>
> > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
>
> > --
>
> > Best regards,
>
> > Pavel Shilovsky
>
> >
>
> > вс, 21 февр. 2021 г. в 22:44, Rohith <rohiths.msft@gmail.com>:
>
> > >
>
> > > Thanks Steve. Will make sure to run checkpatch for further patches.
>
> > >
>
> > >
>
> > >
>
> > > Regards,
>
> > >
>
> > > Rohith
>
> > >
>
> > >
>
> > >
>
> > > Sent from Mail for Windows 10
>
> > >
>
> > >
>
> > >
>
> > > From: Steve French
>
> > > Sent: Sunday, February 21, 2021 6:26 AM
>
> > > To: Pavel Shilovsky
>
> > > Cc: Rohith; linux-cifs@vger.kernel.org; Shyam Prasad N; sribhat.msa@outlook.com
>
> > > Subject: Re: TCON reconnect during STATUS_NETWORK_NAME_DELETED
>
> > >
>
> > >
>
> > >
>
> > > The remaining suggestion of Pavel's is included in this trivial cleanup.
>
> > >
>
> > >
>
> > >
>
> > > Trivial change to clarify code in smb2_is_network_name_deleted
>
> > >
>
> > > Suggested-by: Pavel Shilovsky <pshilov@microsoft.com>
>
> > > Signed-off-by: Steve French <stfrench@microsoft.com>
>
> > > ---
>
> > >  fs/cifs/smb2ops.c | 29 +++++++++++++++--------------
>
> > >  1 file changed, 15 insertions(+), 14 deletions(-)
>
> > >
>
> > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
>
> > > index aee9da88a333..b2191babce26 100644
>
> > > --- a/fs/cifs/smb2ops.c
>
> > > +++ b/fs/cifs/smb2ops.c
>
> > > @@ -2459,23 +2459,24 @@ smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
>
> > >   struct cifs_ses *ses;
>
> > >   struct cifs_tcon *tcon;
>
> > >
>
> > > - if (shdr->Status == STATUS_NETWORK_NAME_DELETED) {
>
> > > - spin_lock(&cifs_tcp_ses_lock);
>
> > > - list_for_each(tmp, &server->smb_ses_list) {
>
> > > - ses = list_entry(tmp, struct cifs_ses, smb_ses_list);
>
> > > - list_for_each(tmp1, &ses->tcon_list) {
>
> > > - tcon = list_entry(tmp1, struct cifs_tcon, tcon_list);
>
> > > - if (tcon->tid == shdr->TreeId) {
>
> > > - tcon->need_reconnect = true;
>
> > > - spin_unlock(&cifs_tcp_ses_lock);
>
> > > - pr_warn_once("Server share %s deleted.\n",
>
> > > -     tcon->treeName);
>
> > > - return;
>
> > > - }
>
> > > + if (shdr->Status != STATUS_NETWORK_NAME_DELETED)
>
> > > + return;
>
> > > +
>
> > > + spin_lock(&cifs_tcp_ses_lock);
>
> > > + list_for_each(tmp, &server->smb_ses_list) {
>
> > > + ses = list_entry(tmp, struct cifs_ses, smb_ses_list);
>
> > > + list_for_each(tmp1, &ses->tcon_list) {
>
> > > + tcon = list_entry(tmp1, struct cifs_tcon, tcon_list);
>
> > > + if (tcon->tid == shdr->TreeId) {
>
> > > + tcon->need_reconnect = true;
>
> > > + spin_unlock(&cifs_tcp_ses_lock);
>
> > > + pr_warn_once("Server share %s deleted.\n",
>
> > > +     tcon->treeName);
>
> > > + return;
>
> > >   }
>
> > >   }
>
> > > - spin_unlock(&cifs_tcp_ses_lock);
>
> > >   }
>
> > > + spin_unlock(&cifs_tcp_ses_lock);
>
> > >  }
>
> > >
>
> > >  static int
>
> > >
>
> > >
>
> > >
>
> > > On Sat, Feb 20, 2021 at 5:38 PM Steve French <smfrench@gmail.com> wrote:
>
> > >
>
> > > I corrected various checkpatch errors - including two of those Pavel noted, and tentatively merged into cifs-2.6.git for-next.  See attached updated patch.
>
> > >
>
> > >
>
> > >
>
> > > Will fix the indentation next.
>
> > >
>
> > >
>
> > >
>
> > > On Sat, Feb 20, 2021 at 12:18 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> > >
>
> > > чт, 18 февр. 2021 г. в 08:10, Rohith <rohiths.msft@gmail.com>:
>
> > > >
>
> > > > Hi Pavel,
>
> > > >
>
> > > >
>
> > > >
>
> > > > Addressed review comments. Can you please take a look.
>
> > > >
>
> > > >
>
> > > >
>
> > > > >> Btw, I think is_status_io timeout should be called in this loop for
>
> > > >
>
> > > > >> every mid not outside the loop (sorry, missed that in the original
>
> > > >
>
> > > > >> review). Could you please fix that separately?
>
> > > >
>
> > > >
>
> > > >
>
> > > > Yes, will send out different patch for status_io_timeout.
>
> > >
>
> > > Thanks!
>
> > >
>
> > > The patch looks good. Please find my minor comments below:
>
> > >
>
> > > 1.
>
> > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>
> > > index 10fe6d6d2dee..b2f51546c2ef 100644
>
> > > --- a/fs/cifs/connect.c
>
> > > +++ b/fs/cifs/connect.c
>
> > > @@ -993,6 +993,10 @@ cifs_demultiplex_thread(void *p)
>
> > >   if (mids[i] != NULL) {
>
> > >   mids[i]->resp_buf_size = server->pdu_size;
>
> > >
>
> > > + if (server->ops->is_network_name_deleted) {
>
> > > +         server->ops->is_network_name_deleted(bufs[i],
>
> > > +                              server);
>
> > > + }
>
> > >
>
> > > ^^^
>
> > > remove extra {}
>
> > >
>
> > > 2.
>
> > > +static void
>
> > > +smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
>
> > > +{
>
> > > + struct smb2_sync_hdr *shdr = (struct smb2_sync_hdr *)buf;
>
> > >
>
> > > + struct list_head *tmp, *tmp1;
>
> > > + struct cifs_ses *ses;
>
> > > + struct cifs_tcon *tcon;
>
> > > +
>
> > > + if (shdr->Status == STATUS_NETWORK_NAME_DELETED) {
>
> > >
>
> > > ^^^
>
> > > let's do the opposite check: if status is not
>
> > > STATUS_NETWORK_NAME_DELETED then return immediately from the function.
>
> > > This will remove overall indentation for the nested for loops.
>
> > >
>
> > > 3.
>
> > > @@ -4605,6 +4632,10 @@ static void smb2_decrypt_offload(struct
>
> > > work_struct *work)
>
> > >  #ifdef CONFIG_CIFS_STATS2
>
> > >   mid->when_received = jiffies;
>
> > >  #endif
>
> > > + if (dw->server->ops->is_network_name_deleted) {
>
> > > +         dw->server->ops->is_network_name_deleted(dw->buf,
>
> > > +                                  dw->server);
>
> > > + }
>
> > >
>
> > > ^^^
>
> > > remove extra {}
>
> > >
>
> > > --
>
> > > Best regards,
>
> > > Pavel Shilovsky
>
> > >
>
> > >
>
> > >
>
> > >
>
> > > --
>
> > >
>
> > > Thanks,
>
> > >
>
> > > Steve
>
> > >
>
> > >
>
> > >
>
> > >
>
> > > --
>
> > >
>
> > > Thanks,
>
> > >
>
> > > Steve
>
> > >
>
> > >
>
>
>
>
>
>
>
> --
>
> Regards,
>
> Shyam
>
>



-- 
Thanks,

Steve

Re: TCON reconnect during STATUS_NETWORK_NAME_DELETED

[Pavel Shilovsky]

The fix looks correct. Thanks!
--
Best regards,
Pavel Shilovsky

вт, 23 февр. 2021 г. в 02:21, Steve French <smfrench@gmail.com>:
>
> merged this patch in with the one it fixes and pushed to cifs-2.6.git for-next
>
> Running buildbot on them now:
>
> http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/509
>
> On Tue, Feb 23, 2021 at 3:49 AM Rohith <rohiths.msft@gmail.com> wrote:
> >
> > Hi Steve,
> >
> >
> >
> > Please find the attached patch which will fix the regression caused by previous patch.
> >
> > During read path when pdu length is greater than CIFSMaxBufSize, “bufs” pointer is not updated during “receive_transform”. So, oops occurred while accessing “bufs” pointer.
> >
> >
> >
> > I ran cifs/100 test on my local machine:
> >
> > dd if=/dev/zero of=$TEST_DIR/$$/big-file bs=4M count=1
> > dd if=$TEST_DIR/$$/big-file of=/dev/null bs=4M count=1
> >
> >
> >
> >
> >
> > Regards,
> >
> > Rohith
> >
> >
> >
> > Sent from Mail for Windows 10
> >
> >
> >
> > From: Shyam Prasad N
> > Sent: Monday, February 22, 2021 10:53 PM
> > To: Pavel Shilovsky
> > Cc: Rohith; Steve French; linux-cifs@vger.kernel.org; sribhat.msa@outlook.com
> > Subject: Re: TCON reconnect during STATUS_NETWORK_NAME_DELETED
> >
> >
> >
> > Looks good to me.
> >
> > Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
> >
> >
> >
> > On Mon, Feb 22, 2021 at 9:21 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
> > >
> >
> > > Both patches look good. Thanks!
> >
> > >
> >
> > > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> >
> > > --
> >
> > > Best regards,
> >
> > > Pavel Shilovsky
> >
> > >
> >
> > > вс, 21 февр. 2021 г. в 22:44, Rohith <rohiths.msft@gmail.com>:
> >
> > > >
> >
> > > > Thanks Steve. Will make sure to run checkpatch for further patches.
> >
> > > >
> >
> > > >
> >
> > > >
> >
> > > > Regards,
> >
> > > >
> >
> > > > Rohith
> >
> > > >
> >
> > > >
> >
> > > >
> >
> > > > Sent from Mail for Windows 10
> >
> > > >
> >
> > > >
> >
> > > >
> >
> > > > From: Steve French
> >
> > > > Sent: Sunday, February 21, 2021 6:26 AM
> >
> > > > To: Pavel Shilovsky
> >
> > > > Cc: Rohith; linux-cifs@vger.kernel.org; Shyam Prasad N; sribhat.msa@outlook.com
> >
> > > > Subject: Re: TCON reconnect during STATUS_NETWORK_NAME_DELETED
> >
> > > >
> >
> > > >
> >
> > > >
> >
> > > > The remaining suggestion of Pavel's is included in this trivial cleanup.
> >
> > > >
> >
> > > >
> >
> > > >
> >
> > > > Trivial change to clarify code in smb2_is_network_name_deleted
> >
> > > >
> >
> > > > Suggested-by: Pavel Shilovsky <pshilov@microsoft.com>
> >
> > > > Signed-off-by: Steve French <stfrench@microsoft.com>
> >
> > > > ---
> >
> > > >  fs/cifs/smb2ops.c | 29 +++++++++++++++--------------
> >
> > > >  1 file changed, 15 insertions(+), 14 deletions(-)
> >
> > > >
> >
> > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> >
> > > > index aee9da88a333..b2191babce26 100644
> >
> > > > --- a/fs/cifs/smb2ops.c
> >
> > > > +++ b/fs/cifs/smb2ops.c
> >
> > > > @@ -2459,23 +2459,24 @@ smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
> >
> > > >   struct cifs_ses *ses;
> >
> > > >   struct cifs_tcon *tcon;
> >
> > > >
> >
> > > > - if (shdr->Status == STATUS_NETWORK_NAME_DELETED) {
> >
> > > > - spin_lock(&cifs_tcp_ses_lock);
> >
> > > > - list_for_each(tmp, &server->smb_ses_list) {
> >
> > > > - ses = list_entry(tmp, struct cifs_ses, smb_ses_list);
> >
> > > > - list_for_each(tmp1, &ses->tcon_list) {
> >
> > > > - tcon = list_entry(tmp1, struct cifs_tcon, tcon_list);
> >
> > > > - if (tcon->tid == shdr->TreeId) {
> >
> > > > - tcon->need_reconnect = true;
> >
> > > > - spin_unlock(&cifs_tcp_ses_lock);
> >
> > > > - pr_warn_once("Server share %s deleted.\n",
> >
> > > > -     tcon->treeName);
> >
> > > > - return;
> >
> > > > - }
> >
> > > > + if (shdr->Status != STATUS_NETWORK_NAME_DELETED)
> >
> > > > + return;
> >
> > > > +
> >
> > > > + spin_lock(&cifs_tcp_ses_lock);
> >
> > > > + list_for_each(tmp, &server->smb_ses_list) {
> >
> > > > + ses = list_entry(tmp, struct cifs_ses, smb_ses_list);
> >
> > > > + list_for_each(tmp1, &ses->tcon_list) {
> >
> > > > + tcon = list_entry(tmp1, struct cifs_tcon, tcon_list);
> >
> > > > + if (tcon->tid == shdr->TreeId) {
> >
> > > > + tcon->need_reconnect = true;
> >
> > > > + spin_unlock(&cifs_tcp_ses_lock);
> >
> > > > + pr_warn_once("Server share %s deleted.\n",
> >
> > > > +     tcon->treeName);
> >
> > > > + return;
> >
> > > >   }
> >
> > > >   }
> >
> > > > - spin_unlock(&cifs_tcp_ses_lock);
> >
> > > >   }
> >
> > > > + spin_unlock(&cifs_tcp_ses_lock);
> >
> > > >  }
> >
> > > >
> >
> > > >  static int
> >
> > > >
> >
> > > >
> >
> > > >
> >
> > > > On Sat, Feb 20, 2021 at 5:38 PM Steve French <smfrench@gmail.com> wrote:
> >
> > > >
> >
> > > > I corrected various checkpatch errors - including two of those Pavel noted, and tentatively merged into cifs-2.6.git for-next.  See attached updated patch.
> >
> > > >
> >
> > > >
> >
> > > >
> >
> > > > Will fix the indentation next.
> >
> > > >
> >
> > > >
> >
> > > >
> >
> > > > On Sat, Feb 20, 2021 at 12:18 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
> > > >
> >
> > > > чт, 18 февр. 2021 г. в 08:10, Rohith <rohiths.msft@gmail.com>:
> >
> > > > >
> >
> > > > > Hi Pavel,
> >
> > > > >
> >
> > > > >
> >
> > > > >
> >
> > > > > Addressed review comments. Can you please take a look.
> >
> > > > >
> >
> > > > >
> >
> > > > >
> >
> > > > > >> Btw, I think is_status_io timeout should be called in this loop for
> >
> > > > >
> >
> > > > > >> every mid not outside the loop (sorry, missed that in the original
> >
> > > > >
> >
> > > > > >> review). Could you please fix that separately?
> >
> > > > >
> >
> > > > >
> >
> > > > >
> >
> > > > > Yes, will send out different patch for status_io_timeout.
> >
> > > >
> >
> > > > Thanks!
> >
> > > >
> >
> > > > The patch looks good. Please find my minor comments below:
> >
> > > >
> >
> > > > 1.
> >
> > > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >
> > > > index 10fe6d6d2dee..b2f51546c2ef 100644
> >
> > > > --- a/fs/cifs/connect.c
> >
> > > > +++ b/fs/cifs/connect.c
> >
> > > > @@ -993,6 +993,10 @@ cifs_demultiplex_thread(void *p)
> >
> > > >   if (mids[i] != NULL) {
> >
> > > >   mids[i]->resp_buf_size = server->pdu_size;
> >
> > > >
> >
> > > > + if (server->ops->is_network_name_deleted) {
> >
> > > > +         server->ops->is_network_name_deleted(bufs[i],
> >
> > > > +                              server);
> >
> > > > + }
> >
> > > >
> >
> > > > ^^^
> >
> > > > remove extra {}
> >
> > > >
> >
> > > > 2.
> >
> > > > +static void
> >
> > > > +smb2_is_network_name_deleted(char *buf, struct TCP_Server_Info *server)
> >
> > > > +{
> >
> > > > + struct smb2_sync_hdr *shdr = (struct smb2_sync_hdr *)buf;
> >
> > > >
> >
> > > > + struct list_head *tmp, *tmp1;
> >
> > > > + struct cifs_ses *ses;
> >
> > > > + struct cifs_tcon *tcon;
> >
> > > > +
> >
> > > > + if (shdr->Status == STATUS_NETWORK_NAME_DELETED) {
> >
> > > >
> >
> > > > ^^^
> >
> > > > let's do the opposite check: if status is not
> >
> > > > STATUS_NETWORK_NAME_DELETED then return immediately from the function.
> >
> > > > This will remove overall indentation for the nested for loops.
> >
> > > >
> >
> > > > 3.
> >
> > > > @@ -4605,6 +4632,10 @@ static void smb2_decrypt_offload(struct
> >
> > > > work_struct *work)
> >
> > > >  #ifdef CONFIG_CIFS_STATS2
> >
> > > >   mid->when_received = jiffies;
> >
> > > >  #endif
> >
> > > > + if (dw->server->ops->is_network_name_deleted) {
> >
> > > > +         dw->server->ops->is_network_name_deleted(dw->buf,
> >
> > > > +                                  dw->server);
> >
> > > > + }
> >
> > > >
> >
> > > > ^^^
> >
> > > > remove extra {}
> >
> > > >
> >
> > > > --
> >
> > > > Best regards,
> >
> > > > Pavel Shilovsky
> >
> > > >
> >
> > > >
> >
> > > >
> >
> > > >
> >
> > > > --
> >
> > > >
> >
> > > > Thanks,
> >
> > > >
> >
> > > > Steve
> >
> > > >
> >
> > > >
> >
> > > >
> >
> > > >
> >
> > > > --
> >
> > > >
> >
> > > > Thanks,
> >
> > > >
> >
> > > > Steve
> >
> > > >
> >
> > > >
> >
> >
> >
> >
> >
> >
> >
> > --
> >
> > Regards,
> >
> > Shyam
> >
> >
>
>
>
> --
> Thanks,
>
> Steve