Bug 12144 - Incorrect response to FSCTL_SET_COMPRESSION request
Incorrect response to FSCTL_SET_COMPRESSION request
Status: RESOLVED FIXED
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: 2016-10-25 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

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 =)