Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[David Howells]

I've been debugging a failure in xfstests generic/286 whereby llseek() return
-EIO and am thinking that the bug is in Samba.  The test can be distilled to:

	mount //my/share /mnt -ooptions
	truncate -s 100M /mnt/a
	for i in $(seq 0 5 100); do xfs_io -c "w ${i}M 1M" /mnt/a; done
	xfstests-dev/src/seek_copy_test /mnt/a /mnt/b

This creates a big sparse file, makes 21 1MiB extents at 5MiB intervals and
then tries to copy them one at a time, using SEEK_DATA/SEEK_HOLE to enumerate
each extent.

I can see the error in the protocol being returned from the server:

   16 2.353013398  192.168.6.2 → 192.168.6.1  SMB2 206 Ioctl Request FSCTL_QUERY_ALLOCATED_RANGES File: a
   17 2.353220936  192.168.6.1 → 192.168.6.2  SMB2 143 Ioctl Response, Error: STATUS_BUFFER_TOO_SMALL

Stracing Samba shows:

    lseek(30, 94371840, SEEK_HOLE)          = 95420416
    lseek(30, 95420416, SEEK_DATA)          = 99614720
    lseek(30, 99614720, SEEK_HOLE)          = 100663296
    lseek(30, 100663296, SEEK_DATA)         = 104857600
    lseek(30, 104857600, SEEK_HOLE)         = 105906176
    sendmsg(35, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\0\0\0I", iov_len=4}, {iov_base=NULL, iov_len=0}, {iov_base="\376SMB@\0\1\0#\0\0\300\v\0\n\0\1\0\0\0\0\0\0\0\265\2\0\0\0\0\0\0"..., iov_len=64}, {iov_base="\t\0\0\0\0\0\0\0", iov_len=8}, {iov_base="\0", iov_len=1}], msg_iovlen=5, msg_controllen=0, msg_flags=0}, MSG_DONTWAIT|MSG_NOSIGNAL) = 77

You can see the error code in the sendmsg() as "...#\0\0\300...".

Turning debugging on on Samba shows:

    [2024/05/21 23:56:58.727547,  2] ../../source3/smbd/smb2_ioctl_filesys.c:707(fsctl_qar)
      QAR output len 336 exceeds max 16
    [2024/05/21 23:56:58.727652,  3] ../../source3/smbd/smb2_server.c:4031(smbd_smb2_request_error_ex)
      smbd_smb2_request_error_ex: smbd_smb2_request_error_ex: idx[1] status[NT_STATUS_BUFFER_TOO_SMALL] || at ../../source3/smbd/smb2_ioctl.c:353

The "exceeds max 16" indicates the "Max Ioctl Out Size" setting passed in the
Ioctl Request frame (which can be seen as 16 in the full packet trace).  336
is 21 * 16.

This stems from the smb2_llseek() function in the cifs filesystem in the Linux
kernel calling:

	rc = SMB2_ioctl(xid, tcon, cfile->fid.persistent_fid,
			cfile->fid.volatile_fid,
			FSCTL_QUERY_ALLOCATED_RANGES,
			(char *)&in_data, sizeof(in_data),
			sizeof(struct file_allocated_range_buffer),
			(char **)&out_data, &out_data_len);

where the max_out_data_len parameter is sizeof() you can see, allowing for
just a single element to be returned.  The file_allocated_range_buffer struct
is just:

	struct file_allocated_range_buffer {
		__le64	file_offset;
		__le64	length;
	} __packed;

which is 16 bytes - hence the maximum output data seen by Samba.


Now, cifs only wants the next extent.  It fills in the input data with the
base file position and the EOF:

	in_data.file_offset = cpu_to_le64(offset);
	in_data.length = cpu_to_le64(i_size_read(inode));

and asks the server to retrieve the first allocated extent within this range.

In Samba, however, fsctl_qar() does not pass in_max_output to
fsctl_qar_seek_fill() and therefore doesn't limit the amount returned.  The
fill loop seems only to be limited by the maximum file offset and not the max
buffer size.

The:

	if (out_output->length > in_max_output) {
		DEBUG(2, ("QAR output len %lu exceeds max %lu\n",
			  (unsigned long)out_output->length,
			  (unsigned long)in_max_output));
		data_blob_free(out_output);
		return NT_STATUS_BUFFER_TOO_SMALL;
	}

check at the end of fsctl_qar() generates the complaint.

I think that fsctl_qar() should probably just discard the excess records.

Looking at:

	https://learn.microsoft.com/en-us/windows/win32/api/winioctl/ni-winioctl-fsctl_query_allocated_ranges 

it's not obvious what should be done if the available records won't fit.

Samba: samba-4.19.6-1.fc39.x86_64
Linux: 6.10 pre -rc1.

David

Re: Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[David Disseldorp]

Hi David,

On Wed, 22 May 2024 00:53:59 +0100, David Howells via samba-technical wrote:

> I've been debugging a failure in xfstests generic/286 whereby llseek() return
> -EIO and am thinking that the bug is in Samba.  The test can be distilled to:
> 
> 	mount //my/share /mnt -ooptions
> 	truncate -s 100M /mnt/a
> 	for i in $(seq 0 5 100); do xfs_io -c "w ${i}M 1M" /mnt/a; done
> 	xfstests-dev/src/seek_copy_test /mnt/a /mnt/b

Thanks for providing the reproducer and detailed analysis...

> This creates a big sparse file, makes 21 1MiB extents at 5MiB intervals and
> then tries to copy them one at a time, using SEEK_DATA/SEEK_HOLE to enumerate
> each extent.
> 
> I can see the error in the protocol being returned from the server:
> 
>    16 2.353013398  192.168.6.2 → 192.168.6.1  SMB2 206 Ioctl Request FSCTL_QUERY_ALLOCATED_RANGES File: a
>    17 2.353220936  192.168.6.1 → 192.168.6.2  SMB2 143 Ioctl Response, Error: STATUS_BUFFER_TOO_SMALL
> 
> Stracing Samba shows:
> 
>     lseek(30, 94371840, SEEK_HOLE)          = 95420416
>     lseek(30, 95420416, SEEK_DATA)          = 99614720
>     lseek(30, 99614720, SEEK_HOLE)          = 100663296
>     lseek(30, 100663296, SEEK_DATA)         = 104857600
>     lseek(30, 104857600, SEEK_HOLE)         = 105906176
>     sendmsg(35, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\0\0\0I", iov_len=4}, {iov_base=NULL, iov_len=0}, {iov_base="\376SMB@\0\1\0#\0\0\300\v\0\n\0\1\0\0\0\0\0\0\0\265\2\0\0\0\0\0\0"..., iov_len=64}, {iov_base="\t\0\0\0\0\0\0\0", iov_len=8}, {iov_base="\0", iov_len=1}], msg_iovlen=5, msg_controllen=0, msg_flags=0}, MSG_DONTWAIT|MSG_NOSIGNAL) = 77
> 
> You can see the error code in the sendmsg() as "...#\0\0\300...".
> 
> Turning debugging on on Samba shows:
> 
>     [2024/05/21 23:56:58.727547,  2] ../../source3/smbd/smb2_ioctl_filesys.c:707(fsctl_qar)
>       QAR output len 336 exceeds max 16
>     [2024/05/21 23:56:58.727652,  3] ../../source3/smbd/smb2_server.c:4031(smbd_smb2_request_error_ex)
>       smbd_smb2_request_error_ex: smbd_smb2_request_error_ex: idx[1] status[NT_STATUS_BUFFER_TOO_SMALL] || at ../../source3/smbd/smb2_ioctl.c:353
> 
> The "exceeds max 16" indicates the "Max Ioctl Out Size" setting passed in the
> Ioctl Request frame (which can be seen as 16 in the full packet trace).  336
> is 21 * 16.
> 
> This stems from the smb2_llseek() function in the cifs filesystem in the Linux
> kernel calling:
> 
> 	rc = SMB2_ioctl(xid, tcon, cfile->fid.persistent_fid,
> 			cfile->fid.volatile_fid,
> 			FSCTL_QUERY_ALLOCATED_RANGES,
> 			(char *)&in_data, sizeof(in_data),
> 			sizeof(struct file_allocated_range_buffer),
> 			(char **)&out_data, &out_data_len);
> 
> where the max_out_data_len parameter is sizeof() you can see, allowing for
> just a single element to be returned.  The file_allocated_range_buffer struct
> is just:
> 
> 	struct file_allocated_range_buffer {
> 		__le64	file_offset;
> 		__le64	length;
> 	} __packed;
> 
> which is 16 bytes - hence the maximum output data seen by Samba.
> 
> 
> Now, cifs only wants the next extent.  It fills in the input data with the
> base file position and the EOF:
> 
> 	in_data.file_offset = cpu_to_le64(offset);
> 	in_data.length = cpu_to_le64(i_size_read(inode));
> 
> and asks the server to retrieve the first allocated extent within this range.
> 
> In Samba, however, fsctl_qar() does not pass in_max_output to
> fsctl_qar_seek_fill() and therefore doesn't limit the amount returned.  The
> fill loop seems only to be limited by the maximum file offset and not the max
> buffer size.
> 
> The:
> 
> 	if (out_output->length > in_max_output) {
> 		DEBUG(2, ("QAR output len %lu exceeds max %lu\n",
> 			  (unsigned long)out_output->length,
> 			  (unsigned long)in_max_output));
> 		data_blob_free(out_output);
> 		return NT_STATUS_BUFFER_TOO_SMALL;
> 	}
> 
> check at the end of fsctl_qar() generates the complaint.
> 
> I think that fsctl_qar() should probably just discard the excess records.
> 
> Looking at:
> 
> 	https://learn.microsoft.com/en-us/windows/win32/api/winioctl/ni-winioctl-fsctl_query_allocated_ranges 
> 
> it's not obvious what should be done if the available records won't fit.

MS-FSCC from https://winprotocoldoc.blob.core.windows.net/productionwindowsarchives/MS-FSCC/%5bMS-FSCC%5d.pdf
is a more protocol-specific reference here, but it's still unclear
regarding partial / truncated responses.

We do have an existing test for the STATUS_BUFFER_TOO_SMALL response in
test_ioctl_sparse_qar_malformed(), which would have been run against a
Windows server to confirm matching protocol behaviour. But it doesn't
cover partial responses.

I think the best way to proceed here would be to capture traffic for the
same workload against a Windows SMB server. This could be don't by using
your cifs.ko workload or extending test_ioctl_sparse_qar_malformed().

Cheers, David

Re: Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[David Howells]

David Disseldorp <ddiss@samba.org> wrote:
> ...
> I think the best way to proceed here would be to capture traffic for the
> same workload against a Windows SMB server. This could be don't by using
> your cifs.ko workload or extending test_ioctl_sparse_qar_malformed().

I don't have a windows server I can try.  Steve may be able to try that.

David

Re: Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[David Disseldorp]

On Wed, 22 May 2024 11:36:25 +0100, David Howells wrote:

> David Disseldorp <ddiss@samba.org> wrote:
> > ...
> > I think the best way to proceed here would be to capture traffic for the
> > same workload against a Windows SMB server. This could be don't by using
> > your cifs.ko workload or extending test_ioctl_sparse_qar_malformed().  
> 
> I don't have a windows server I can try.  Steve may be able to try that.

I'll put it on my todo list for the next time I have a Windows VM
sitting around. I do recall testing Samba alongside Windows when doing
the initial implementation, but QAR is very FS block / allocation size
specific, so 1:1 behaviour isn't straightforward (nor is it required by
the FSCTL_QUERY_ALLOCATED_RANGES / FSCTL_SET_ZERO_DATA specs).

Cheers, David

Re: Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[ronnie sahlberg]

On Thu, 23 May 2024 at 14:54, David Disseldorp <ddiss@samba.org> wrote:
>
> On Wed, 22 May 2024 11:36:25 +0100, David Howells wrote:
>
> > David Disseldorp <ddiss@samba.org> wrote:
> > > ...
> > > I think the best way to proceed here would be to capture traffic for the
> > > same workload against a Windows SMB server. This could be don't by using
> > > your cifs.ko workload or extending test_ioctl_sparse_qar_malformed().
> >
> > I don't have a windows server I can try.  Steve may be able to try that.
>
> I'll put it on my todo list for the next time I have a Windows VM
> sitting around. I do recall testing Samba alongside Windows when doing
> the initial implementation, but QAR is very FS block / allocation size
> specific, so 1:1 behaviour isn't straightforward (nor is it required by
> the FSCTL_QUERY_ALLOCATED_RANGES / FSCTL_SET_ZERO_DATA specs).

I recall talks with microsoft folks on the list about these as well
and I think they were problematic because ntfs and other windows
filesystems do not really have the same semantics at tha tlinux has.
I think one issue is that "set-zero" nothing more than a hint
and depending on phase-of-moon and how the server feels might
sometimes punch a hole,
might sometimes fill in a hole  or do a combination.
The behaviour was hinted could even differ from one run to the next on
the same server, because reasons.

There were other issues as well making the behavior unpredictable
becase one could not assume the alignment of blocks, or if it would
even be constant throughout the file.


It might be best to just ignore tests that fail in this area. And just
accept that some things, at best, is a best-effort approximation.
(as long as dataloss does not happen, of course. That is never acceptable)
At the end of the day it is a lot of guesswork and trying to fit a
square peg (unpredictable ntfs behavior) into a round hole (linux vfs
api).



>
> Cheers, David
>

Re: Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[David Howells]

ronnie sahlberg <ronniesahlberg@gmail.com> wrote:

> On Thu, 23 May 2024 at 14:54, David Disseldorp <ddiss@samba.org> wrote:
> It might be best to just ignore tests that fail in this area. And just
> accept that some things, at best, is a best-effort approximation.
> (as long as dataloss does not happen, of course. That is never acceptable)
> At the end of the day it is a lot of guesswork and trying to fit a
> square peg (unpredictable ntfs behavior) into a round hole (linux vfs
> api).

The problem is that it essentially renders SEEK_DATA/SEEK_HOLE unusable for
applications on cifs.  If there's more than one extent above the starting
position, they'll fail with EIO.  The only way to do it is to provide for a
sufficiently large buffer to accommodate however many extents that there are
(and there could be millions, in theory) in order to get just the first one.

David

Re: Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[ronnie sahlberg]

On Thu, 23 May 2024 at 16:21, David Howells <dhowells@redhat.com> wrote:
>
> ronnie sahlberg <ronniesahlberg@gmail.com> wrote:
>
> > On Thu, 23 May 2024 at 14:54, David Disseldorp <ddiss@samba.org> wrote:
> > It might be best to just ignore tests that fail in this area. And just
> > accept that some things, at best, is a best-effort approximation.
> > (as long as dataloss does not happen, of course. That is never acceptable)
> > At the end of the day it is a lot of guesswork and trying to fit a
> > square peg (unpredictable ntfs behavior) into a round hole (linux vfs
> > api).
>
> The problem is that it essentially renders SEEK_DATA/SEEK_HOLE unusable for
> applications on cifs.  If there's more than one extent above the starting
> position, they'll fail with EIO.  The only way to do it is to provide for a
> sufficiently large buffer to accommodate however many extents that there are
> (and there could be millions, in theory) in order to get just the first one.

Wait, I didn't read all the text in the initial posts correctly.
Do you mean if you ask for "max x bytes of response, enough for n
entries" then if there
are > n entries on the server you get nothing back?

I am pretty sure Windows will return as many entries as fits in the
reponse out-data-size
nad some error code.
But you are saying that instead of returning a truncated out-blob that
samba will return nothing?
I will test this tomorrow on a win16 server.



>
> David
>

Re: Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[David Howells]

ronnie sahlberg <ronniesahlberg@gmail.com> wrote:

> > The problem is that it essentially renders SEEK_DATA/SEEK_HOLE unusable for
> > applications on cifs.  If there's more than one extent above the starting
> > position, they'll fail with EIO.  The only way to do it is to provide for a
> > sufficiently large buffer to accommodate however many extents that there are
> > (and there could be millions, in theory) in order to get just the first one.
> 
> Wait, I didn't read all the text in the initial posts correctly.
> Do you mean if you ask for "max x bytes of response, enough for n
> entries" then if there
> are > n entries on the server you get nothing back?
> 
> I am pretty sure Windows will return as many entries as fits in the
> reponse out-data-size
> nad some error code.
> But you are saying that instead of returning a truncated out-blob that
> samba will return nothing?

It returns a STATUS_BUFFER_TOO_SMALL error if there's more than one extent
record to return.

David

Re: Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[Tom Talpey]

On 5/23/2024 2:36 AM, David Howells wrote:
> ronnie sahlberg <ronniesahlberg@gmail.com> wrote:
> 
>>> The problem is that it essentially renders SEEK_DATA/SEEK_HOLE unusable for
>>> applications on cifs.  If there's more than one extent above the starting
>>> position, they'll fail with EIO.  The only way to do it is to provide for a
>>> sufficiently large buffer to accommodate however many extents that there are
>>> (and there could be millions, in theory) in order to get just the first one.
>>
>> Wait, I didn't read all the text in the initial posts correctly.
>> Do you mean if you ask for "max x bytes of response, enough for n
>> entries" then if there
>> are > n entries on the server you get nothing back?
>>
>> I am pretty sure Windows will return as many entries as fits in the
>> reponse out-data-size
>> nad some error code.
>> But you are saying that instead of returning a truncated out-blob that > If OutputBufferSize < ((OutputBufferIndex + 1) * 
sizeof(FILE_ALLOCATED_RANGE_BUFFER)) then:
> 
>     Set Status to STATUS_BUFFER_OVERFLOW.

>> samba will return nothing?
> 
> It returns a STATUS_BUFFER_TOO_SMALL error if there's more than one extent
> record to return.

Yeah, I think this is a Samba server issue. Ronnie is right that it
should return a partial response and a STATUS_BUFFER_OVERFLOW error
indicating that it's partial. It's not supposed to return
STATUS_BUFFER_TOO_SMALL unless the entire buffer is less than one
entry.

MS-FSA section 2.5.10.22

https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-fsa/385dec98-90fe-477f-9789-20a47a7b8467

Tom.

Re: Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[Paulo Alcantara]

Tom Talpey <tom@talpey.com> writes:

> Yeah, I think this is a Samba server issue. Ronnie is right that it
> should return a partial response and a STATUS_BUFFER_OVERFLOW error
> indicating that it's partial. It's not supposed to return
> STATUS_BUFFER_TOO_SMALL unless the entire buffer is less than one
> entry.
>
> MS-FSA section 2.5.10.22
>
> https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-fsa/385dec98-90fe-477f-9789-20a47a7b8467

Yes.  I've just tested it against Windows Server 2022 and it correctly
returns STATUS_BUFFER_OVERFLOW.

Re: Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[Jeremy Allison]

On Thu, May 23, 2024 at 12:28:35PM -0300, Paulo Alcantara wrote:
>Tom Talpey <tom@talpey.com> writes:
>
>> Yeah, I think this is a Samba server issue. Ronnie is right that it
>> should return a partial response and a STATUS_BUFFER_OVERFLOW error
>> indicating that it's partial. It's not supposed to return
>> STATUS_BUFFER_TOO_SMALL unless the entire buffer is less than one
>> entry.
>>
>> MS-FSA section 2.5.10.22
>>
>> https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-fsa/385dec98-90fe-477f-9789-20a47a7b8467
>
>Yes.  I've just tested it against Windows Server 2022 and it correctly
>returns STATUS_BUFFER_OVERFLOW.

Bug is in fsctl_qar():

         ndr_ret = ndr_push_struct_blob(out_output, mem_ctx, &qar_rsp,
                 (ndr_push_flags_fn_t)ndr_push_fsctl_query_alloced_ranges_rsp);
         if (ndr_ret != NDR_ERR_SUCCESS) {
                 DEBUG(0, ("failed to marshall QAR rsp\n"));
                 return NT_STATUS_INVALID_PARAMETER;
         }

         if (out_output->length > in_max_output) {
                 DEBUG(2, ("QAR output len %lu exceeds max %lu\n",
                           (unsigned long)out_output->length,
                           (unsigned long)in_max_output));
                 data_blob_free(out_output);
                 return NT_STATUS_BUFFER_TOO_SMALL;
         }

I'm guessing in this case we need to just truncate out_output->length
to in_max_output and return STATUS_BUFFER_OVERFLOW.

Re: Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[Stefan Metzmacher]

Am 24.05.24 um 00:49 schrieb Jeremy Allison via samba-technical:
> On Thu, May 23, 2024 at 12:28:35PM -0300, Paulo Alcantara wrote:
>> Tom Talpey <tom@talpey.com> writes:
>>
>>> Yeah, I think this is a Samba server issue. Ronnie is right that it
>>> should return a partial response and a STATUS_BUFFER_OVERFLOW error
>>> indicating that it's partial. It's not supposed to return
>>> STATUS_BUFFER_TOO_SMALL unless the entire buffer is less than one
>>> entry.
>>>
>>> MS-FSA section 2.5.10.22
>>>
>>> https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-fsa/385dec98-90fe-477f-9789-20a47a7b8467
>>
>> Yes.  I've just tested it against Windows Server 2022 and it correctly
>> returns STATUS_BUFFER_OVERFLOW.
> 
> Bug is in fsctl_qar():
> 
>          ndr_ret = ndr_push_struct_blob(out_output, mem_ctx, &qar_rsp,
>                  (ndr_push_flags_fn_t)ndr_push_fsctl_query_alloced_ranges_rsp);
>          if (ndr_ret != NDR_ERR_SUCCESS) {
>                  DEBUG(0, ("failed to marshall QAR rsp\n"));
>                  return NT_STATUS_INVALID_PARAMETER;
>          }
> 
>          if (out_output->length > in_max_output) {
>                  DEBUG(2, ("QAR output len %lu exceeds max %lu\n",
>                            (unsigned long)out_output->length,
>                            (unsigned long)in_max_output));
>                  data_blob_free(out_output);
>                  return NT_STATUS_BUFFER_TOO_SMALL;
>          }
> 
> I'm guessing in this case we need to just truncate out_output->length
> to in_max_output and return STATUS_BUFFER_OVERFLOW.

But I guess we should only truncate at multiples of the size of a single entry.
and return BUFFER_TOO_SMALL when it's to small to hold a single entry.

As far as I can see struct file_alloced_range_buf has a size of 16.

I guess fsctl_qar_buf_push() should also get a max_output_length argument
and should check early if additional 16 bytes can be appended
and return NT_STATUS_BUFFER_TOO_SMALL if not.

Then fsctl_qar_seek_fill() should catch NT_STATUS_BUFFER_TOO_SMALL and turn it
into STATUS_BUFFER_OVERFLOW if at least one element was filled in before.

metze

Re: Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[David Howells]

Hi Jeremy,

> Bug is in fsctl_qar():
> 
>         ndr_ret = ndr_push_struct_blob(out_output, mem_ctx, &qar_rsp,
>                 (ndr_push_flags_fn_t)ndr_push_fsctl_query_alloced_ranges_rsp);
>         if (ndr_ret != NDR_ERR_SUCCESS) {
>                 DEBUG(0, ("failed to marshall QAR rsp\n"));
>                 return NT_STATUS_INVALID_PARAMETER;
>         }
> 
>         if (out_output->length > in_max_output) {
>                 DEBUG(2, ("QAR output len %lu exceeds max %lu\n",
>                           (unsigned long)out_output->length,
>                           (unsigned long)in_max_output));
>                 data_blob_free(out_output);
>                 return NT_STATUS_BUFFER_TOO_SMALL;
>         }
> 
> I'm guessing in this case we need to just truncate out_output->length
> to in_max_output and return STATUS_BUFFER_OVERFLOW.

Do you perchance have a fix for this?  I'm seeing it cause failures in
xfstests when running against cifs connected to samba.

Thanks,
David

Re: Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[David Disseldorp]

Thanks for the follow up ping...

On Thu, 22 Aug 2024 23:26:00 +0100, David Howells wrote:

> >         if (out_output->length > in_max_output) {
> >                 DEBUG(2, ("QAR output len %lu exceeds max %lu\n",
> >                           (unsigned long)out_output->length,
> >                           (unsigned long)in_max_output));
> >                 data_blob_free(out_output);
> >                 return NT_STATUS_BUFFER_TOO_SMALL;
> >         }
> > 
> > I'm guessing in this case we need to just truncate out_output->length
> > to in_max_output and return STATUS_BUFFER_OVERFLOW.  
> 
> Do you perchance have a fix for this?  I'm seeing it cause failures in
> xfstests when running against cifs connected to samba.

I've proposed a fix via
https://gitlab.com/samba-team/samba/-/merge_requests/3775

If you want to try it yourself...

The following changes since commit b0996ed589a931902a304237d6c03efce2b16f6b:

  s3:tests: Fix spelling error (2024-08-22 10:38:09 +0000)

are available in the Git repository at:

  https://gitlab.com/ddiss/samba.git qar_rsp_truncation

for you to fetch changes up to 3c034c4d177ea2367b3131f813381d91c98ab7e1:

  s4:torture/smb2: test FSCTL_QUERY_ALLOCATED_RANGES truncation (2024-08-23 13:06:04 +0000)

----------------------------------------------------------------
David Disseldorp (2):
      smb2_ioctl: truncate FSCTL_QUERY_ALLOCATED_RANGES responses
      s4:torture/smb2: test FSCTL_QUERY_ALLOCATED_RANGES truncation

 source3/smbd/smb2_ioctl.c         |   4 +-
 source3/smbd/smb2_ioctl_filesys.c |  54 ++++++++------
 source4/libcli/smb2/ioctl.c       |   3 +-
 source4/torture/smb2/ioctl.c      | 150 +++++++++++++++++++++++++++++++++++++-
 4 files changed, 187 insertions(+), 24 deletions(-)

Re: Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[David Howells]

Hi David,

I tried to apply the patch to the Fedora samba rpm, but I get:

mold: error: undefined symbol: torture_assert_size_equal
>>> referenced by <artificial>
>>>               /tmp/ccVA4FUD.ltrans35.ltrans.o:(test_ioctl_sparse_qar_truncated.lto_priv.0)
>>> referenced by <artificial>
>>>               /tmp/ccVA4FUD.ltrans35.ltrans.o:(test_ioctl_sparse_qar_truncated.lto_priv.0)
>>> referenced by <artificial>
>>>               /tmp/ccVA4FUD.ltrans35.ltrans.o:(test_ioctl_sparse_qar_truncated.lto_priv.0)
collect2: error: ld returned 1 exit status


Do I actually need the torture test patch?

David

Re: Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[David Disseldorp]

Hi David,

On Wed, 28 Aug 2024 11:25:40 +0100, David Howells wrote:

> Hi David,
> 
> I tried to apply the patch to the Fedora samba rpm, but I get:
> 
> mold: error: undefined symbol: torture_assert_size_equal
> >>> referenced by <artificial>
> >>>               /tmp/ccVA4FUD.ltrans35.ltrans.o:(test_ioctl_sparse_qar_truncated.lto_priv.0)
> >>> referenced by <artificial>
> >>>               /tmp/ccVA4FUD.ltrans35.ltrans.o:(test_ioctl_sparse_qar_truncated.lto_priv.0)
> >>> referenced by <artificial>
> >>>               /tmp/ccVA4FUD.ltrans35.ltrans.o:(test_ioctl_sparse_qar_truncated.lto_priv.0)  
> collect2: error: ld returned 1 exit status

I've no idea which Samba version Fedora ships.
torture_assert_size_equal() was added to lib/torture/torture.h via
46f0c2696582 (samba >= 4.20.0).

> Do I actually need the torture test patch?

No, not if you can use your xfstests reproducer. The server fix is now
in Samba's master branch as commit 5e278a52646 ("smb2_ioctl: fix
truncated FSCTL_QUERY_ALLOCATED_RANGES responses").

Re: Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[David Howells]

Okay, that fixes the problem.

For reference, the file can be prepared thusly:

	xfs_io -c "pwrite 0 16M" -c "fpunch 0 1M" -c "fpunch 2M 1M" -c "fpunch 4M 1M" -c "fpunch 6M 1M" -c "fpunch 8M 1M" /xfstest.test/foo

and then the test run:

	xfs_io -c "seek -h 1" /xfstest.test/foo

Something like punch-hole is needed to set the sparse flag - otherwise QAR
isn't used by llseek().

So:

	Tested-by: David Howells <dhowells@redhat.com>

if you need it.

The Fedora samba version I applied this to was:

	samba-4.19.7-1.fc39.x86_64

though I had to drop the testing bits as they didn't build.

David

Re: Bug in Samba's implementation of FSCTL_QUERY_ALLOCATED_RANGES?

[David Disseldorp]

On Wed, 28 Aug 2024 12:52:25 +0100, David Howells via samba-technical wrote:

> Okay, that fixes the problem.
> 
> For reference, the file can be prepared thusly:
> 
> 	xfs_io -c "pwrite 0 16M" -c "fpunch 0 1M" -c "fpunch 2M 1M" -c "fpunch 4M 1M" -c "fpunch 6M 1M" -c "fpunch 8M 1M" /xfstest.test/foo
> 
> and then the test run:
> 
> 	xfs_io -c "seek -h 1" /xfstest.test/foo
> 
> Something like punch-hole is needed to set the sparse flag - otherwise QAR
> isn't used by llseek().
> 
> So:
> 
> 	Tested-by: David Howells <dhowells@redhat.com>
> 
> if you need it.

I appreciate the test feedback. The change has already been committed
(with you referenced in Reported-by).

> The Fedora samba version I applied this to was:
> 
> 	samba-4.19.7-1.fc39.x86_64
> 
> though I had to drop the testing bits as they didn't build.

Yes, as mentioned, samba >= 4.20 is needed for the test's
torture_assert_size_equal() calls.

Cheers, David