Bug 15172 - Compound SMB2 FLUSH+CLOSE requests from MacOSX are not handled correctly.
Summary: Compound SMB2 FLUSH+CLOSE requests from MacOSX are not handled correctly.
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jule Anger
QA Contact: Samba QA Contact
URL:
Keywords:
: 15119 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-09-13 22:56 UTC by Jeremy Allison
Modified: 2023-01-26 17:50 UTC (History)
6 users (show)

See Also:


Attachments
"Raw" patch that fixes it for master. (1.63 KB, patch)
2022-09-14 00:10 UTC, Jeremy Allison
no flags Details
WIP patches for master. (10.33 KB, patch)
2022-09-22 22:30 UTC, Jeremy Allison
no flags Details
WIP patches for master. (14.64 KB, patch)
2022-09-22 23:13 UTC, Jeremy Allison
no flags Details
WIP patches for master. (19.55 KB, patch)
2022-09-22 23:14 UTC, Jeremy Allison
no flags Details
git-am fix for 4.17.next. (16.11 KB, patch)
2022-11-17 19:24 UTC, Jeremy Allison
slow: review+
Details
git-am fix for 4.16.next. (16.20 KB, patch)
2022-11-17 19:25 UTC, Jeremy Allison
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2022-09-13 22:56:30 UTC

    
Comment 1 Jeremy Allison 2022-09-14 00:10:09 UTC
Created attachment 17515 [details]
"Raw" patch that fixes it for master.
Comment 2 Jeremy Allison 2022-09-14 04:48:23 UTC
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.
Comment 3 Jeremy Allison 2022-09-15 21:32:58 UTC
*** Bug 15119 has been marked as a duplicate of this bug. ***
Comment 4 Jeremy Allison 2022-09-21 20:08:25 UTC
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.
Comment 5 Jeremy Allison 2022-09-22 22:30:43 UTC
Created attachment 17526 [details]
WIP patches for master.

Implement tests 1 and 2 against Windows (pass).
Comment 6 Jeremy Allison 2022-09-22 23:13:18 UTC
Created attachment 17527 [details]
WIP patches for master.

All 4 tests requested by Metze. Requested clarification from dochelp.
Comment 7 Jeremy Allison 2022-09-22 23:14:46 UTC
Created attachment 17528 [details]
WIP patches for master.

Now with all 4 tests :-).
Comment 8 Jeremy Allison 2022-10-03 19:08:20 UTC
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
Comment 9 Jeremy Allison 2022-10-19 21:15:15 UTC
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.
Comment 10 Jeremy Allison 2022-10-21 18:04:23 UTC
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.
Comment 11 Samba QA Contact 2022-11-17 05:56:15 UTC
This bug was referenced in samba master:

17a110c1b58196eb8ecf3c76eb97e8508976c544
6f149dfd9d8d2619a9e18975ebcf5e69df2b7766
e668c3a82cd566b405c976d45659dd79786948de
26adf3344337f4e8d5d2107e6ba42e5ea7656372
Comment 12 Jeremy Allison 2022-11-17 19:24:05 UTC
Created attachment 17663 [details]
git-am fix for 4.17.next.

Cherry-picked from master.
Comment 13 Jeremy Allison 2022-11-17 19:25:02 UTC
Created attachment 17664 [details]
git-am fix for 4.16.next.

Back-ported from master (first patch needed change in python test position).
Comment 14 Jeremy Allison 2023-01-13 17:55:45 UTC
Ping ! Can I get a review on these please ? I'd love to get them into 4.17.next, 4.16.next.
Comment 15 Ralph Böhme 2023-01-13 18:25:25 UTC
Reassigning to Jule for inclusion in 4.16 and 4.17.
Comment 16 Jule Anger 2023-01-16 09:11:21 UTC
Pushed to autobuild-v4-{17,16}-test.
Comment 17 Samba QA Contact 2023-01-16 10:48:20 UTC
This bug was referenced in samba v4-16-test:

c9ed55b39efc7638b898db6df8352b10950c6c93
9b357c947fd6c36779f49dc37f9c81392c9c81fb
bfadcc893e6ff2cd7f34896a194e7029fd6a76f5
e5e39bbc77f5f842b69f0789f3dbfa58bfbdd010
Comment 18 Samba QA Contact 2023-01-16 10:50:11 UTC
This bug was referenced in samba v4-17-test:

67d388c71f75aad94b3019b1dbbab261c01e6604
7b4652b80278ef21fe0bb8709676038f31e71a05
61babd9af831ee47731cfe062c884e974e74e2fd
1e94c94ae8520dfc91f8462f70ace026c96584e9
Comment 19 Jule Anger 2023-01-16 11:59:03 UTC
Closing out bug report.

Thanks!
Comment 20 Samba QA Contact 2023-01-26 17:50:05 UTC
This bug was referenced in samba v4-17-stable (Release samba-4.17.5):

67d388c71f75aad94b3019b1dbbab261c01e6604
7b4652b80278ef21fe0bb8709676038f31e71a05
61babd9af831ee47731cfe062c884e974e74e2fd
1e94c94ae8520dfc91f8462f70ace026c96584e9