Created attachment 17515 [details] "Raw" patch that fixes it for master.
More importantly, given our current code - when the SMB2 FLUSH is cancelled and returns NT_STATUS_INTERNAL_ERROR, the pthreadpool thread is left running and when it completes after the close has finished the server crashes. So whatever we eventually decide for dealing with potentially async compound requests, we'll have to use the smb2_request_set_async_internal() trick for compound SMB2 FLUSH requests.
*** Bug 15119 has been marked as a duplicate of this bug. ***
Metze requested 4 tests, which I am now coding up. 1). Opendir foo notify foo create foo/bar notify recv delete foo/bar C -------------- notify close (compound handle). -------------- 2). Opendir foo notify foo create foo/bar notify recv delete foo/bar C -------------- getinfo foo-handle notify close (compound handle). -------------- 3). Opendir foo notify foo create foo/bar notify recv C -------------- notify close (compound handle). -------------- 4). Opendir foo notify foo create foo/bar notify recv C -------------- getinfo foo-handle notify close (compound handle). -------------- I now have test #1 done, and when I send the compound notify+close, I get NT_STATUS_INTERNAL_ERROR against Windows 2020 returned for the notify (which should not have gone async as it had a pending notify to reply). So it isn't as simple as always allowing async in a compound. Brad, I think you need to rethink what your client is doing here.
Created attachment 17526 [details] WIP patches for master. Implement tests 1 and 2 against Windows (pass).
Created attachment 17527 [details] WIP patches for master. All 4 tests requested by Metze. Requested clarification from dochelp.
Created attachment 17528 [details] WIP patches for master. Now with all 4 tests :-).
From dochelp: Windows does not allow ASYNC processing of compounded messages (returning STATUS_INTERNAL_ERROR) except for the final compounded message. We do allow ASYNC processing for a few special cases: * A DFS FSCTL, which may block on active directory * Executing a PIPED (READ|WRITE) or OFFLINE FILE (READ|WRITE). * A WRITE that extends valid disk length by a large amount. * A couple of Create scenarios - retry Create on access violation, and waiting for an oplock break
Those tests aren't needed now we know exactly how Windows behaves. Also, it turns out that SMB_OP_WRITE and SMB_OP_READ in a compound against Samba are *always* handled synchronously - note lines: source3/smbd/smb2_aio.c:schedule_smb2_aio_read() 319 if (smbd_smb2_is_compound(smbreq->smb2req)) { 320 return NT_STATUS_RETRY; 321 } source3/smbd/smb2_aio.c:schedule_aio_smb2_write() 458 if (smbd_smb2_is_compound(smbreq->smb2req)) { 459 return NT_STATUS_RETRY; 460 } - returning NT_STATUS_RETRY from these functions causes the callers to fallback to doing a synchronous write or read.
https://gitlab.com/samba-team/samba/-/merge_requests/2764 Fixes by returning INTERNAL_ERROR for the non-final request in a compound. However, as SMB2_OP_READ/SMB2_OP_WRITE use synchronous mode if they are in any position in a compound, we might fix by causing synchronous mode here. I'll wait to discuss with Ralph to decide. There's a further enhancement we can do for SMB2_OP_READ/SMB2_OP_WRITE by allowing them to go async if they are the last request in a compound - currently we force them to be sync even if they are the last request in a compound. I'm planning to add code to allow SMB2_OP_READ/SMB2_OP_WRITE to go async if they're the last request in a compound using the same logic as I'm using here for SMB_OP_FLUSH, but I'll submit that as a separate MR once this is done.
This bug was referenced in samba master: 17a110c1b58196eb8ecf3c76eb97e8508976c544 6f149dfd9d8d2619a9e18975ebcf5e69df2b7766 e668c3a82cd566b405c976d45659dd79786948de 26adf3344337f4e8d5d2107e6ba42e5ea7656372
Created attachment 17663 [details] git-am fix for 4.17.next. Cherry-picked from master.
Created attachment 17664 [details] git-am fix for 4.16.next. Back-ported from master (first patch needed change in python test position).
Ping ! Can I get a review on these please ? I'd love to get them into 4.17.next, 4.16.next.
Reassigning to Jule for inclusion in 4.16 and 4.17.
Pushed to autobuild-v4-{17,16}-test.
This bug was referenced in samba v4-16-test: c9ed55b39efc7638b898db6df8352b10950c6c93 9b357c947fd6c36779f49dc37f9c81392c9c81fb bfadcc893e6ff2cd7f34896a194e7029fd6a76f5 e5e39bbc77f5f842b69f0789f3dbfa58bfbdd010
This bug was referenced in samba v4-17-test: 67d388c71f75aad94b3019b1dbbab261c01e6604 7b4652b80278ef21fe0bb8709676038f31e71a05 61babd9af831ee47731cfe062c884e974e74e2fd 1e94c94ae8520dfc91f8462f70ace026c96584e9
Closing out bug report. Thanks!
This bug was referenced in samba v4-17-stable (Release samba-4.17.5): 67d388c71f75aad94b3019b1dbbab261c01e6604 7b4652b80278ef21fe0bb8709676038f31e71a05 61babd9af831ee47731cfe062c884e974e74e2fd 1e94c94ae8520dfc91f8462f70ace026c96584e9
This bug was referenced in samba v4-16-stable (Release samba-4.16.9): c9ed55b39efc7638b898db6df8352b10950c6c93 9b357c947fd6c36779f49dc37f9c81392c9c81fb bfadcc893e6ff2cd7f34896a194e7029fd6a76f5 e5e39bbc77f5f842b69f0789f3dbfa58bfbdd010