diff mbox series

ksmbd: add default data stream name in FILE_STREAM_INFORMATION

Message ID 20210918120239.96627-1-linkinjeon@kernel.org (mailing list archive)
State New, archived
Headers show
Series ksmbd: add default data stream name in FILE_STREAM_INFORMATION | expand

Commit Message

Namjae Jeon Sept. 18, 2021, 12:02 p.m. UTC
Windows client expect to get default stream name(::DATA) in
FILE_STREAM_INFORMATION response even if there is no stream data in file.
This patch fix update failure when writing ppt or doc files.

Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ksmbd/smb2pdu.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

Tom Talpey Sept. 18, 2021, 12:39 p.m. UTC | #1
This doesn't appear to be what's documented in MS-FSA section 2.1.5.11.29.
There, it appears to call for returning an empty stream list,
and STATUS_SUCCESS, when no streams are present.

Also, why does the code special-case directories? The conditionals
on StreamSize and StreamAllocation size are entirely redundant,
after the top-level if (!S_ISDIR...), btw.

I'd suggest asking Microsoft dochelp for clarification before
implementing this.

Tom.

On 9/18/2021 8:02 AM, Namjae Jeon wrote:
> Windows client expect to get default stream name(::DATA) in
> FILE_STREAM_INFORMATION response even if there is no stream data in file.
> This patch fix update failure when writing ppt or doc files.
> 
> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
> ---
>   fs/ksmbd/smb2pdu.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 49a1ca75f427..301605e0cbf7 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -4465,17 +4465,15 @@ static void get_file_stream_info(struct ksmbd_work *work,
>   		file_info->NextEntryOffset = cpu_to_le32(next);
>   	}
>   
> -	if (nbytes) {
> +	if (!S_ISDIR(stat.mode)) {
>   		file_info = (struct smb2_file_stream_info *)
>   			&rsp->Buffer[nbytes];
>   		streamlen = smbConvertToUTF16((__le16 *)file_info->StreamName,
>   					      "::$DATA", 7, conn->local_nls, 0);
>   		streamlen *= 2;
>   		file_info->StreamNameLength = cpu_to_le32(streamlen);
> -		file_info->StreamSize = S_ISDIR(stat.mode) ? 0 :
> -			cpu_to_le64(stat.size);
> -		file_info->StreamAllocationSize = S_ISDIR(stat.mode) ? 0 :
> -			cpu_to_le64(stat.size);
> +		file_info->StreamSize = 0;
> +		file_info->StreamAllocationSize = 0;
>   		nbytes += sizeof(struct smb2_file_stream_info) + streamlen;
>   	}
>   
>
Namjae Jeon Sept. 18, 2021, 9:36 p.m. UTC | #2
Hi Tom,
2021-09-18 21:39 GMT+09:00, Tom Talpey <tom@talpey.com>:
> This doesn't appear to be what's documented in MS-FSA section 2.1.5.11.29.
> There, it appears to call for returning an empty stream list,
> and STATUS_SUCCESS, when no streams are present.
As I know, Specification doesn't describe all windows behavior. So
smbtorture testcase test such corner cases.
>
> Also, why does the code special-case directories? The conditionals
> on StreamSize and StreamAllocation size are entirely redundant,
> after the top-level if (!S_ISDIR...), btw.
As I know, default data stream(::DATA) is for only file, not
directory. And streams tests in smbtorture pass, Please see:

$ ./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234 smb2.streams.dir
smbtorture 4.10.6
Using seed 1631969885
time: 2021-09-18 12:58:05.268099
test: dir
time: 2021-09-18 12:58:05.268848
teststreams\stream.txt:Stream One
(../../source4/torture/smb2/streams.c:248) opening non-existent directory stream
(../../source4/torture/smb2/streams.c:263) opening basedir  stream
(../../source4/torture/smb2/streams.c:279) opening basedir ::$DATA stream
(../../source4/torture/smb2/streams.c:295) list the streams on the basedir
time: 2021-09-18 12:58:05.313561
success: dir
The command "./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.dir" exited with 0.
0.11s$ ./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234 smb2.streams.io
smbtorture 4.10.6
Using seed 1631969885
time: 2021-09-18 12:58:05.357178
test: io
time: 2021-09-18 12:58:05.357884
(../../source4/torture/smb2/streams.c:335) creating a stream on a
non-existent file
(../../source4/torture/smb2/streams.c:355) check that open of base
file is allowed
(../../source4/torture/smb2/streams.c:362) writing to stream
(../../source4/torture/smb2/streams.c:377) modifying stream
(../../source4/torture/smb2/streams.c:387) creating a stream2 on a existing file
(../../source4/torture/smb2/streams.c:394) modifying stream
(../../source4/torture/smb2/streams.c:431) deleting stream
(../../source4/torture/smb2/streams.c:444) delete a stream via delete-on-close
(../../source4/torture/smb2/streams.c:476) deleting file
time: 2021-09-18 12:58:05.422830
success: io
The command "./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.io" exited with 0.
0.09s$ ./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.sharemodes
smbtorture 4.10.6
Using seed 1631969885
time: 2021-09-18 12:58:05.467364
test: sharemodes
time: 2021-09-18 12:58:05.468186
(../../source4/torture/smb2/streams.c:592) Testing stream share mode conflicts
time: 2021-09-18 12:58:05.521840
success: sharemodes
The command "./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.sharemodes" exited with 0.
0.10s$ ./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.names
smbtorture 4.10.6
Using seed 1631969885
time: 2021-09-18 12:58:05.565595
test: names
time: 2021-09-18 12:58:05.566433
(../../source4/torture/smb2/streams.c:869) testing stream names
time: 2021-09-18 12:58:05.626686
success: names
The command "./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.names" exited with 0.
0.12s$ ./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.names2
smbtorture 4.10.6
Using seed 1631969885
time: 2021-09-18 12:58:05.671829
test: names2
time: 2021-09-18 12:58:05.672704
(../../source4/torture/smb2/streams.c:1160) testing stream names
time: 2021-09-18 12:58:05.751656
success: names2
The command "./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.names2" exited with 0.
0.08s$ ./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.names3
smbtorture 4.10.6
Using seed 1631969885
time: 2021-09-18 12:58:05.796248
test: names3
time: 2021-09-18 12:58:05.797156
(../../source4/torture/smb2/streams.c:1288) testing case insensitive
stream names
time: 2021-09-18 12:58:05.835048
success: names3
The command "./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.names3" exited with 0.
0.10s$ ./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.rename
smbtorture 4.10.6
Using seed 1631969885
time: 2021-09-18 12:58:05.879226
test: rename
time: 2021-09-18 12:58:05.880108
(../../source4/torture/smb2/streams.c:1379) testing stream renames
time: 2021-09-18 12:58:05.938765
success: rename
The command "./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.rename" exited with 0.
0.11s$ ./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.rename2
smbtorture 4.10.6
Using seed 1631969885
time: 2021-09-18 12:58:05.983358
test: rename2
time: 2021-09-18 12:58:05.984182
(../../source4/torture/smb2/streams.c:1504) Checking SMB2 rename of a
stream using :<stream>
(../../source4/torture/smb2/streams.c:1518) Checking SMB2 rename of an
overwriting stream using :<stream>
(../../source4/torture/smb2/streams.c:1548) Checking SMB2 rename of a
stream using <base>:<stream>
(../../source4/torture/smb2/streams.c:1559) Checking SMB2 rename to
default stream using :<stream>
time: 2021-09-18 12:58:06.048102
success: rename2
The command "./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.rename2" exited with 0.
0.10s$ ./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.create-disposition
smbtorture 4.10.6
Using seed 1631969886
time: 2021-09-18 12:58:06.094196
test: create-disposition
time: 2021-09-18 12:58:06.095203
(../../source4/torture/smb2/streams.c:1666) Checking create disp: open
(../../source4/torture/smb2/streams.c:1680) Checking create disp: overwrite
(../../source4/torture/smb2/streams.c:1694) Checking create disp: overwrite_if
(../../source4/torture/smb2/streams.c:1712) Checking create disp: supersede
(../../source4/torture/smb2/streams.c:1732) Checking create disp:
overwrite_if on stream
time: 2021-09-18 12:58:06.148406
success: create-disposition
The command "./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.create-disposition" exited with 0.
0.10s$ ./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.attributes
smbtorture 4.10.6
Using seed 1631969886
time: 2021-09-18 12:58:06.192777
test: attributes
time: 2021-09-18 12:58:06.193624
(../../source4/torture/smb2/streams.c:1810) testing attribute setting on stream
time: 2021-09-18 12:58:06.256161
success: attributes
The command "./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.attributes" exited with 0.
0.09s$ ./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.zero-byte
smbtorture 4.10.6
Using seed 1631969886
time: 2021-09-18 12:58:06.302540
test: zero-byte
time: 2021-09-18 12:58:06.303431
(../../source4/torture/smb2/streams.c:512) Check 0 byte named stream
time: 2021-09-18 12:58:06.353826
success: zero-byte
The command "./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.zero-byte" exited with 0.
0.21s$ ./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.basefile-rename-with-open-stream
smbtorture 4.10.6
Using seed 1631969886
time: 2021-09-18 12:58:06.397700
test: basefile-rename-with-open-stream
time: 2021-09-18 12:58:06.398478
Creating file with stream
Writing to stream
Renaming base file
time: 2021-09-18 12:58:06.565832
success: basefile-rename-with-open-stream
The command "./bin/smbtorture //127.0.0.1/cifsd-test3/ -Utestuser%1234
smb2.streams.basefile-rename-with-open-stream" exited with 0.
>
> I'd suggest asking Microsoft dochelp for clarification before
> implementing this.
I'm not sure I'll get the answer I want from the dochelp quickly. I
vote we can apply this patch to fix existing issue, and parallel
contact the dochelp and update the code if there is some points.

Thanks!
>
> Tom.
>
> On 9/18/2021 8:02 AM, Namjae Jeon wrote:
>> Windows client expect to get default stream name(::DATA) in
>> FILE_STREAM_INFORMATION response even if there is no stream data in file.
>> This patch fix update failure when writing ppt or doc files.
>>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>   fs/ksmbd/smb2pdu.c | 8 +++-----
>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index 49a1ca75f427..301605e0cbf7 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -4465,17 +4465,15 @@ static void get_file_stream_info(struct ksmbd_work
>> *work,
>>   		file_info->NextEntryOffset = cpu_to_le32(next);
>>   	}
>>
>> -	if (nbytes) {
>> +	if (!S_ISDIR(stat.mode)) {
>>   		file_info = (struct smb2_file_stream_info *)
>>   			&rsp->Buffer[nbytes];
>>   		streamlen = smbConvertToUTF16((__le16 *)file_info->StreamName,
>>   					      "::$DATA", 7, conn->local_nls, 0);
>>   		streamlen *= 2;
>>   		file_info->StreamNameLength = cpu_to_le32(streamlen);
>> -		file_info->StreamSize = S_ISDIR(stat.mode) ? 0 :
>> -			cpu_to_le64(stat.size);
>> -		file_info->StreamAllocationSize = S_ISDIR(stat.mode) ? 0 :
>> -			cpu_to_le64(stat.size);
>> +		file_info->StreamSize = 0;
>> +		file_info->StreamAllocationSize = 0;
>>   		nbytes += sizeof(struct smb2_file_stream_info) + streamlen;
>>   	}
>>
>>
>
Steve French Sept. 20, 2021, 4:47 a.m. UTC | #3
On Sat, Sep 18, 2021 at 9:06 PM Tom Talpey <tom@talpey.com> wrote:
>
> This doesn't appear to be what's documented in MS-FSA section 2.1.5.11.29.
> There, it appears to call for returning an empty stream list,
> and STATUS_SUCCESS, when no streams are present.

I tried a quick test to Windows and it does appear to return $DATA
stream by default:

# ./smbinfo filestreaminfo /mnt/junk
Name: ::$DATA
Size: 179765 bytes
Allocation size: 196608 bytes

when I tried the same thing to a directory on a share mounted to Windows 10
(NTFS), I get no streams returned.

So it does look like default stream ($DATA) is only returned for files

> Also, why does the code special-case directories? The conditionals
> on StreamSize and StreamAllocation size are entirely redundant,
> after the top-level if (!S_ISDIR...), btw.
>
> I'd suggest asking Microsoft dochelp for clarification before
> implementing this.
>
> Tom.
>
> On 9/18/2021 8:02 AM, Namjae Jeon wrote:
> > Windows client expect to get default stream name(::DATA) in
> > FILE_STREAM_INFORMATION response even if there is no stream data in file.
> > This patch fix update failure when writing ppt or doc files.
Steve French Sept. 20, 2021, 4:53 a.m. UTC | #4
Patch looks correct - tentatively merged pending testing (and additional review)

On Sun, Sep 19, 2021 at 11:47 PM Steve French <smfrench@gmail.com> wrote:
>
> On Sat, Sep 18, 2021 at 9:06 PM Tom Talpey <tom@talpey.com> wrote:
> >
> > This doesn't appear to be what's documented in MS-FSA section 2.1.5.11.29.
> > There, it appears to call for returning an empty stream list,
> > and STATUS_SUCCESS, when no streams are present.
>
> I tried a quick test to Windows and it does appear to return $DATA
> stream by default:
>
> # ./smbinfo filestreaminfo /mnt/junk
> Name: ::$DATA
> Size: 179765 bytes
> Allocation size: 196608 bytes
>
> when I tried the same thing to a directory on a share mounted to Windows 10
> (NTFS), I get no streams returned.
>
> So it does look like default stream ($DATA) is only returned for files
>
> > Also, why does the code special-case directories? The conditionals
> > on StreamSize and StreamAllocation size are entirely redundant,
> > after the top-level if (!S_ISDIR...), btw.
> >
> > I'd suggest asking Microsoft dochelp for clarification before
> > implementing this.
> >
> > Tom.
> >
> > On 9/18/2021 8:02 AM, Namjae Jeon wrote:
> > > Windows client expect to get default stream name(::DATA) in
> > > FILE_STREAM_INFORMATION response even if there is no stream data in file.
> > > This patch fix update failure when writing ppt or doc files.
>
>
>
>
> --
> Thanks,
>
> Steve
Tom Talpey Sept. 20, 2021, 4:08 p.m. UTC | #5
On 9/20/2021 12:47 AM, Steve French wrote:
> On Sat, Sep 18, 2021 at 9:06 PM Tom Talpey <tom@talpey.com> wrote:
>>
>> This doesn't appear to be what's documented in MS-FSA section 2.1.5.11.29.
>> There, it appears to call for returning an empty stream list,
>> and STATUS_SUCCESS, when no streams are present.
> 
> I tried a quick test to Windows and it does appear to return $DATA
> stream by default:
> 
> # ./smbinfo filestreaminfo /mnt/junk
> Name: ::$DATA
> Size: 179765 bytes
> Allocation size: 196608 bytes

Ok, so the implication is that the default stream is indeed always
present, if the filesystem supports streams. The language in MS-FSA
would therefore be correct, but a bit vague. I agree that returning
a ::$DATA for ordinary files is appropriate.

> when I tried the same thing to a directory on a share mounted to Windows 10
> (NTFS), I get no streams returned.
> 
> So it does look like default stream ($DATA) is only returned for files


My concern here is, what's so special about directories? A special file
or fifo, a symlink or reparse/junction, etc. Is it appropriate to cons
up a ::$DATA for these? What should the size values be, if so?

Tom.


>> Also, why does the code special-case directories? The conditionals
>> on StreamSize and StreamAllocation size are entirely redundant,
>> after the top-level if (!S_ISDIR...), btw.
>>
>> I'd suggest asking Microsoft dochelp for clarification before
>> implementing this.
>>
>> Tom.
>>
>> On 9/18/2021 8:02 AM, Namjae Jeon wrote:
>>> Windows client expect to get default stream name(::DATA) in
>>> FILE_STREAM_INFORMATION response even if there is no stream data in file.
>>> This patch fix update failure when writing ppt or doc files.
> 
> 
> 
>
Namjae Jeon Sept. 20, 2021, 4:34 p.m. UTC | #6
2021-09-21 1:08 GMT+09:00, Tom Talpey <tom@talpey.com>:
> On 9/20/2021 12:47 AM, Steve French wrote:
>> On Sat, Sep 18, 2021 at 9:06 PM Tom Talpey <tom@talpey.com> wrote:
>>>
>>> This doesn't appear to be what's documented in MS-FSA section
>>> 2.1.5.11.29.
>>> There, it appears to call for returning an empty stream list,
>>> and STATUS_SUCCESS, when no streams are present.
>>
>> I tried a quick test to Windows and it does appear to return $DATA
>> stream by default:
>>
>> # ./smbinfo filestreaminfo /mnt/junk
>> Name: ::$DATA
>> Size: 179765 bytes
>> Allocation size: 196608 bytes
>
> Ok, so the implication is that the default stream is indeed always
> present, if the filesystem supports streams. The language in MS-FSA
> would therefore be correct, but a bit vague. I agree that returning
> a ::$DATA for ordinary files is appropriate.
>
>> when I tried the same thing to a directory on a share mounted to Windows
>> 10
>> (NTFS), I get no streams returned.
>>
>> So it does look like default stream ($DATA) is only returned for files
>
>
> My concern here is, what's so special about directories? A special file
> or fifo, a symlink or reparse/junction, etc. Is it appropriate to cons
> up a ::$DATA for these? What should the size values be, if so?
Special files in linux(ksmbd share) is showing as regular file on
windows client.
>
> Tom.
>
>
>>> Also, why does the code special-case directories? The conditionals
>>> on StreamSize and StreamAllocation size are entirely redundant,
>>> after the top-level if (!S_ISDIR...), btw.
>>>
>>> I'd suggest asking Microsoft dochelp for clarification before
>>> implementing this.
>>>
>>> Tom.
>>>
>>> On 9/18/2021 8:02 AM, Namjae Jeon wrote:
>>>> Windows client expect to get default stream name(::DATA) in
>>>> FILE_STREAM_INFORMATION response even if there is no stream data in
>>>> file.
>>>> This patch fix update failure when writing ppt or doc files.
>>
>>
>>
>>
>
Steve French Sept. 20, 2021, 4:36 p.m. UTC | #7
On Mon, Sep 20, 2021 at 11:08 AM Tom Talpey <tom@talpey.com> wrote:
>
> On 9/20/2021 12:47 AM, Steve French wrote:
> > On Sat, Sep 18, 2021 at 9:06 PM Tom Talpey <tom@talpey.com> wrote:
> >>
> >> This doesn't appear to be what's documented in MS-FSA section 2.1.5.11.29.
> >> There, it appears to call for returning an empty stream list,
> >> and STATUS_SUCCESS, when no streams are present.
> >
> > I tried a quick test to Windows and it does appear to return $DATA
> > stream by default:
> >
> > # ./smbinfo filestreaminfo /mnt/junk
> > Name: ::$DATA
> > Size: 179765 bytes
> > Allocation size: 196608 bytes
>
> Ok, so the implication is that the default stream is indeed always
> present, if the filesystem supports streams. The language in MS-FSA
> would therefore be correct, but a bit vague. I agree that returning
> a ::$DATA for ordinary files is appropriate.
>
> > when I tried the same thing to a directory on a share mounted to Windows 10
> > (NTFS), I get no streams returned.
> >
> > So it does look like default stream ($DATA) is only returned for files
>
>
> My concern here is, what's so special about directories? A special file
> or fifo, a symlink or reparse/junction, etc. Is it appropriate to cons
> up a ::$DATA for these? What should the size values be, if so?

Here are examples (share exported Windows 10 NTFS) of files vs.
directories with or without streams, note that directories do not show
the "default stream" $DATA but files do, but both support adding
streams, and reparse points at least in the case of hardlinks to files
show the target's streams.  It is a bit confusing because the Windows
"dir /R" command filters out the ($DATA) stream returned on files even
though it is returned across the network.

# smbinfo filestreaminfo /mnt/dir-without-stream/


# smbinfo filestreaminfo /mnt/dir-with-stream/
Name: test_stream
Size: 19 bytes
Allocation size: 24 bytes


# smbinfo filestreaminfo /mnt/file-without-stream
Name: ::$DATA
Size: 24 bytes
Allocation size: 24 bytes


# smbinfo filestreaminfo /mnt/file-with-stream
Name: ::$DATA
Size: 17 bytes
Allocation size: 24 bytes

Name: test_stream
Size: 19 bytes
Allocation size: 24 bytes


# smbinfo filestreaminfo /mnt/link-file-with-stream
Name: ::$DATA
Size: 17 bytes
Allocation size: 24 bytes

Name: test_stream
Size: 19 bytes
Allocation size: 24 bytes


# smbinfo filestreaminfo /mnt/link-file-without-stream
Name: ::$DATA
Size: 24 bytes
Allocation size: 24 bytes

Note that locally on Windows "DIR /R" filters out the $DATA (default) stream

C:\shares\scratch>dir /R \shares\scratch
 Volume in drive C is OSDisk
 Volume Serial Number is 0AF4-9CC1

 Directory of C:\shares\scratch

09/20/2021  11:32 AM    <DIR>          .
09/20/2021  11:32 AM    <DIR>          ..
09/20/2021  11:16 AM    <DIR>          dir-with-stream
                                    19 dir-with-stream:test_stream:$DATA
09/20/2021  11:15 AM    <DIR>          dir-without-stream
09/20/2021  11:14 AM                17 file-with-stream
                                    19 file-with-stream:test_stream:$DATA
09/20/2021  11:14 AM                24 file-without-stream
09/20/2021  11:14 AM                17 link-file-with-stream
                                    19 link-file-with-stream:test_stream:$DATA
09/20/2021  11:14 AM                24 link-file-without-stream


--
Thanks,

Steve
Tom Talpey Sept. 21, 2021, 6:19 p.m. UTC | #8
On 9/20/2021 12:34 PM, Namjae Jeon wrote:
> 2021-09-21 1:08 GMT+09:00, Tom Talpey <tom@talpey.com>:
<snip>
>>
>> My concern here is, what's so special about directories? A special file
>> or fifo, a symlink or reparse/junction, etc. Is it appropriate to cons
>> up a ::$DATA for these? What should the size values be, if so?
> Special files in linux(ksmbd share) is showing as regular file on
> windows client.

This brings up an interesting second question. Is that a good thing?

It seems risky, and perhaps wrong, that one can open such a special file 
and read or write it over SMB3. I can see allowing read attributes, etc, 
but certainly not full access. To me, at a minimum the read or write 
should be failed by ksmbd, if not the CREATE itself. ksmbd should not be 
representing these as ordinary files.

Tom.
Steve French Sept. 21, 2021, 7:17 p.m. UTC | #9
On Tue, Sep 21, 2021 at 1:19 PM Tom Talpey <tom@talpey.com> wrote:
>
> On 9/20/2021 12:34 PM, Namjae Jeon wrote:
> > 2021-09-21 1:08 GMT+09:00, Tom Talpey <tom@talpey.com>:
> <snip>
> >>
> >> My concern here is, what's so special about directories? A special file
> >> or fifo, a symlink or reparse/junction, etc. Is it appropriate to cons
> >> up a ::$DATA for these? What should the size values be, if so?
> > Special files in linux(ksmbd share) is showing as regular file on
> > windows client.
>
> This brings up an interesting second question. Is that a good thing?
>
> It seems risky, and perhaps wrong, that one can open such a special file
> and read or write it over SMB3. I can see allowing read attributes, etc,
> but certainly not full access. To me, at a minimum the read or write
> should be failed by ksmbd, if not the CREATE itself. ksmbd should not be
> representing these as ordinary files.

Symlinks, FIFO, and char and block devices have special reparse points
defined by Windows for these, but they would not typically be read, in
some sense they act more like an empty directory.   The exception
seems to be hard links.
diff mbox series

Patch

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 49a1ca75f427..301605e0cbf7 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -4465,17 +4465,15 @@  static void get_file_stream_info(struct ksmbd_work *work,
 		file_info->NextEntryOffset = cpu_to_le32(next);
 	}
 
-	if (nbytes) {
+	if (!S_ISDIR(stat.mode)) {
 		file_info = (struct smb2_file_stream_info *)
 			&rsp->Buffer[nbytes];
 		streamlen = smbConvertToUTF16((__le16 *)file_info->StreamName,
 					      "::$DATA", 7, conn->local_nls, 0);
 		streamlen *= 2;
 		file_info->StreamNameLength = cpu_to_le32(streamlen);
-		file_info->StreamSize = S_ISDIR(stat.mode) ? 0 :
-			cpu_to_le64(stat.size);
-		file_info->StreamAllocationSize = S_ISDIR(stat.mode) ? 0 :
-			cpu_to_le64(stat.size);
+		file_info->StreamSize = 0;
+		file_info->StreamAllocationSize = 0;
 		nbytes += sizeof(struct smb2_file_stream_info) + streamlen;
 	}