All of lore.kernel.org
 help / color / mirror / Atom feed
* Interop Issue: SMB2+ async replies, and the kernel, Samba side fix enclosed.
@ 2016-02-23 12:55 Ira Cooper
       [not found] ` <CAAPGDwLnp90t7wiRb_tw-Md0jv-8w0Ye66jNcsRNioganVe2xg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
       [not found] ` <CAKywueSMFrvreB8JMtwj8kt1BY1Yyq-Guy3Q3=wZFzuLCCOjZA@mail.gmail.com>
  0 siblings, 2 replies; 15+ messages in thread
From: Ira Cooper @ 2016-02-23 12:55 UTC (permalink / raw)
  To: Samba Technical, linux-cifs; +Cc: sfrench

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

If the server sends an interim response, then the real response, the real
response, is handled by standard_receive3() in the kernel, instead of the
proper function, and this causes a disconnect.  If there isn't a
disconnect, I believe the reply will just be discarded if I understand the
code correctly.  (That is a big if here ;) )

I've written a patch for Samba to stop sending interim replies on SMB2_READ
and SMB2_WRITE, when non-compounded to stop the impact of this issue.  We
may want to add SMB2_CREATE to the list of ops we don't send async replies
for non-compounded, but I'm not sold either way, I know we CAN go async
there!  I want some opinions here.

This is not hidden behind a flag because compatibility issues that don't
impact protocol correctness usually aren't.

Setting req->async_te = talloc_new(NULL); is just ugly, though it works.
If you have a cleaner approach, I welcome it.

I request you please ASK me before pushing this one, yes, that means you
jra!

For those interested in reproduction: I'd suggest using a server that's
rebuilt with a lower timeout set in smb2_read.c, though we've hit it with
vfs_glusterfs straight up, in our testing.

Thanks,

-Ira / ira@(samba.org|redhat.com|wakeful.net)

[-- Attachment #2: 0001-smbd-Work-around-for-AIO-with-the-Linux-CIFS-client-.patch --]
[-- Type: application/mbox, Size: 1808 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Interop Issue: SMB2+ async replies, and the kernel, Samba side fix enclosed.
       [not found] ` <CAAPGDwLnp90t7wiRb_tw-Md0jv-8w0Ye66jNcsRNioganVe2xg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-02-23 13:05   ` Stefan Metzmacher
       [not found]     ` <1D07D524-6243-4DF5-BBEB-13EE3A2B62AE@samba.org>
  2016-02-23 14:28   ` Shirish Pargaonkar
  2016-02-23 16:54   ` Jeremy Allison
  2 siblings, 1 reply; 15+ messages in thread
From: Stefan Metzmacher @ 2016-02-23 13:05 UTC (permalink / raw)
  To: Ira Cooper, Samba Technical, linux-cifs-u79uwXL29TY76Z2rM5mHXA; +Cc: sfrench

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

Hi Ira,

> If the server sends an interim response, then the real response, the real
> response, is handled by standard_receive3() in the kernel, instead of the
> proper function, and this causes a disconnect.  If there isn't a
> disconnect, I believe the reply will just be discarded if I understand the
> code correctly.  (That is a big if here ;) )
> 
> I've written a patch for Samba to stop sending interim replies on SMB2_READ
> and SMB2_WRITE, when non-compounded to stop the impact of this issue.  We
> may want to add SMB2_CREATE to the list of ops we don't send async replies
> for non-compounded, but I'm not sold either way, I know we CAN go async
> there!  I want some opinions here.
> 
> This is not hidden behind a flag because compatibility issues that don't
> impact protocol correctness usually aren't.
> 
> Setting req->async_te = talloc_new(NULL); is just ugly, though it works.
> If you have a cleaner approach, I welcome it.

Fix the broken kernel client!

I think this is nothing we should work around in the server.
The situation might be different if Windows clients would be unhappy,
but the linux client is more or less under our control and can be fixed.

People hitting this can just use SMB1 until it's fixed.

metze


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Interop Issue: SMB2+ async replies, and the kernel, Samba side fix enclosed.
       [not found] ` <CAAPGDwLnp90t7wiRb_tw-Md0jv-8w0Ye66jNcsRNioganVe2xg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2016-02-23 13:05   ` Stefan Metzmacher
@ 2016-02-23 14:28   ` Shirish Pargaonkar
  2016-02-23 16:54   ` Jeremy Allison
  2 siblings, 0 replies; 15+ messages in thread
From: Shirish Pargaonkar @ 2016-02-23 14:28 UTC (permalink / raw)
  To: Ira Cooper; +Cc: Samba Technical, linux-cifs, sfrench

I think the course of action on cifs client should be realize this is
in interim response
and drop it without percolating up and wait for the real reply.
Looking into that...

Regards,

Shirish

On Tue, Feb 23, 2016 at 6:55 AM, Ira Cooper <ira-mA6sH8tAw6XR7s880joybQ@public.gmane.org> wrote:
> If the server sends an interim response, then the real response, the real
> response, is handled by standard_receive3() in the kernel, instead of the
> proper function, and this causes a disconnect.  If there isn't a
> disconnect, I believe the reply will just be discarded if I understand the
> code correctly.  (That is a big if here ;) )
>
> I've written a patch for Samba to stop sending interim replies on SMB2_READ
> and SMB2_WRITE, when non-compounded to stop the impact of this issue.  We
> may want to add SMB2_CREATE to the list of ops we don't send async replies
> for non-compounded, but I'm not sold either way, I know we CAN go async
> there!  I want some opinions here.
>
> This is not hidden behind a flag because compatibility issues that don't
> impact protocol correctness usually aren't.
>
> Setting req->async_te = talloc_new(NULL); is just ugly, though it works.
> If you have a cleaner approach, I welcome it.
>
> I request you please ASK me before pushing this one, yes, that means you
> jra!
>
> For those interested in reproduction: I'd suggest using a server that's
> rebuilt with a lower timeout set in smb2_read.c, though we've hit it with
> vfs_glusterfs straight up, in our testing.
>
> Thanks,
>
> -Ira / ira@(samba.org|redhat.com|wakeful.net)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Interop Issue: SMB2+ async replies, and the kernel, Samba side fix enclosed.
       [not found] ` <CAAPGDwLnp90t7wiRb_tw-Md0jv-8w0Ye66jNcsRNioganVe2xg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2016-02-23 13:05   ` Stefan Metzmacher
  2016-02-23 14:28   ` Shirish Pargaonkar
@ 2016-02-23 16:54   ` Jeremy Allison
  2 siblings, 0 replies; 15+ messages in thread
From: Jeremy Allison @ 2016-02-23 16:54 UTC (permalink / raw)
  To: Ira Cooper; +Cc: Samba Technical, linux-cifs-u79uwXL29TY76Z2rM5mHXA, sfrench

On Tue, Feb 23, 2016 at 07:55:59AM -0500, Ira Cooper wrote:
> If the server sends an interim response, then the real response, the real
> response, is handled by standard_receive3() in the kernel, instead of the
> proper function, and this causes a disconnect.  If there isn't a
> disconnect, I believe the reply will just be discarded if I understand the
> code correctly.  (That is a big if here ;) )
> 
> I've written a patch for Samba to stop sending interim replies on SMB2_READ
> and SMB2_WRITE, when non-compounded to stop the impact of this issue.  We
> may want to add SMB2_CREATE to the list of ops we don't send async replies
> for non-compounded, but I'm not sold either way, I know we CAN go async
> there!  I want some opinions here.
> 
> This is not hidden behind a flag because compatibility issues that don't
> impact protocol correctness usually aren't.
> 
> Setting req->async_te = talloc_new(NULL); is just ugly, though it works.
> If you have a cleaner approach, I welcome it.
> 
> I request you please ASK me before pushing this one, yes, that means you
> jra!

Actually I wasn't planning on pushing at all :-).

I don't think we should stop sending interim replies on SMB2_READ
or SMB2_WRITE, those replies have effects on the client timers
on properly functioning clients.

Linux client needs fixing here I'm afraid (and I know that's
not fun, sorry :-( ).

Jeremy.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Interop Issue: SMB2+ async replies, and the kernel, Samba side fix enclosed.
       [not found]     ` <1D07D524-6243-4DF5-BBEB-13EE3A2B62AE@samba.org>
@ 2016-02-24  4:41       ` Ira Cooper
       [not found]         ` <CAAPGDwL-VsVmPzFFEM5nOAgFCz68a2KHWGAy9KH6ED5PD0vx_Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 15+ messages in thread
From: Ira Cooper @ 2016-02-24  4:41 UTC (permalink / raw)
  To: Steven French; +Cc: Stefan Metzmacher, linux-cifs, Samba Technical

You'll find it easy to reproduce if you:

Set  aio read size = 1 in smb.conf.

And modify line 101 in smb2_read.c or close by to be:

    return smbd_smb2_request_pending_queue(req, subreq, 1);

Writing a 1 GB file, umounting the share, and mounting the share again,
then using dd to read it back with rsize and wsize set to 1MB, should do it.

We can reproduce it as is with Gluster, I suspect that may be enough to do
it with XFS or other filesystems.

Does anyone know what the windows timeout for sending the interim reply
is?  Barring this bug, sending it when we don't have to is wasteful.

Thanks,

-Ira

On Tue, Feb 23, 2016 at 11:11 PM, Steven French <sfrench@samba.org> wrote:

> Do we have a reproduction scenario?  I don’t remember this one and it
> doesn’t send familiar at all - I use smb3 kernel client to Samba every day.
>
> And yes - if we have a bug in the kernel client here let’s fix it.
>
>
>
> On Feb 23, 2016, at 7:05 AM, Stefan Metzmacher <metze@samba.org> wrote:
>
> Hi Ira,
>
> If the server sends an interim response, then the real response, the real
> response, is handled by standard_receive3() in the kernel, instead of the
> proper function, and this causes a disconnect.  If there isn't a
> disconnect, I believe the reply will just be discarded if I understand the
> code correctly.  (That is a big if here ;) )
>
> I've written a patch for Samba to stop sending interim replies on SMB2_READ
> and SMB2_WRITE, when non-compounded to stop the impact of this issue.  We
> may want to add SMB2_CREATE to the list of ops we don't send async replies
> for non-compounded, but I'm not sold either way, I know we CAN go async
> there!  I want some opinions here.
>
> This is not hidden behind a flag because compatibility issues that don't
> impact protocol correctness usually aren't.
>
> Setting req->async_te = talloc_new(NULL); is just ugly, though it works.
> If you have a cleaner approach, I welcome it.
>
>
> Fix the broken kernel client!
>
> I think this is nothing we should work around in the server.
> The situation might be different if Windows clients would be unhappy,
> but the linux client is more or less under our control and can be fixed.
>
> People hitting this can just use SMB1 until it's fixed.
>
> metze
>
>
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Interop Issue: SMB2+ async replies, and the kernel, Samba side fix enclosed.
       [not found]         ` <CAAPGDwL-VsVmPzFFEM5nOAgFCz68a2KHWGAy9KH6ED5PD0vx_Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-02-24 19:04           ` Jeremy Allison
  2016-03-01 13:40             ` Volker Lendecke
  0 siblings, 1 reply; 15+ messages in thread
From: Jeremy Allison @ 2016-02-24 19:04 UTC (permalink / raw)
  To: Ira Cooper
  Cc: Steven French, Stefan Metzmacher,
	linux-cifs-u79uwXL29TY76Z2rM5mHXA, Samba Technical

On Tue, Feb 23, 2016 at 11:41:34PM -0500, Ira Cooper wrote:
> You'll find it easy to reproduce if you:
> 
> Set  aio read size = 1 in smb.conf.
> 
> And modify line 101 in smb2_read.c or close by to be:
> 
>     return smbd_smb2_request_pending_queue(req, subreq, 1);
> 
> Writing a 1 GB file, umounting the share, and mounting the share again,
> then using dd to read it back with rsize and wsize set to 1MB, should do it.
> 
> We can reproduce it as is with Gluster, I suspect that may be enough to do
> it with XFS or other filesystems.
> 
> Does anyone know what the windows timeout for sending the interim reply
> is?  Barring this bug, sending it when we don't have to is wasteful.

Hmmm. We can only test this by causing a Windows read to
take a long time. Any idea how to test this ? Does Win32
have named pipes in the fs we could use for that ? If not
we could test using a program that creates a \\pipe\named_pipe
and then responds slowly... But I don't know if the
timeout replies on np's are the same as in the filesystem.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Interop Issue: SMB2+ async replies, and the kernel, Samba side fix enclosed.
       [not found] ` <CAKywueSMFrvreB8JMtwj8kt1BY1Yyq-Guy3Q3=wZFzuLCCOjZA@mail.gmail.com>
@ 2016-02-27  9:17   ` Pavel Shilovsky
       [not found]   ` <CAKywueSMFrvreB8JMtwj8kt1BY1Yyq-Guy3Q3=wZFzuLCCOjZA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  1 sibling, 0 replies; 15+ messages in thread
From: Pavel Shilovsky @ 2016-02-27  9:17 UTC (permalink / raw)
  To: Ira Cooper; +Cc: sfrench, linux-cifs, Samba Technical

2016-02-27 12:11 GMT+03:00 Pavel Shilovsky <pshilovsky@samba.org>:

> 2016-02-23 15:55 GMT+03:00 Ira Cooper <ira@wakeful.net>:
>
>> If the server sends an interim response, then the real response, the real
>> response, is handled by standard_receive3() in the kernel, instead of the
>> proper function, and this causes a disconnect.  If there isn't a
>> disconnect, I believe the reply will just be discarded if I understand the
>> code correctly.  (That is a big if here ;) )
>>
>> I've written a patch for Samba to stop sending interim replies on
>> SMB2_READ
>> and SMB2_WRITE, when non-compounded to stop the impact of this issue.  We
>> may want to add SMB2_CREATE to the list of ops we don't send async replies
>> for non-compounded, but I'm not sold either way, I know we CAN go async
>> there!  I want some opinions here.
>>
>> This is not hidden behind a flag because compatibility issues that don't
>> impact protocol correctness usually aren't.
>>
>> Setting req->async_te = talloc_new(NULL); is just ugly, though it works.
>> If you have a cleaner approach, I welcome it.
>>
>> I request you please ASK me before pushing this one, yes, that means you
>> jra!
>>
>> For those interested in reproduction: I'd suggest using a server that's
>> rebuilt with a lower timeout set in smb2_read.c, though we've hit it with
>> vfs_glusterfs straight up, in our testing.
>>
>> Thanks,
>>
>> -Ira / ira@(samba.org|redhat.com|wakeful.net)
>>
>
>
> Thank you for catching this!
>
> It is the issue in the kernel client - a check for interim responses is
> missed for SMB2_READ command. I created a patch that should fix the problem.
>
> Could you please test it?
>
> --
> Best regards,
> Pavel Shilovsky
>
>
CC'ing @samba-technical from another email address (my original email was
rejected by the server).

>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Interop Issue: SMB2+ async replies, and the kernel, Samba side fix enclosed.
       [not found]   ` <CAKywueSMFrvreB8JMtwj8kt1BY1Yyq-Guy3Q3=wZFzuLCCOjZA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-02-27  9:26     ` Pavel Shilovsky
  2016-02-27  9:31     ` Pavel Shilovsky
  1 sibling, 0 replies; 15+ messages in thread
From: Pavel Shilovsky @ 2016-02-27  9:26 UTC (permalink / raw)
  To: Ira Cooper; +Cc: Samba Technical, linux-cifs, sfrench

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

2016-02-27 12:11 GMT+03:00 Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>
> 2016-02-23 15:55 GMT+03:00 Ira Cooper <ira-mA6sH8tAw6XR7s880joybQ@public.gmane.org>:
>>
>> If the server sends an interim response, then the real response, the real
>> response, is handled by standard_receive3() in the kernel, instead of the
>> proper function, and this causes a disconnect.  If there isn't a
>> disconnect, I believe the reply will just be discarded if I understand the
>> code correctly.  (That is a big if here ;) )
>>
>> I've written a patch for Samba to stop sending interim replies on SMB2_READ
>> and SMB2_WRITE, when non-compounded to stop the impact of this issue.  We
>> may want to add SMB2_CREATE to the list of ops we don't send async replies
>> for non-compounded, but I'm not sold either way, I know we CAN go async
>> there!  I want some opinions here.
>>
>> This is not hidden behind a flag because compatibility issues that don't
>> impact protocol correctness usually aren't.
>>
>> Setting req->async_te = talloc_new(NULL); is just ugly, though it works.
>> If you have a cleaner approach, I welcome it.
>>
>> I request you please ASK me before pushing this one, yes, that means you
>> jra!
>>
>> For those interested in reproduction: I'd suggest using a server that's
>> rebuilt with a lower timeout set in smb2_read.c, though we've hit it with
>> vfs_glusterfs straight up, in our testing.
>>
>> Thanks,
>>
>> -Ira / ira@(samba.org|redhat.com|wakeful.net)
>
>
>
> Thank you for catching this!
>
> It is the issue in the kernel client - a check for interim responses is missed for SMB2_READ command. I created a patch that should fix the problem.
>
> Could you please test it?
>
> --
> Best regards,
> Pavel Shilovsky

CC'ing @samba-technical (my original email was rejected by the server).

Sorry for the spam.

-- 
Best regards,
Pavel Shilovsky

[-- Attachment #2: 0001-CIFS-Fix-SMB2-interim-response-processing-for-read-r.patch --]
[-- Type: text/x-patch, Size: 2082 bytes --]

From 09904bf0c6a1ecc7f61d4755db33b762b53176d6 Mon Sep 17 00:00:00 2001
From: Pavel Shilovsky <pshilovsky@samba.org>
Date: Sat, 27 Feb 2016 11:58:18 +0300
Subject: [PATCH] CIFS: Fix SMB2+ interim response processing for read requests

For interim responses we only need to parse a header and update
a number credits. Now it is done for all SMB2+ command except
SMB2_READ which is wrong. Fix this by adding such processing.

Signed-off-by: Pavel Shilovsky <pshilovsky@samba.org>
---
 fs/cifs/cifssmb.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 90b4f9f..76fcb50 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -1396,11 +1396,10 @@ openRetry:
  * current bigbuf.
  */
 static int
-cifs_readv_discard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
+discard_remaining_data(struct TCP_Server_Info *server)
 {
 	unsigned int rfclen = get_rfc1002_length(server->smallbuf);
 	int remaining = rfclen + 4 - server->total_read;
-	struct cifs_readdata *rdata = mid->callback_data;
 
 	while (remaining > 0) {
 		int length;
@@ -1414,10 +1413,20 @@ cifs_readv_discard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
 		remaining -= length;
 	}
 
-	dequeue_mid(mid, rdata->result);
 	return 0;
 }
 
+static int
+cifs_readv_discard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
+{
+	int length;
+	struct cifs_readdata *rdata = mid->callback_data;
+
+	length = discard_remaining_data(server);
+	dequeue_mid(mid, rdata->result);
+	return length;
+}
+
 int
 cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
 {
@@ -1446,6 +1455,12 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
 		return length;
 	server->total_read += length;
 
+	if (server->ops->is_status_pending &&
+	    server->ops->is_status_pending(buf, server, 0)) {
+		discard_remaining_data(server);
+		return -1;
+	}
+
 	/* Was the SMB read successful? */
 	rdata->result = server->ops->map_error(buf, false);
 	if (rdata->result != 0) {
-- 
2.1.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: Interop Issue: SMB2+ async replies, and the kernel, Samba side fix enclosed.
       [not found]   ` <CAKywueSMFrvreB8JMtwj8kt1BY1Yyq-Guy3Q3=wZFzuLCCOjZA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2016-02-27  9:26     ` Pavel Shilovsky
@ 2016-02-27  9:31     ` Pavel Shilovsky
       [not found]       ` <CAKywueRePEMq7d2bYDZN48_esZrx0P2FofrDDxJ62RtQtX1Xww-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  1 sibling, 1 reply; 15+ messages in thread
From: Pavel Shilovsky @ 2016-02-27  9:31 UTC (permalink / raw)
  To: Ira Cooper; +Cc: Samba Technical, linux-cifs, sfrench

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

2016-02-27 12:11 GMT+03:00 Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> 2016-02-23 15:55 GMT+03:00 Ira Cooper <ira-mA6sH8tAw6XR7s880joybQ@public.gmane.org>:
>>
>> If the server sends an interim response, then the real response, the real
>> response, is handled by standard_receive3() in the kernel, instead of the
>> proper function, and this causes a disconnect.  If there isn't a
>> disconnect, I believe the reply will just be discarded if I understand the
>> code correctly.  (That is a big if here ;) )
>>
>> I've written a patch for Samba to stop sending interim replies on
>> SMB2_READ
>> and SMB2_WRITE, when non-compounded to stop the impact of this issue.  We
>> may want to add SMB2_CREATE to the list of ops we don't send async replies
>> for non-compounded, but I'm not sold either way, I know we CAN go async
>> there!  I want some opinions here.
>>
>> This is not hidden behind a flag because compatibility issues that don't
>> impact protocol correctness usually aren't.
>>
>> Setting req->async_te = talloc_new(NULL); is just ugly, though it works.
>> If you have a cleaner approach, I welcome it.
>>
>> I request you please ASK me before pushing this one, yes, that means you
>> jra!
>>
>> For those interested in reproduction: I'd suggest using a server that's
>> rebuilt with a lower timeout set in smb2_read.c, though we've hit it with
>> vfs_glusterfs straight up, in our testing.
>>
>> Thanks,
>>
>> -Ira / ira@(samba.org|redhat.com|wakeful.net)
>
>
>
> Thank you for catching this!
>
> It is the issue in the kernel client - a check for interim responses is
> missed for SMB2_READ command. I created a patch that should fix the problem.
>
> Could you please test it?
>
> --
> Best regards,
> Pavel Shilovsky

CC'ing @samba-technical.

Sorry for the spam.

-- 
Best regards,
Pavel Shilovsky

[-- Attachment #2: 0001-CIFS-Fix-SMB2-interim-response-processing-for-read-r.patch --]
[-- Type: text/x-patch, Size: 2082 bytes --]

From 09904bf0c6a1ecc7f61d4755db33b762b53176d6 Mon Sep 17 00:00:00 2001
From: Pavel Shilovsky <pshilovsky@samba.org>
Date: Sat, 27 Feb 2016 11:58:18 +0300
Subject: [PATCH] CIFS: Fix SMB2+ interim response processing for read requests

For interim responses we only need to parse a header and update
a number credits. Now it is done for all SMB2+ command except
SMB2_READ which is wrong. Fix this by adding such processing.

Signed-off-by: Pavel Shilovsky <pshilovsky@samba.org>
---
 fs/cifs/cifssmb.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 90b4f9f..76fcb50 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -1396,11 +1396,10 @@ openRetry:
  * current bigbuf.
  */
 static int
-cifs_readv_discard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
+discard_remaining_data(struct TCP_Server_Info *server)
 {
 	unsigned int rfclen = get_rfc1002_length(server->smallbuf);
 	int remaining = rfclen + 4 - server->total_read;
-	struct cifs_readdata *rdata = mid->callback_data;
 
 	while (remaining > 0) {
 		int length;
@@ -1414,10 +1413,20 @@ cifs_readv_discard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
 		remaining -= length;
 	}
 
-	dequeue_mid(mid, rdata->result);
 	return 0;
 }
 
+static int
+cifs_readv_discard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
+{
+	int length;
+	struct cifs_readdata *rdata = mid->callback_data;
+
+	length = discard_remaining_data(server);
+	dequeue_mid(mid, rdata->result);
+	return length;
+}
+
 int
 cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
 {
@@ -1446,6 +1455,12 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
 		return length;
 	server->total_read += length;
 
+	if (server->ops->is_status_pending &&
+	    server->ops->is_status_pending(buf, server, 0)) {
+		discard_remaining_data(server);
+		return -1;
+	}
+
 	/* Was the SMB read successful? */
 	rdata->result = server->ops->map_error(buf, false);
 	if (rdata->result != 0) {
-- 
2.1.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: Interop Issue: SMB2+ async replies, and the kernel, Samba side fix enclosed.
       [not found]       ` <CAKywueRePEMq7d2bYDZN48_esZrx0P2FofrDDxJ62RtQtX1Xww-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-02-27 13:50         ` Shirish Pargaonkar
       [not found]           ` <CADT32eLG7aNKGqprQPwJdxdDyw__c39sUgX7vdkg=hNNtV63QQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 15+ messages in thread
From: Shirish Pargaonkar @ 2016-02-27 13:50 UTC (permalink / raw)
  To: Pavel Shilovsky; +Cc: Ira Cooper, Samba Technical, linux-cifs, sfrench

Pavel, briefly tested this, it looks good. No error (messages) logged.

Tested-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

On Sat, Feb 27, 2016 at 3:31 AM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2016-02-27 12:11 GMT+03:00 Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
>> 2016-02-23 15:55 GMT+03:00 Ira Cooper <ira-mA6sH8tAw6XR7s880joybQ@public.gmane.org>:
>>>
>>> If the server sends an interim response, then the real response, the real
>>> response, is handled by standard_receive3() in the kernel, instead of the
>>> proper function, and this causes a disconnect.  If there isn't a
>>> disconnect, I believe the reply will just be discarded if I understand the
>>> code correctly.  (That is a big if here ;) )
>>>
>>> I've written a patch for Samba to stop sending interim replies on
>>> SMB2_READ
>>> and SMB2_WRITE, when non-compounded to stop the impact of this issue.  We
>>> may want to add SMB2_CREATE to the list of ops we don't send async replies
>>> for non-compounded, but I'm not sold either way, I know we CAN go async
>>> there!  I want some opinions here.
>>>
>>> This is not hidden behind a flag because compatibility issues that don't
>>> impact protocol correctness usually aren't.
>>>
>>> Setting req->async_te = talloc_new(NULL); is just ugly, though it works.
>>> If you have a cleaner approach, I welcome it.
>>>
>>> I request you please ASK me before pushing this one, yes, that means you
>>> jra!
>>>
>>> For those interested in reproduction: I'd suggest using a server that's
>>> rebuilt with a lower timeout set in smb2_read.c, though we've hit it with
>>> vfs_glusterfs straight up, in our testing.
>>>
>>> Thanks,
>>>
>>> -Ira / ira@(samba.org|redhat.com|wakeful.net)
>>
>>
>>
>> Thank you for catching this!
>>
>> It is the issue in the kernel client - a check for interim responses is
>> missed for SMB2_READ command. I created a patch that should fix the problem.
>>
>> Could you please test it?
>>
>> --
>> Best regards,
>> Pavel Shilovsky
>
> CC'ing @samba-technical.
>
> Sorry for the spam.
>
> --
> Best regards,
> Pavel Shilovsky

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Interop Issue: SMB2+ async replies, and the kernel, Samba side fix enclosed.
       [not found]           ` <CADT32eLG7aNKGqprQPwJdxdDyw__c39sUgX7vdkg=hNNtV63QQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-02-28  6:22             ` Pavel Shilovsky
  2016-02-29  4:15               ` Steve French
       [not found]               ` <CAKywueRcieBmFFjBxzGqQupG1T1Y9aj-viO6q15Dv2w6fHETfQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 2 replies; 15+ messages in thread
From: Pavel Shilovsky @ 2016-02-28  6:22 UTC (permalink / raw)
  To: Shirish Pargaonkar; +Cc: Ira Cooper, Samba Technical, linux-cifs, sfrench

2016-02-27 16:50 GMT+03:00 Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> Pavel, briefly tested this, it looks good. No error (messages) logged.
>
> Tested-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Thanks!

-- 
Best regards,
Pavel Shilovsky

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Interop Issue: SMB2+ async replies, and the kernel, Samba side fix enclosed.
  2016-02-28  6:22             ` Pavel Shilovsky
@ 2016-02-29  4:15               ` Steve French
       [not found]               ` <CAKywueRcieBmFFjBxzGqQupG1T1Y9aj-viO6q15Dv2w6fHETfQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  1 sibling, 0 replies; 15+ messages in thread
From: Steve French @ 2016-02-29  4:15 UTC (permalink / raw)
  To: Pavel Shilovsky; +Cc: sfrench, Samba Technical, Ira Cooper, linux-cifs

merged into cifs-2.6.git

Added Shirish's tested-by and cc: stable

Let me know if any objections to going to stable etc

On Sun, Feb 28, 2016 at 12:22 AM, Pavel Shilovsky <piastryyy@gmail.com>
wrote:

> 2016-02-27 16:50 GMT+03:00 Shirish Pargaonkar <shirishpargaonkar@gmail.com
> >:
> > Pavel, briefly tested this, it looks good. No error (messages) logged.
> >
> > Tested-by: Shirish Pargaonkar <shirishpargaonkar@gmail.com>
>
> Thanks!
>
> --
> 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
>



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Interop Issue: SMB2+ async replies, and the kernel, Samba side fix enclosed.
       [not found]               ` <CAKywueRcieBmFFjBxzGqQupG1T1Y9aj-viO6q15Dv2w6fHETfQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-02-29  4:17                 ` Steve French
  0 siblings, 0 replies; 15+ messages in thread
From: Steve French @ 2016-02-29  4:17 UTC (permalink / raw)
  To: Pavel Shilovsky
  Cc: Shirish Pargaonkar, Ira Cooper, Samba Technical, linux-cifs, sfrench

merged into cifs-2.6.git

Added Shirish's tested-by and cc: stable

Let me know if any objections to going to stable etc

On Sun, Feb 28, 2016 at 12:22 AM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2016-02-27 16:50 GMT+03:00 Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>> Pavel, briefly tested this, it looks good. No error (messages) logged.
>>
>> Tested-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> Thanks!
>
> --
> Best regards,
> Pavel Shilovsky
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Interop Issue: SMB2+ async replies, and the kernel, Samba side fix enclosed.
  2016-02-24 19:04           ` Jeremy Allison
@ 2016-03-01 13:40             ` Volker Lendecke
       [not found]               ` <20160301134039.GA21826-3ekOc4rQMZmzQB+pC5nmwQ@public.gmane.org>
  0 siblings, 1 reply; 15+ messages in thread
From: Volker Lendecke @ 2016-03-01 13:40 UTC (permalink / raw)
  To: Jeremy Allison
  Cc: Steven French, Stefan Metzmacher, Ira Cooper, Samba Technical,
	linux-cifs

On Wed, Feb 24, 2016 at 11:04:08AM -0800, Jeremy Allison wrote:
> Hmmm. We can only test this by causing a Windows read to
> take a long time. Any idea how to test this ? Does Win32
> have named pipes in the fs we could use for that ? If not
> we could test using a program that creates a \\pipe\named_pipe
> and then responds slowly... But I don't know if the
> timeout replies on np's are the same as in the filesystem.

Just use a USB2 stick.

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt@sernet.de

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: Interop Issue: SMB2+ async replies, and the kernel, Samba side fix enclosed.
       [not found]               ` <20160301134039.GA21826-3ekOc4rQMZmzQB+pC5nmwQ@public.gmane.org>
@ 2016-03-01 20:10                 ` Christian Ambach
  0 siblings, 0 replies; 15+ messages in thread
From: Christian Ambach @ 2016-03-01 20:10 UTC (permalink / raw)
  To: Volker.Lendecke-PS7XAnAlDA+VvDNblw4Uiw, Jeremy Allison
  Cc: Steven French, Stefan Metzmacher, Ira Cooper, Samba Technical,
	linux-cifs-u79uwXL29TY76Z2rM5mHXA

Am 01.03.16 um 14:40 schrieb Volker Lendecke:
> On Wed, Feb 24, 2016 at 11:04:08AM -0800, Jeremy Allison wrote:
>> Hmmm. We can only test this by causing a Windows read to
>> take a long time. Any idea how to test this ? Does Win32
>> have named pipes in the fs we could use for that ? If not
>> we could test using a program that creates a \\pipe\named_pipe
>> and then responds slowly... But I don't know if the
>> timeout replies on np's are the same as in the filesystem.
>
> Just use a USB2 stick.
>
[MS-SMB2] gives some hints when Windows will send out interim replies:
<296> Section 3.3.5.12: Windows SMB2 servers send an interim response to 
the client and handle the read asynchronously if the read is not 
finished in 0.5 milliseconds.
<297> Section 3.3.5.12: Windows-based servers handle the following
commands asynchronously: SMB2 Create (section 2.2.13) when this create would
result in an oplock break, SMB2 IOCTL Request (section 2.2.31) for
FSCTL_PIPE_TRANSCEIVE if it blocks for more than 1 millisecond, SMB2 IOCTL
Request for FSCTL_SRV_COPYCHUNK or FSCTL_SRV_COPYCHUNK_WRITE (section 
2.2.31)
when oplock break happens, SMB2 Change_Notify Request (section 2.2.35) if
it blocks for more than 0.5 milliseconds, SMB2 Read request (section 2.2.19)
for named pipes if it blocks for more than 0.5 milliseconds,
SMB2 Write request (section 2.2.21) for named pipes if it blocks for more
than 0.5 milliseconds, SMB2 Write Request (section 2.2.21) for large 
file write,
SMB2 lock request (section 2.2.26) if the LOCKFLAG_FAIL_IMMEDIATELY flag 
is not set,
and SMB2 FLUSH Request (section 2.2.17) for named pipes.

Cheers,
Christian

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2016-03-01 20:10 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-23 12:55 Interop Issue: SMB2+ async replies, and the kernel, Samba side fix enclosed Ira Cooper
     [not found] ` <CAAPGDwLnp90t7wiRb_tw-Md0jv-8w0Ye66jNcsRNioganVe2xg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-02-23 13:05   ` Stefan Metzmacher
     [not found]     ` <1D07D524-6243-4DF5-BBEB-13EE3A2B62AE@samba.org>
2016-02-24  4:41       ` Ira Cooper
     [not found]         ` <CAAPGDwL-VsVmPzFFEM5nOAgFCz68a2KHWGAy9KH6ED5PD0vx_Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-02-24 19:04           ` Jeremy Allison
2016-03-01 13:40             ` Volker Lendecke
     [not found]               ` <20160301134039.GA21826-3ekOc4rQMZmzQB+pC5nmwQ@public.gmane.org>
2016-03-01 20:10                 ` Christian Ambach
2016-02-23 14:28   ` Shirish Pargaonkar
2016-02-23 16:54   ` Jeremy Allison
     [not found] ` <CAKywueSMFrvreB8JMtwj8kt1BY1Yyq-Guy3Q3=wZFzuLCCOjZA@mail.gmail.com>
2016-02-27  9:17   ` Pavel Shilovsky
     [not found]   ` <CAKywueSMFrvreB8JMtwj8kt1BY1Yyq-Guy3Q3=wZFzuLCCOjZA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-02-27  9:26     ` Pavel Shilovsky
2016-02-27  9:31     ` Pavel Shilovsky
     [not found]       ` <CAKywueRePEMq7d2bYDZN48_esZrx0P2FofrDDxJ62RtQtX1Xww-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-02-27 13:50         ` Shirish Pargaonkar
     [not found]           ` <CADT32eLG7aNKGqprQPwJdxdDyw__c39sUgX7vdkg=hNNtV63QQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-02-28  6:22             ` Pavel Shilovsky
2016-02-29  4:15               ` Steve French
     [not found]               ` <CAKywueRcieBmFFjBxzGqQupG1T1Y9aj-viO6q15Dv2w6fHETfQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-02-29  4:17                 ` 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.