RE: updated ksmbd (cifsd)

["Namjae Jeon" <namjae.jeon@samsung.com>]

 
> I just looked briefly, but I'm wondering about a few things:
> 
> 1. The xattr's to store additional meta data are not compatible with
>    Samba's way of storing things:
>     https://git.samba.org/?p=samba.git;a=blob;f=librpc/idl/xattr.idl
> 
>    In order to make it possible to use the same filesystem with both servers
>    it would be great if the well established way used in Samba would be used
>    as well.
Yes, Maybe... I will check it after sending ksmbd to linux-next.
 
> 2. Why does smb2_set_info_sec() have fp->saccess |= FILE_SHARE_DELETE_LE; ?
Because of smb2.acls.GENERIC failure.

TESTING FILE GENERIC BITS
get the original sd
Testing generic bits 0x00000000
time: 2020-12-15 00:00:37.940992
failure: GENERIC [
(../../source4/torture/smb2/acls.c:439) Incorrect status NT_STATUS_SHARING_VIOLATION - should be NT_STATUS_OK

I really don't understand this test. This testcase expect that FILE_DELETE is set in response if desired access
of smb2 open is FILE_MAXIMAL_ACCESS.
I don't know why smb2 open should be allowed although FILE_SHARE_DELETE is not set in previous open in this test.
Can you give me a hint ?

> 3. Why does ksmbd_override_fsids() only reset cred->fs[g|u]id, but group_info
>    is kept unchanged, I guess at least the groups array should be set to be empty.
Yes, We need to handle the groups list. Will fix it.

> 4. What is work->saved_cred_level used for in ksmbd_override_fsids()?
>    It seems to be unused and adds a lot of complexity.
ksmbd_override_fsids could be called recursively.
work->saved_cred_level prevents ksmbd from overriding fs[g|u]id again.
 
> 5. Documentation/filesystems/cifsd.rst and fs/cifsd/Kconfig still references https://github.com/cifsd-team/cifsd-tools
>   instead of https://github.com/cifsd-team/ksmbd-tools
Okay. Will update it.

> 6. Why is SMB_SERVER_CHECK_CAP_NET_ADMIN an compile time option and why is it off by default?
>    I think the behavior should be enforced without a switch.
I can make it default yes. Can you explain more why it should be enforced ?

> These are just some initial thoughts.
Thanks for sharing your review!
> 
> metze