cifs: Do not send echoes before Negotiate is complete
diff mbox

Message ID 20170416193724.4945-1-sprabhu@redhat.com
State New
Headers show

Commit Message

Sachin Prabhu April 16, 2017, 7:37 p.m. UTC
commit 4fcd1813e640 ("Fix reconnect to not defer smb3 session reconnect
long after socket reconnect") added support for Negotiate requests to
be initiated by echo calls.

To avoid delays in calling echo after a reconnect, I added the patch
introduced by the commit b8c600120fc8 ("Call echo service immediately
after socket reconnect").

This has however caused a regression with cifs shares which do not have
support for echo calls to trigger Negotiate requests. On connections
which need to call Negotiation, the echo calls trigger an error which
triggers a reconnect which in turn triggers another echo call. This
results in a loop which is only broken when an operation is performed on
the cifs share. For an idle share, it can DOS a server.

The patch uses the smb_operation can_echo() for cifs so that it is
called only if connection has been already been setup.

kernel bz: 194531

Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
---
 fs/cifs/smb1ops.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Jonathan Liu April 16, 2017, 10:59 p.m. UTC | #1
Hi Sachin,

On 17 April 2017 at 05:37, Sachin Prabhu <sprabhu@redhat.com> wrote:
> commit 4fcd1813e640 ("Fix reconnect to not defer smb3 session reconnect
> long after socket reconnect") added support for Negotiate requests to
> be initiated by echo calls.
>
> To avoid delays in calling echo after a reconnect, I added the patch
> introduced by the commit b8c600120fc8 ("Call echo service immediately
> after socket reconnect").
>
> This has however caused a regression with cifs shares which do not have
> support for echo calls to trigger Negotiate requests. On connections
> which need to call Negotiation, the echo calls trigger an error which
> triggers a reconnect which in turn triggers another echo call. This
> results in a loop which is only broken when an operation is performed on
> the cifs share. For an idle share, it can DOS a server.
>
> The patch uses the smb_operation can_echo() for cifs so that it is
> called only if connection has been already been setup.
>
> kernel bz: 194531
>
> Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
> ---
>  fs/cifs/smb1ops.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index cc93ba4..60a1979 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -1015,6 +1015,15 @@ cifs_dir_needs_close(struct cifsFileInfo *cfile)
>         return !cfile->srch_inf.endOfSearch && !cfile->invalidHandle;
>  }
>
> +static bool
> +cifs_can_echo(struct TCP_Server_Info *server)
> +{
> +       if (server->tcpStatus == CifsGood)
> +               return true;
> +
> +       return false;
> +}
> +
>  struct smb_version_operations smb1_operations = {
>         .send_cancel = send_nt_cancel,
>         .compare_fids = cifs_compare_fids,
> @@ -1049,6 +1058,7 @@ struct smb_version_operations smb1_operations = {
>         .get_dfs_refer = CIFSGetDFSRefer,
>         .qfs_tcon = cifs_qfs_tcon,
>         .is_path_accessible = cifs_is_path_accessible,
> +       .can_echo = cifs_can_echo,
>         .query_path_info = cifs_query_path_info,
>         .query_file_info = cifs_query_file_info,
>         .get_srv_inum = cifs_get_srv_inum,
> --
> 2.9.3
>

This fixes the issue for me.

Tested-by: Jonathan Liu <net147@gmail.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jonathan Liu April 16, 2017, 11:14 p.m. UTC | #2
Hi Sachin,

On 17 April 2017 at 05:37, Sachin Prabhu <sprabhu@redhat.com> wrote:
> commit 4fcd1813e640 ("Fix reconnect to not defer smb3 session reconnect
> long after socket reconnect") added support for Negotiate requests to
> be initiated by echo calls.
>
> To avoid delays in calling echo after a reconnect, I added the patch
> introduced by the commit b8c600120fc8 ("Call echo service immediately
> after socket reconnect").
>
> This has however caused a regression with cifs shares which do not have
> support for echo calls to trigger Negotiate requests. On connections
> which need to call Negotiation, the echo calls trigger an error which
> triggers a reconnect which in turn triggers another echo call. This
> results in a loop which is only broken when an operation is performed on
> the cifs share. For an idle share, it can DOS a server.
>
> The patch uses the smb_operation can_echo() for cifs so that it is
> called only if connection has been already been setup.
>
> kernel bz: 194531
>
> Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
> ---
>  fs/cifs/smb1ops.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index cc93ba4..60a1979 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -1015,6 +1015,15 @@ cifs_dir_needs_close(struct cifsFileInfo *cfile)
>         return !cfile->srch_inf.endOfSearch && !cfile->invalidHandle;
>  }
>
> +static bool
> +cifs_can_echo(struct TCP_Server_Info *server)
> +{
> +       if (server->tcpStatus == CifsGood)
> +               return true;
> +
> +       return false;

scripts/checkpatch.pl shows whitespace errors:
ERROR: code indent should use tabs where possible
#43: FILE: fs/cifs/smb1ops.c:1022:
+^I        return true;$

total: 1 errors, 0 warnings, 22 lines checked


Can be written more simply as:
return server->tcpStatus == CifsGood;

> +}
> +
>  struct smb_version_operations smb1_operations = {
>         .send_cancel = send_nt_cancel,
>         .compare_fids = cifs_compare_fids,
> @@ -1049,6 +1058,7 @@ struct smb_version_operations smb1_operations = {
>         .get_dfs_refer = CIFSGetDFSRefer,
>         .qfs_tcon = cifs_qfs_tcon,
>         .is_path_accessible = cifs_is_path_accessible,
> +       .can_echo = cifs_can_echo,
>         .query_path_info = cifs_query_path_info,
>         .query_file_info = cifs_query_file_info,
>         .get_srv_inum = cifs_get_srv_inum,
> --
> 2.9.3
>

Regards,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve French April 17, 2017, 4:28 a.m. UTC | #3
I fixed the checkpatch error (missing tab in Sachin's patch) and
merged into cifs-2.6.git

thx

On Sun, Apr 16, 2017 at 5:59 PM, Jonathan Liu <net147@gmail.com> wrote:
>
> Hi Sachin,
>
> On 17 April 2017 at 05:37, Sachin Prabhu <sprabhu@redhat.com> wrote:
> > commit 4fcd1813e640 ("Fix reconnect to not defer smb3 session reconnect
> > long after socket reconnect") added support for Negotiate requests to
> > be initiated by echo calls.
> >
> > To avoid delays in calling echo after a reconnect, I added the patch
> > introduced by the commit b8c600120fc8 ("Call echo service immediately
> > after socket reconnect").
> >
> > This has however caused a regression with cifs shares which do not have
> > support for echo calls to trigger Negotiate requests. On connections
> > which need to call Negotiation, the echo calls trigger an error which
> > triggers a reconnect which in turn triggers another echo call. This
> > results in a loop which is only broken when an operation is performed on
> > the cifs share. For an idle share, it can DOS a server.
> >
> > The patch uses the smb_operation can_echo() for cifs so that it is
> > called only if connection has been already been setup.
> >
> > kernel bz: 194531
> >
> > Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
> > ---
> >  fs/cifs/smb1ops.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> > index cc93ba4..60a1979 100644
> > --- a/fs/cifs/smb1ops.c
> > +++ b/fs/cifs/smb1ops.c
> > @@ -1015,6 +1015,15 @@ cifs_dir_needs_close(struct cifsFileInfo *cfile)
> >         return !cfile->srch_inf.endOfSearch && !cfile->invalidHandle;
> >  }
> >
> > +static bool
> > +cifs_can_echo(struct TCP_Server_Info *server)
> > +{
> > +       if (server->tcpStatus == CifsGood)
> > +               return true;
> > +
> > +       return false;
> > +}
> > +
> >  struct smb_version_operations smb1_operations = {
> >         .send_cancel = send_nt_cancel,
> >         .compare_fids = cifs_compare_fids,
> > @@ -1049,6 +1058,7 @@ struct smb_version_operations smb1_operations = {
> >         .get_dfs_refer = CIFSGetDFSRefer,
> >         .qfs_tcon = cifs_qfs_tcon,
> >         .is_path_accessible = cifs_is_path_accessible,
> > +       .can_echo = cifs_can_echo,
> >         .query_path_info = cifs_query_path_info,
> >         .query_file_info = cifs_query_file_info,
> >         .get_srv_inum = cifs_get_srv_inum,
> > --
> > 2.9.3
> >
>
> This fixes the issue for me.
>
> Tested-by: Jonathan Liu <net147@gmail.com>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Shilovsky April 17, 2017, 3:28 p.m. UTC | #4
2017-04-16 21:27 GMT-07:00 Steve French <smfrench@gmail.com>:
> I fixed the checkpatch error (missing tab in Sachin's patch) and merged into
> cifs-2.6.git
>
> thx
>
> On Sun, Apr 16, 2017 at 5:59 PM, Jonathan Liu <net147@gmail.com> wrote:
>>
>> Hi Sachin,
>>
>> On 17 April 2017 at 05:37, Sachin Prabhu <sprabhu@redhat.com> wrote:
>> > commit 4fcd1813e640 ("Fix reconnect to not defer smb3 session reconnect
>> > long after socket reconnect") added support for Negotiate requests to
>> > be initiated by echo calls.
>> >
>> > To avoid delays in calling echo after a reconnect, I added the patch
>> > introduced by the commit b8c600120fc8 ("Call echo service immediately
>> > after socket reconnect").
>> >
>> > This has however caused a regression with cifs shares which do not have
>> > support for echo calls to trigger Negotiate requests. On connections
>> > which need to call Negotiation, the echo calls trigger an error which
>> > triggers a reconnect which in turn triggers another echo call. This
>> > results in a loop which is only broken when an operation is performed on
>> > the cifs share. For an idle share, it can DOS a server.
>> >
>> > The patch uses the smb_operation can_echo() for cifs so that it is
>> > called only if connection has been already been setup.
>> >
>> > kernel bz: 194531
>> >
>> > Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
>> > ---
>> >  fs/cifs/smb1ops.c | 10 ++++++++++
>> >  1 file changed, 10 insertions(+)
>> >
>> > diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
>> > index cc93ba4..60a1979 100644
>> > --- a/fs/cifs/smb1ops.c
>> > +++ b/fs/cifs/smb1ops.c
>> > @@ -1015,6 +1015,15 @@ cifs_dir_needs_close(struct cifsFileInfo *cfile)
>> >         return !cfile->srch_inf.endOfSearch && !cfile->invalidHandle;
>> >  }
>> >
>> > +static bool
>> > +cifs_can_echo(struct TCP_Server_Info *server)
>> > +{
>> > +       if (server->tcpStatus == CifsGood)
>> > +               return true;
>> > +
>> > +       return false;
>> > +}
>> > +
>> >  struct smb_version_operations smb1_operations = {
>> >         .send_cancel = send_nt_cancel,
>> >         .compare_fids = cifs_compare_fids,
>> > @@ -1049,6 +1058,7 @@ struct smb_version_operations smb1_operations = {
>> >         .get_dfs_refer = CIFSGetDFSRefer,
>> >         .qfs_tcon = cifs_qfs_tcon,
>> >         .is_path_accessible = cifs_is_path_accessible,
>> > +       .can_echo = cifs_can_echo,
>> >         .query_path_info = cifs_query_path_info,
>> >         .query_file_info = cifs_query_file_info,
>> >         .get_srv_inum = cifs_get_srv_inum,
>> > --
>> > 2.9.3
>> >
>>
>> This fixes the issue for me.
>>
>> Tested-by: Jonathan Liu <net147@gmail.com>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
>
> --
> Thanks,
>
> Steve


I think this patch should go to stable as well.

--
Best regards,
Pavel Shilovsky
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve French April 17, 2017, 10:10 p.m. UTC | #5
Already in cifs-2.6.git (updated to include cc:stable)

On Mon, Apr 17, 2017 at 10:28 AM, Pavel Shilovsky <piastryyy@gmail.com> wrote:
> 2017-04-16 21:27 GMT-07:00 Steve French <smfrench@gmail.com>:
>> I fixed the checkpatch error (missing tab in Sachin's patch) and merged into
>> cifs-2.6.git
>>
>> thx
>>
>> On Sun, Apr 16, 2017 at 5:59 PM, Jonathan Liu <net147@gmail.com> wrote:
>>>
>>> Hi Sachin,
>>>
>>> On 17 April 2017 at 05:37, Sachin Prabhu <sprabhu@redhat.com> wrote:
>>> > commit 4fcd1813e640 ("Fix reconnect to not defer smb3 session reconnect
>>> > long after socket reconnect") added support for Negotiate requests to
>>> > be initiated by echo calls.
>>> >
>>> > To avoid delays in calling echo after a reconnect, I added the patch
>>> > introduced by the commit b8c600120fc8 ("Call echo service immediately
>>> > after socket reconnect").
>>> >
>>> > This has however caused a regression with cifs shares which do not have
>>> > support for echo calls to trigger Negotiate requests. On connections
>>> > which need to call Negotiation, the echo calls trigger an error which
>>> > triggers a reconnect which in turn triggers another echo call. This
>>> > results in a loop which is only broken when an operation is performed on
>>> > the cifs share. For an idle share, it can DOS a server.
>>> >
>>> > The patch uses the smb_operation can_echo() for cifs so that it is
>>> > called only if connection has been already been setup.
>>> >
>>> > kernel bz: 194531
>>> >
>>> > Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
>>> > ---
>>> >  fs/cifs/smb1ops.c | 10 ++++++++++
>>> >  1 file changed, 10 insertions(+)
>>> >
>>> > diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
>>> > index cc93ba4..60a1979 100644
>>> > --- a/fs/cifs/smb1ops.c
>>> > +++ b/fs/cifs/smb1ops.c
>>> > @@ -1015,6 +1015,15 @@ cifs_dir_needs_close(struct cifsFileInfo *cfile)
>>> >         return !cfile->srch_inf.endOfSearch && !cfile->invalidHandle;
>>> >  }
>>> >
>>> > +static bool
>>> > +cifs_can_echo(struct TCP_Server_Info *server)
>>> > +{
>>> > +       if (server->tcpStatus == CifsGood)
>>> > +               return true;
>>> > +
>>> > +       return false;
>>> > +}
>>> > +
>>> >  struct smb_version_operations smb1_operations = {
>>> >         .send_cancel = send_nt_cancel,
>>> >         .compare_fids = cifs_compare_fids,
>>> > @@ -1049,6 +1058,7 @@ struct smb_version_operations smb1_operations = {
>>> >         .get_dfs_refer = CIFSGetDFSRefer,
>>> >         .qfs_tcon = cifs_qfs_tcon,
>>> >         .is_path_accessible = cifs_is_path_accessible,
>>> > +       .can_echo = cifs_can_echo,
>>> >         .query_path_info = cifs_query_path_info,
>>> >         .query_file_info = cifs_query_file_info,
>>> >         .get_srv_inum = cifs_get_srv_inum,
>>> > --
>>> > 2.9.3
>>> >
>>>
>>> This fixes the issue for me.
>>>
>>> Tested-by: Jonathan Liu <net147@gmail.com>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>>
>> --
>> Thanks,
>>
>> Steve
>
>
> I think this patch should go to stable as well.
>
> --
> Best regards,
> Pavel Shilovsky

Patch
diff mbox

diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
index cc93ba4..60a1979 100644
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -1015,6 +1015,15 @@  cifs_dir_needs_close(struct cifsFileInfo *cfile)
 	return !cfile->srch_inf.endOfSearch && !cfile->invalidHandle;
 }
 
+static bool
+cifs_can_echo(struct TCP_Server_Info *server)
+{
+	if (server->tcpStatus == CifsGood)
+	        return true;
+
+	return false;
+}
+
 struct smb_version_operations smb1_operations = {
 	.send_cancel = send_nt_cancel,
 	.compare_fids = cifs_compare_fids,
@@ -1049,6 +1058,7 @@  struct smb_version_operations smb1_operations = {
 	.get_dfs_refer = CIFSGetDFSRefer,
 	.qfs_tcon = cifs_qfs_tcon,
 	.is_path_accessible = cifs_is_path_accessible,
+	.can_echo = cifs_can_echo,
 	.query_path_info = cifs_query_path_info,
 	.query_file_info = cifs_query_file_info,
 	.get_srv_inum = cifs_get_srv_inum,