Bug 7331 - Compound async SMB 2 requests don't work right.
Summary: Compound async SMB 2 requests don't work right.
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.5
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: x64 Windows 7
: P3 critical
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-06 13:19 UTC by Ira Cooper
Modified: 2020-12-11 07:22 UTC (History)
2 users (show)

See Also:


Attachments
smb2_compound.patch (10.45 KB, patch)
2010-04-06 13:20 UTC, Ira Cooper
no flags Details
Source for the SMB2 reproduction exe. (2.37 KB, text/plain)
2010-04-06 13:22 UTC, Ira Cooper
no flags Details
exe to reproduce the issue. (141.18 KB, application/x-zip-compressed)
2010-04-06 13:22 UTC, Ira Cooper
no flags Details
A better reproduction program. (139.93 KB, application/x-zip-compressed)
2010-04-06 19:50 UTC, Ira Cooper
no flags Details
THIS PATCH DOES NOT YET WORK (17.18 KB, patch)
2010-04-16 19:29 UTC, Jeremy Allison
no flags Details
Patch for master (19.19 KB, patch)
2010-04-17 01:48 UTC, Jeremy Allison
no flags Details
Wireshark capture of the program running over SMB2 Win7 to W2K8R2. (3.84 KB, application/octet-stream)
2010-04-17 22:39 UTC, Jeremy Allison
no flags Details
Samba program running to Samba with my patch (1.96 KB, application/octet-stream)
2010-04-17 23:04 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ira Cooper 2010-04-06 13:19:40 UTC
We have found that SMB 2 compound requests that cause the server to go async cause the non-async reply part of the compound reply to get held until the reply is ready overall, which may never happen for requests like notify.

Reproduction code/exe are provided.

You will need to enable some form of real notify handling to reproduce this.  We use vfs_notfiy_fam, but I'd think inotify should show the issue also.

I've also provided a prototype patch.  I do not believe the patch is ready for production use.  But I do think it helps highlight this issue.

There is also a fix for the fact that reply processing is not "serial", aka: There is not only one request in flight within the server at once.  This complicated some of the debugging and making sense of what the server was doing.   I was also unsure it was intentional.

Platform Tested on Windows Side: Windows 7 - 64 bit.
Platform Tested Server Side: Solaris 10u8 - x64.
Comment 1 Ira Cooper 2010-04-06 13:20:49 UTC
Created attachment 5602 [details]
smb2_compound.patch

This patch is not production ready.
Comment 2 Ira Cooper 2010-04-06 13:22:28 UTC
Created attachment 5603 [details]
Source for the SMB2 reproduction exe.
Comment 3 Ira Cooper 2010-04-06 13:22:59 UTC
Created attachment 5604 [details]
exe to reproduce the issue.
Comment 4 Jeremy Allison 2010-04-06 17:53:48 UTC
Hmmm. When I run this exe on Win7 I'm not seeing a compound request :-(. Can you attach your wireshark trace so I can take a look please ?

Jeremy.
Comment 5 Ira Cooper 2010-04-06 19:50:35 UTC
Created attachment 5605 [details]
A better reproduction program.

This program should show off the error.  My apologies for the previous program.  For maximum effect please copy the exe to the share to be tested and have it test the same directory the program is in.  From a net use'd volume.  (that is how I was able to reproduce the C/N issue.)

The speed the cancel is sent with the second time is critical to the reproduction of the bug, in its full glory.

But the second part of the bug also shows here in that there is only a reply to the notify, not the create.  (The interim reply to the notify.)
Comment 6 Jeremy Allison 2010-04-06 22:42:02 UTC
Thanks, I'll get to this by Friday. I have to fix another bug I've found in our compound operation processing first (smbtorture4 test to ensure we don't regress to follow).

Jeremy.
Comment 7 Stefan Metzmacher 2010-04-07 03:04:53 UTC
Jeremy please discuss patches with me before you push them to master,
this is really tricky code and we need to get a clean solution.

Ira, can you please upload a network capture that shows the correct behavior against a windows server. And one that shows the misbehavior against samba.
Comment 8 Ira Cooper 2010-04-07 11:57:00 UTC
I can't provide a pcap due to the security people in my company.  It isn't my call.  Trust me, I'd rather provide it.

That said, at least I provided a full reproduction program.  I'd hope that's enough for now.

As far as Microsoft goes: http://connect.microsoft.com/WNDP/feedback/details/522650

They have an issue here too, but there are two issues, and they have only one of them.

There is the issue of processing the cancel before you get the request to the point where it can be cancelled.  That is what the bug that we filed with Microsoft is about.  samba also has that bug.

In addition samba has another bug where when doing compound message processing with async requests it will not send out the reply to the non-async parts until the async parts complete.  That is why I patched the way interim responses are handled.

Personally: While I think what I did improved things, I am not sold it is "right".  I am not sure enough of the life-cycle of some of the objects involved to know, and there is certainly some playing with the header of the async reply, that I may have gotten wrong.  (Not the interim, but the actual second reply.)

Also there is the fact that the actual request gets processed with the code I wrote... which in and of itself is a bit gross, and needs to be looked at with a critical eye.  It is nasty code, no denying it.  But the problem is nasty.

A larger refactoring may be able to clean the issue up more.  But that is well beyond the scope I wanted to goto without talking to the samba team.

Passing the test program is not strictly enough to deal with this issue.  But the test program should be able to highlight all the things you want to fix.

-Ira
Comment 9 Ira Cooper 2010-04-07 11:59:19 UTC
> Also there is the fact that the actual request gets processed with the code I
> wrote... which in and of itself is a bit gross, and needs to be looked at with
> a critical eye.  It is nasty code, no denying it.  But the problem is nasty.

Gets reprocessed.  So the same request will be used to send 2 packets.

Pardon,

-Ira
Comment 10 Jeremy Allison 2010-04-16 19:16:23 UTC
Ok, I have a patch which is within a hair of working and fixing both problems reported here. It allows requests to go async, and also forces compound packets to be processed ahead of incoming requests, thus fixing the issue of a cancel request coming in before it has been processed enough to be cancelled.

Trouble is it's just not quite there yet and causes the client to drop the connection.

I'm going to post it here for backup, and also so you can see how I'm progressing on this. Metze, please don't make any changes to the S3 SMB2 code until I'm finished (hoping to get this done on Monday :-).

Jeremy.
Comment 11 Jeremy Allison 2010-04-16 19:29:02 UTC
Created attachment 5636 [details]
THIS PATCH DOES NOT YET WORK

But should give an idea of where I'm going with this. Please let me finish this (should be done no later than Tues 20th Pacific time:-).

Jeremy.
Comment 12 Jeremy Allison 2010-04-17 01:48:13 UTC
Created attachment 5637 [details]
Patch for master

Ok, this patch works and passes valgrind. But it still causes the client to stop sending requests and terminate after 30 seconds of inactivity, so there's something in the async reply we send in a compound reply that goes async that it doesn't like.

This patch is very close though, I'm expecting only to have to do minor changes to it. It also gives me the framework I need to do the async SHARE_MODE_VIOLATION, oplocks, and locking async requests.

Metze, please review.

Jeremy.
Comment 13 Jeremy Allison 2010-04-17 01:49:24 UTC
Forgot to add. I'm going to run the reproducer program against W2K8R2 to see what the replies from that look like, and if it also causes the client to disconnect.

Jeremy.
Comment 14 Jeremy Allison 2010-04-17 22:37:28 UTC
Hurrah. Win7 to W2K8R2 with this program fails in pretty much the same way as it does to Samba with my patch (client ends up sending a RST packet). They get the close too, and their cancel packets are a little different. I'll upload the traces for comparisons.

Jeremy.
Comment 15 Jeremy Allison 2010-04-17 22:39:20 UTC
Created attachment 5638 [details]
Wireshark capture of the program running over SMB2 Win7 to W2K8R2.
Comment 16 Jeremy Allison 2010-04-17 23:04:21 UTC
Created attachment 5639 [details]
Samba program running to Samba with my patch

This is a capture of the same program running to Samba. It's interesting in that when we're replying to a chained request, we add the async reply at the end of the chain. W2K8R2 doesn't do that - it seems to split off the async reply into another reply which it sends after the chained part of the reply. I think I'll do the same and see if it causes the program to send us the close request as well (which I see in the W2K8R2 trace but not in the one to Samba).

Jeremy.
Comment 17 Jeremy Allison 2010-04-18 00:43:50 UTC
Ok, checked my fixes into master. Look at :

bf45b4f4fda2c4e0d697bb30720c780325c3cd84

followed by :

1aa80f5788da34cd10141e6bfb5c263346dda75e

I'm now happy I can go in and finish the code for NT_STATUS_SHARE_MODE_VIOLATION and the oplock and lock async requests.

Jeremy.
Comment 18 Ira Cooper 2010-04-18 06:32:34 UTC
Quickly looking at the samba trace I can see the bug:

In my version of the spec: 3.3.5.16: - Receiving an SMB2 CANCEL Request

There are no STATUS_CANCELLED replies here, thus the trace is in error, and the client is disconnecting.

-Ira
Comment 19 Jeremy Allison 2010-04-18 19:54:13 UTC
Ah, that's it - great, thanks. Should be able to add that pretty quickly.
Cheers,
Jeremy.
Comment 20 Jeremy Allison 2010-04-18 22:29:48 UTC
Thanks. Added git commit 59fa1e1890e0a007f56776d9539bf3f1ce074a34 to master which now causes your test program to pass perfectly against master !

W00t! W2K8R2 doesn't do that :-).

Let me know when you've confirmed this and I'll close out the bug report.

Thanks a *lot* for this ! We now have the infrastructure to add all sorts of stuff to the SMB2 server. Maybe we'll be able to turn it on by default in 3.6.0 after all !

Jeremy.
Comment 21 Stefan Metzmacher 2010-04-19 03:31:34 UTC
Jeremy,

I think we should match the "broken" w2k8r2 behavior,
until Microsoft releases a fixed product.

Otherwise, people will test client code against samba
and will later notice in production that it doesn't
against any windows version.
Comment 22 Jeremy Allison 2010-04-19 09:28:29 UTC
No I don't think you're right here metze. W2K8R2 misses sending a response, causing the client to drop the connection (which is what we used to do too). This is not a feature to be emulated, it's simply a bug (that they will have to fix). I'm all for compatibility, but dropping responses to break clients in the same way as Windows is a step too far :-).
Jeremy.
Comment 23 Ira Cooper 2010-04-19 09:59:43 UTC
Jeremy:

We tested it locally, we've found the patches work and the traffic looks good, we'll be able to move into more in-depth SMB2 testing now.
 
Thank you for your assistance, and we share your enthusiasm with the fix!  We look forward to continuing to contribute to the community.

PS: I am not sure you need to break the async reply off the compound reply chain.  But I can see doing so to be more like w2k8 server.  Either way works.

-Ira
Comment 24 Jeremy Allison 2010-04-19 10:12:05 UTC
Yes, I was thinking about putting the code back that sends the async reply as part of the compound reply. It does make the code paths easier and the client didn't seem to have a problem with it. I might do so as part of refactoring work.

Right now the other things on my work queue need addressing before we can say we have functional parity, so please be aware oplocks, share mode violation deferring and blocking locks don't work yet.

Jeremy.