| Message ID | 20240705032725.39761-1-hobin.woo@samsung.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ksmb: discard write access to the directory open | expand |
2024년 7월 5일 (금) 오후 12:27, Hobin Woo <hobin.woo@samsung.com>님이 작성: > > may_open() does not allow a directory to be opened with the write access. > However, some writing flags set by client result in adding write access > on server, making ksmbd incompatible with FUSE file system. Simply, let's > discard the write access when opening a directory. > > list_add corruption. next is NULL. > ------------[ cut here ]------------ > kernel BUG at lib/list_debug.c:26! > pc : __list_add_valid+0x88/0xbc > lr : __list_add_valid+0x88/0xbc > Call trace: > __list_add_valid+0x88/0xbc > fuse_finish_open+0x11c/0x170 > fuse_open_common+0x284/0x5e8 > fuse_dir_open+0x14/0x24 > do_dentry_open+0x2a4/0x4e0 > dentry_open+0x50/0x80 > smb2_open+0xbe4/0x15a4 > handle_ksmbd_work+0x478/0x5ec > process_one_work+0x1b4/0x448 > worker_thread+0x25c/0x430 > kthread+0x104/0x1d4 > ret_from_fork+0x10/0x20 > > Signed-off-by: Yoonho Shin <yoonho.shin@samsung.com> > Signed-off-by: Hobin Woo <hobin.woo@samsung.com> > --- > v2: > - Describe 'is_dir' in function parameters of 'smb2_create_open_flags' Applied it to #ksmbd-for-next-next. Thanks for your patch!
On 7/5/24 5:27 AM, Hobin Woo wrote: > may_open() does not allow a directory to be opened with the write access. > However, some writing flags set by client result in adding write access > on server, making ksmbd incompatible with FUSE file system. Simply, let's > discard the write access when opening a directory. afair this should cause a failure like EACCESS or EISDIR and just be ignored. What does a Windows server return in this case, or Samba for that matter as it (hopefully) will behave like Windows. -slow
2024년 7월 5일 (금) 오후 8:54, Ralph Boehme <slow@samba.org>님이 작성: > > On 7/5/24 5:27 AM, Hobin Woo wrote: > > may_open() does not allow a directory to be opened with the write access. > > However, some writing flags set by client result in adding write access > > on server, making ksmbd incompatible with FUSE file system. Simply, let's > > discard the write access when opening a directory. > > afair this should cause a failure like EACCESS or EISDIR and just be > ignored. What does a Windows server return in this case, or Samba for > that matter as it (hopefully) will behave like Windows. From what I've checked the packet dump, Samba doesn't return any errors in the same case. Samba seems to open it with O_RDONLY if it is directory, So there is no problem, is it right? > > -slow >
On 7/5/24 2:33 PM, Namjae Jeon wrote: > 2024년 7월 5일 (금) 오후 8:54, Ralph Boehme <slow@samba.org>님이 작성: >> >> On 7/5/24 5:27 AM, Hobin Woo wrote: >>> may_open() does not allow a directory to be opened with the write access. >>> However, some writing flags set by client result in adding write access >>> on server, making ksmbd incompatible with FUSE file system. Simply, let's >>> discard the write access when opening a directory. >> >> afair this should cause a failure like EACCESS or EISDIR and just be >> ignored. What does a Windows server return in this case, or Samba for >> that matter as it (hopefully) will behave like Windows. > From what I've checked the packet dump, Samba doesn't return any > errors in the same case. > Samba seems to open it with O_RDONLY if it is directory, So there is > no problem, is it right? Hm, it seems my memory is playing tricks on me. Samba indeed forces O_RDONLY for directories in the servers and ignores the client requested access mask. Interestingly we don't seem to have any test for this case, at least I couldn't find any with a quick search. Quickly putting together a torture test shows Windows behaves the same. MS-FSA doesn't mention any such check should be done for directories as well. Getinfo on such a handle even returns the original unmodified client access mask, including the write access. Sorry for the noise! :) -slow
On 7/5/2024 9:16 AM, Ralph Boehme wrote: > On 7/5/24 2:33 PM, Namjae Jeon wrote: >> 2024년 7월 5일 (금) 오후 8:54, Ralph Boehme <slow@samba.org>님이 작성: >>> >>> On 7/5/24 5:27 AM, Hobin Woo wrote: >>>> may_open() does not allow a directory to be opened with the write >>>> access. >>>> However, some writing flags set by client result in adding write access >>>> on server, making ksmbd incompatible with FUSE file system. Simply, >>>> let's >>>> discard the write access when opening a directory. >>> >>> afair this should cause a failure like EACCESS or EISDIR and just be >>> ignored. What does a Windows server return in this case, or Samba for >>> that matter as it (hopefully) will behave like Windows. >> From what I've checked the packet dump, Samba doesn't return any >> errors in the same case. >> Samba seems to open it with O_RDONLY if it is directory, So there is >> no problem, is it right? > > Hm, it seems my memory is playing tricks on me. Samba indeed forces > O_RDONLY for directories in the servers and ignores the client requested > access mask. Interestingly we don't seem to have any test for this case, > at least I couldn't find any with a quick search. Quickly putting > together a torture test shows Windows behaves the same. MS-FSA doesn't > mention any such check should be done for directories as well. Getinfo > on such a handle even returns the original unmodified client access > mask, including the write access. > > Sorry for the noise! :) > > -slow I would ask to see that the SMB3 protocol test suite results are not impacted by this change, and ideally the various Linux/XFS tests as well. I don't see any indication these were run? The other thing I want to point out is that the crash reported in the commit message is a FUSE failure. Why is that not a bug, and why does the message not justify why the "fix" is in ksmbd? Tom.
fixed typo in commit title and applied to ksmbd-for-next as well On Fri, Jul 5, 2024 at 6:40 AM Namjae Jeon <linkinjeon@kernel.org> wrote: > > 2024년 7월 5일 (금) 오후 12:27, Hobin Woo <hobin.woo@samsung.com>님이 작성: > > > > may_open() does not allow a directory to be opened with the write access. > > However, some writing flags set by client result in adding write access > > on server, making ksmbd incompatible with FUSE file system. Simply, let's > > discard the write access when opening a directory. > > > > list_add corruption. next is NULL. > > ------------[ cut here ]------------ > > kernel BUG at lib/list_debug.c:26! > > pc : __list_add_valid+0x88/0xbc > > lr : __list_add_valid+0x88/0xbc > > Call trace: > > __list_add_valid+0x88/0xbc > > fuse_finish_open+0x11c/0x170 > > fuse_open_common+0x284/0x5e8 > > fuse_dir_open+0x14/0x24 > > do_dentry_open+0x2a4/0x4e0 > > dentry_open+0x50/0x80 > > smb2_open+0xbe4/0x15a4 > > handle_ksmbd_work+0x478/0x5ec > > process_one_work+0x1b4/0x448 > > worker_thread+0x25c/0x430 > > kthread+0x104/0x1d4 > > ret_from_fork+0x10/0x20 > > > > Signed-off-by: Yoonho Shin <yoonho.shin@samsung.com> > > Signed-off-by: Hobin Woo <hobin.woo@samsung.com> > > --- > > v2: > > - Describe 'is_dir' in function parameters of 'smb2_create_open_flags' > Applied it to #ksmbd-for-next-next. > Thanks for your patch! >
diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c index e7e07891781b..7d26fdcebbf9 100644 --- a/fs/smb/server/smb2pdu.c +++ b/fs/smb/server/smb2pdu.c @@ -2051,15 +2051,22 @@ int smb2_tree_connect(struct ksmbd_work *work) * @access: file access flags * @disposition: file disposition flags * @may_flags: set with MAY_ flags + * @is_dir: is creating open flags for directory * * Return: file open flags */ static int smb2_create_open_flags(bool file_present, __le32 access, __le32 disposition, - int *may_flags) + int *may_flags, + bool is_dir) { int oflags = O_NONBLOCK | O_LARGEFILE; + if (is_dir) { + access &= ~FILE_WRITE_DESIRE_ACCESS_LE; + ksmbd_debug(SMB, "Discard write access to a directory\n"); + } + if (access & FILE_READ_DESIRED_ACCESS_LE && access & FILE_WRITE_DESIRE_ACCESS_LE) { oflags |= O_RDWR; @@ -3167,7 +3174,9 @@ int smb2_open(struct ksmbd_work *work) open_flags = smb2_create_open_flags(file_present, daccess, req->CreateDisposition, - &may_flags); + &may_flags, + req->CreateOptions & FILE_DIRECTORY_FILE_LE || + (file_present && S_ISDIR(d_inode(path.dentry)->i_mode))); if (!test_tree_conn_flag(tcon, KSMBD_TREE_CONN_FLAG_WRITABLE)) { if (open_flags & (O_CREAT | O_TRUNC)) {