The Samba-Bugzilla – Bug 7331
Compound async SMB 2 requests don't work right.
Last modified: 2010-04-19 10:12:05 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.
Created attachment 5602 [details]
This patch is not production ready.
Created attachment 5603 [details]
Source for the SMB2 reproduction exe.
Created attachment 5604 [details]
exe to reproduce the issue.
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 ?
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.)
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 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.
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.
> 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.
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 :-).
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:-).
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.
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.
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.
Created attachment 5638 [details]
Wireshark capture of the program running over SMB2 Win7 to W2K8R2.
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).
Ok, checked my fixes into master. Look at :
followed by :
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.
Quickly looking at the samba trace I can see the bug:
In my version of the spec: 188.8.131.52: - Receiving an SMB2 CANCEL Request
There are no STATUS_CANCELLED replies here, thus the trace is in error, and the client is disconnecting.
Ah, that's it - great, thanks. Should be able to add that pretty quickly.
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 !
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.
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 :-).
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.
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.