Bug 12144 - Incorrect response to FSCTL_SET_COMPRESSION request
Incorrect response to FSCTL_SET_COMPRESSION request
Status: ASSIGNED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services
4.3.6
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-08-12 07:01 UTC by Nick Barrett
Modified: 2017-01-11 07:59 UTC (History)
4 users (show)

See Also:


Attachments
proposed fix - as send to samba-technical ML (5.73 KB, patch)
2016-10-03 23:27 UTC, David Disseldorp
jra: review+
Details
patchset for 4.4, cherry picked from master (6.61 KB, patch)
2016-10-06 11:24 UTC, David Disseldorp
jra: review+
Details
patchset for 4.5, cherry picked from master (6.61 KB, patch)
2016-10-06 11:25 UTC, David Disseldorp
jra: review+
Details
Follow up patch-set fixing set-compression(format=NONE) (3.67 KB, patch)
2017-01-09 16:26 UTC, David Disseldorp
jra: review+
Details
git-am fix for 4.5.next, 4.4.next. (4.07 KB, patch)
2017-01-10 01:03 UTC, Jeremy Allison
ddiss: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Barrett 2016-08-12 07:01:37 UTC
Hi there,

I have been using FreeNAS for some time now to host my Hyper-V virtual machine files in a CIFS share. I have been using verion FreeNAS-9.3-STABLE-201602031011 for some time. I recently tried to upgrade to FreeNAS 9.10 and found Hyper-V was no longer able to run my VMs.

FreeNAS 9.3 uses Samba version 4.1.21. I think FreeNAS 9.10 uses 4.3.6.

I used WireShark to see if I could see a difference between the two and found that Hyper-V would make a FSCTL_SET_COMPRESSION on a .vsv file. In FreeNAS 9.3, the response would be STATUS_INVALID_DEVICE_REQUEST but in FreeNAS 9.10 the response would be STATUS_NOT_SUPPORTED.

From looking at https://msdn.microsoft.com/en-us/library/cc232043.aspx I believe that STATUS_NOT_SUPPORTED is not a valid response and it should still be returning STATUS_INVALID_DEVICE_REQUEST.

While I am a software developer, I'm totally out of my depth with this so if I've got it all wrong, then my humblest apologies. If not, then I this is able to be resolved. Please let me know if I can provide any more information.

Cheers,

Nick Barrett
Comment 1 Nick Barrett 2016-08-27 11:13:01 UTC
I've been digging a bit deeper into this. I had a look at the source at https://github.com/samba-team/samba and did a search for FSCTL_SET_COMPRESSION. This lead me to https://github.com/samba-team/samba/blob/82801f9ec895deb9536a2b0a4e0ce4b3d5853220/source3/smbd/smb2_ioctl_filesys.c.

In smb2_ioctl_filesys.c on line 450, there's a case of FSCTL_SET_COMPRESSION in a switch statement. This calls a method called fsctl_set_cmprn.

In fsctl_set_cmprn on line 102, there appears to be a check to see if the file system supports compression. If it doesn't it returns NT_STATUS_NOT_SUPPORTED. I believe this is the problem. I believe it should return STATUS_INVALID_DEVICE_REQUEST instead.

I'm hoping someone can verify this =)
Comment 2 David Disseldorp 2016-10-03 19:36:36 UTC
Thanks a lot for your thorough investigation here Nick! Your analysis looks correct. The only thing I'd like to check before committing the  NT_STATUS_NOT_SUPPORTED->STATUS_INVALID_DEVICE_REQUEST change is test against ReFS on Server 2016 (TP5), which doesn't support compression.
Comment 3 David Disseldorp 2016-10-03 23:27:48 UTC
Created attachment 12539 [details]
proposed fix - as send to samba-technical ML
Comment 4 David Disseldorp 2016-10-05 12:05:31 UTC
Comment on attachment 12539 [details]
proposed fix - as send to samba-technical ML

@Jeremy: please review if you have a spare moment. I think this change is correct, based on the ReFS responses to the newly added tests.
Comment 5 Jeremy Allison 2016-10-05 18:35:04 UTC
Comment on attachment 12539 [details]
proposed fix - as send to samba-technical ML

LGTM. Pushing to master.
Comment 6 David Disseldorp 2016-10-06 10:03:21 UTC
(In reply to Jeremy Allison from comment #5)
> LGTM. Pushing to master.

Thank you Jeremy, and thanks for the ntvfs knownfail addition (I missed that in local testing).

I'll attach v4-4-test and v4-5-test based patches for maintenance.
Comment 7 David Disseldorp 2016-10-06 11:24:40 UTC
Created attachment 12548 [details]
patchset for 4.4, cherry picked from master
Comment 8 David Disseldorp 2016-10-06 11:25:14 UTC
Created attachment 12549 [details]
patchset for 4.5, cherry picked from master
Comment 9 Josh Paetzel 2016-10-06 16:41:27 UTC
Is there a patch for 4.3.x?  We aren't planning to use 4.4 until late November.
Comment 10 David Disseldorp 2016-10-06 17:02:05 UTC
(In reply to Josh Paetzel from comment #9)
> Is there a patch for 4.3.x?  We aren't planning to use 4.4 until late
> November.

4.3.x takes security fixes only now that 4.4 and 4.5 are available:
https://wiki.samba.org/index.php/Samba_Release_Planning

That said, the patches should apply cleanly to 4.3.x, if you care to carry them downstream. The smbtorture test changes need not be applied.
Comment 11 Jeremy Allison 2016-10-06 19:34:30 UTC
Re-assigning to Karolin for inclusion in 4.5.next, 4.4.next.
Comment 12 Karolin Seeger 2016-10-19 06:58:27 UTC
(In reply to Jeremy Allison from comment #11)
Pushed to autobuild-v4-{5,4}-test.
Comment 13 Karolin Seeger 2016-10-25 07:40:32 UTC
(In reply to Karolin Seeger from comment #12)
Pushed to both branches.
Closing out bug report.

Thanks!
Comment 14 Nick Barrett 2016-10-25 07:59:33 UTC
My thanks to you all David, Jeremy and Karolin for getting this sorted. It's very much appreciated =)
Comment 15 Nick Barrett 2016-12-05 00:43:01 UTC
Hello again,

Thanks again for everyones effort on this one.

It's my understanding that the team at FreeNas have back ported the fix to the version that is currently used in FreeNas.

I have tested a build that includes this fix and I am still experiencing the same issue.

I've had a read over the code to the best of my ability. I believe the GET_COMPRESSION has been dealt with, but the SET_COMPRESSION is still returning NT_STATUS_NOT_SUPPORTED instead of NT_STATUS_INVALID_DEVICE_REQUEST.

I believe the line of code that needs to be changed is here https://github.com/samba-team/samba/blob/master/source3/smbd/smb2_ioctl_filesys.c#L109

As such, I think the test test_ioctl_compress_notsup_set here (https://github.com/samba-team/samba/blob/f6f6263f1f03db965b64b5d7858e44ab5ffb0aeb/source4/torture/smb2/ioctl.c#L2609) needs to check for the return of NT_STATUS_INVALID_DEVICE_REQUEST as well.
Comment 16 David Disseldorp 2016-12-05 10:10:02 UTC
(In reply to Nick Barrett from comment #15)
> Hello again,
> 
> Thanks again for everyones effort on this one.

Thanks for your persistence :)

> It's my understanding that the team at FreeNas have back ported the fix to
> the version that is currently used in FreeNas.
> 
> I have tested a build that includes this fix and I am still experiencing the
> same issue.
> 
> I've had a read over the code to the best of my ability. I believe the
> GET_COMPRESSION has been dealt with, but the SET_COMPRESSION is still
> returning NT_STATUS_NOT_SUPPORTED instead of
> NT_STATUS_INVALID_DEVICE_REQUEST.

Yes, that's correct.

> I believe the line of code that needs to be changed is here
> https://github.com/samba-team/samba/blob/master/source3/smbd/
> smb2_ioctl_filesys.c#L109
> 
> As such, I think the test test_ioctl_compress_notsup_set here
> (https://github.com/samba-team/samba/blob/
> f6f6263f1f03db965b64b5d7858e44ab5ffb0aeb/source4/torture/smb2/ioctl.c#L2609)
> needs to check for the return of NT_STATUS_INVALID_DEVICE_REQUEST as well.

test_ioctl_compress_notsup_set() passes against a Windows Server 2016 ReFS share, which doesn't support compression.

Would you possibly be able to test your application against a Windows Server 2016 ReFS share and provide a network trace?
Comment 17 David Disseldorp 2016-12-20 10:08:10 UTC
(In reply to David Disseldorp from comment #16)
...
> test_ioctl_compress_notsup_set() passes against a Windows Server 2016 ReFS
> share, which doesn't support compression.
> 
> Would you possibly be able to test your application against a Windows Server
> 2016 ReFS share and provide a network trace?

Hi Nick, did you get a chance to test this?
Comment 18 Nick Barrett 2016-12-20 19:53:20 UTC
Hi David,

Thanks for your patience. I haven't had the chance yet. So far I have upgraded my failover cluster to Server 2016. Next step is to add some storage so I can try an ReFS share. I'll let you know how I get on.
Comment 19 Nick Barrett 2017-01-04 03:09:21 UTC
I've done some further investigation.

Setup:
I have upgraded all my Hyper-V servers to Windows Server 2016.
For my test, I added a spare hard drive to one of the servers and formatted it using ReFS. I then shared this ReFS formatted drive.

One thing that I noticed this time around, was that when Hyper-V is calling FSCTL_SET_COMPRESSION on the share, it is asking for COMPRESSION_FORMAT_NONE. Unfortunately I have lost the wireshark dumps from last time, so I can't confirm that they also asked for COMPRESSION_FORMAT_NONE but I presume they did.

Tests:
Looking through the WireShark dumps, the ReFS formatted share returns STATUS_SUCCESS. You mentioned that ReFS doesn't support compression, so I'm guessing it responds with STATUS_SUCCESS because the request is asking for COMPRESSION_FORMAT_NONE. I don't really have a way to test for requesting other compression formats as I am just using Hyper-V for my tests.

I took a WireShark dump again of Samba version 4.1.21 (which is the version found in FreeNAS-9.3-STABLE-201602031011 that I am successfully running) and I can confirm that it returns STATUS_INVALID_DEVICE_REQUEST for the same request.

Thoughts:
In my case it appears that Hyper-V is trying to ensure there isn't any compression going on. My FreeNas server has compression turned off on the volume that's hosting the VM files. I don't know how this would all work for a volume that does have compression enabled.
For a non-compressed volume though, here's my guess as to what the response values should be to a FSCTL_SET_COMPRESSION request:
Request                      Response
COMPRESSION_FORMAT_NONE      STATUS_SUCCESS
COMPRESSION_FORMAT_DEFAULT   STATUS_INVALID_DEVICE_REQUEST
COMPRESSION_FORMAT_LZNT1     STATUS_INVALID_DEVICE_REQUEST

If you need any other specific test run then let me know. =)
Comment 20 Nick Barrett 2017-01-04 06:27:56 UTC
It took me a while but I created a VM running FreeNas using FreeNAS-9.10.2 (a476f16) which uses Samba 4.4.5.

I can confirm that a FSCTL_SET_COMPRESSION request for COMPRESSION_FORMAT_NONE results in a STATUS_NOT_SUPPORTED response which causes Hyper-V to fail.
Comment 21 David Disseldorp 2017-01-04 10:23:13 UTC
Thanks for the update, Nick!

I'll work on a test + fix for this behaviour, and will post here once I have something working against Windows 2016 (ReFS) and Samba.
Comment 22 David Disseldorp 2017-01-05 19:51:43 UTC
(In reply to Nick Barrett from comment #19)
...
> Request                      Response
> COMPRESSION_FORMAT_NONE      STATUS_SUCCESS
> COMPRESSION_FORMAT_DEFAULT   STATUS_INVALID_DEVICE_REQUEST
> COMPRESSION_FORMAT_LZNT1     STATUS_INVALID_DEVICE_REQUEST

As mentioned earlier, f6f6263f1f03db965b64b5d7858e44ab5ffb0aeb confirmed that STATUS_NOT_SUPPORTED is the correct response, if we're to use Windows 2016 ReFS behaviour as the benchmark.

What I did miss in the previous change was the STATUS_SUCCESS response to set(COMPRESSION_FORMAT_NONE) requests - thanks for doing the investigation on this.
The latest set of patches (sent to samba-technical with you in cc) fix this on the server side.
Comment 23 Nick Barrett 2017-01-05 22:14:11 UTC
Hi David,

After rereading your responses a couple of times, I now understand what you are saying about the result to requesting actual file compression being STATUS_NOT_SUPPORTED instead of STATUS_INVALID_DEVICE_REQUEST. For some reason I got it stuck in my head. My apologies for keep bringing it up.

Thanks again for sorting this. =)
Comment 24 David Disseldorp 2017-01-09 16:26:05 UTC
Created attachment 12805 [details]
Follow up patch-set fixing set-compression(format=NONE)

Same patch-set, as sent to the technical ML:
https://lists.samba.org/archive/samba-technical/2017-January/117945.html

Behaviour confirmed against WS2016 ReFS.
Comment 25 Jeremy Allison 2017-01-09 18:30:36 UTC
Comment on attachment 12805 [details]
Follow up patch-set fixing set-compression(format=NONE)

LGTM. Pushed to master.
Comment 26 Jeremy Allison 2017-01-10 01:03:03 UTC
Created attachment 12806 [details]
git-am fix for 4.5.next, 4.4.next.

Cherry-pick from what went into master. Applies cleanly to 4.5.next, 4.4.next.
Comment 27 David Disseldorp 2017-01-10 16:26:35 UTC
Comment on attachment 12806 [details]
git-am fix for 4.5.next, 4.4.next.

Thanks Jeremy!
@Karo: please pull for 4.5.next and 4.4.next.
Comment 28 Karolin Seeger 2017-01-11 07:59:09 UTC
(In reply to David Disseldorp from comment #27)
Pushed to autobuild-v4-{5,4}-test.