| Message ID | 20230927143009.8882-1-linkinjeon@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | ksmbd: not allow to open file if delelete on close bit is set | expand |
On 9/27/2023 10:30 AM, Namjae Jeon wrote: > Cthon test fail with the following error. > > check for proper open/unlink operation > nfsjunk files before unlink: > -rwxr-xr-x 1 root root 0 9월 25 11:03 ./nfs2y8Jm9 > ./nfs2y8Jm9 open; unlink ret = 0 > nfsjunk files after unlink: > -rwxr-xr-x 1 root root 0 9월 25 11:03 ./nfs2y8Jm9 > data compare ok > nfsjunk files after close: > ls: cannot access './nfs2y8Jm9': No such file or directory > special tests failed > > Cthon expect to second unlink failure when file is already unlinked. > ksmbd can not allow to open file if flags of ksmbd inode is set with > S_DEL_ON_CLS flags. > > Reported-by: Tom Talpey <tom@talpey.com> I don't remember reporting this. But more fundamentally, the Connectathon test is an NFS exerciser, and specific to NFS (and Posix) semantics. Delete-on-last-close is not one of them. Won't failing a new open break Windows semantics if it's enforced by default? Normally Windows checks for FILE_SHARE_DELETE before deciding this. Maybe other checks as well... Tom. > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> > --- > fs/smb/server/vfs_cache.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/fs/smb/server/vfs_cache.c b/fs/smb/server/vfs_cache.c > index f41f8d6108ce..f2e2a7cc24a9 100644 > --- a/fs/smb/server/vfs_cache.c > +++ b/fs/smb/server/vfs_cache.c > @@ -577,6 +577,11 @@ struct ksmbd_file *ksmbd_open_fd(struct ksmbd_work *work, struct file *filp) > goto err_out; > } > > + if (fp->f_ci->m_flags & S_DEL_ON_CLS) { > + ret = -ENOENT; > + goto err_out; > + } > + > ret = __open_id(&work->sess->file_table, fp, OPEN_ID_TYPE_VOLATILE_ID); > if (ret) { > ksmbd_inode_put(fp->f_ci);
2023-09-29 0:23 GMT+09:00, Tom Talpey <tom@talpey.com>: > On 9/27/2023 10:30 AM, Namjae Jeon wrote: >> Cthon test fail with the following error. >> >> check for proper open/unlink operation >> nfsjunk files before unlink: >> -rwxr-xr-x 1 root root 0 9월 25 11:03 ./nfs2y8Jm9 >> ./nfs2y8Jm9 open; unlink ret = 0 >> nfsjunk files after unlink: >> -rwxr-xr-x 1 root root 0 9월 25 11:03 ./nfs2y8Jm9 >> data compare ok >> nfsjunk files after close: >> ls: cannot access './nfs2y8Jm9': No such file or directory >> special tests failed >> >> Cthon expect to second unlink failure when file is already unlinked. >> ksmbd can not allow to open file if flags of ksmbd inode is set with >> S_DEL_ON_CLS flags. >> >> Reported-by: Tom Talpey <tom@talpey.com> > > I don't remember reporting this. You told me basic test of cthon run fine but special test fail. > > But more fundamentally, the Connectathon test is an NFS exerciser, and > specific to NFS (and Posix) semantics. Delete-on-last-close is not one > of them. > > Won't failing a new open break Windows semantics if it's enforced by > default? Normally Windows checks for FILE_SHARE_DELETE before deciding > this. Maybe other checks as well... > > Tom. > >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >> --- >> fs/smb/server/vfs_cache.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/fs/smb/server/vfs_cache.c b/fs/smb/server/vfs_cache.c >> index f41f8d6108ce..f2e2a7cc24a9 100644 >> --- a/fs/smb/server/vfs_cache.c >> +++ b/fs/smb/server/vfs_cache.c >> @@ -577,6 +577,11 @@ struct ksmbd_file *ksmbd_open_fd(struct ksmbd_work >> *work, struct file *filp) >> goto err_out; >> } >> >> + if (fp->f_ci->m_flags & S_DEL_ON_CLS) { >> + ret = -ENOENT; >> + goto err_out; >> + } >> + >> ret = __open_id(&work->sess->file_table, fp, >> OPEN_ID_TYPE_VOLATILE_ID); >> if (ret) { >> ksmbd_inode_put(fp->f_ci); >
On 9/28/2023 11:51 AM, Namjae Jeon wrote: > 2023-09-29 0:23 GMT+09:00, Tom Talpey <tom@talpey.com>: >> On 9/27/2023 10:30 AM, Namjae Jeon wrote: >>> Cthon test fail with the following error. >>> >>> check for proper open/unlink operation >>> nfsjunk files before unlink: >>> -rwxr-xr-x 1 root root 0 9월 25 11:03 ./nfs2y8Jm9 >>> ./nfs2y8Jm9 open; unlink ret = 0 >>> nfsjunk files after unlink: >>> -rwxr-xr-x 1 root root 0 9월 25 11:03 ./nfs2y8Jm9 >>> data compare ok >>> nfsjunk files after close: >>> ls: cannot access './nfs2y8Jm9': No such file or directory >>> special tests failed >>> >>> Cthon expect to second unlink failure when file is already unlinked. >>> ksmbd can not allow to open file if flags of ksmbd inode is set with >>> S_DEL_ON_CLS flags. >>> >>> Reported-by: Tom Talpey <tom@talpey.com> >> >> I don't remember reporting this. > You told me basic test of cthon run fine but special test fail. Well yes but I didn't say the failure was incorrect. Connectathon is a useful test, but it's not a protocol test. What makes it handy for me is that it's easy to deploy and run, and it is not a synthetic client, that is, it makes ordinary syscalls and exercices the local in-kernel client code. The "special" tests in particular are about NFS client quirks, and specifically quirks that were interesting in, like, 1999. They need to be taken with a huge lump of salt, and an even larger lump today. It's ok, I'm not concerned that you added my Reported-by. But it needs a lot more research before pushing this upstream. Tom. >> But more fundamentally, the Connectathon test is an NFS exerciser, and >> specific to NFS (and Posix) semantics. Delete-on-last-close is not one >> of them. >> >> Won't failing a new open break Windows semantics if it's enforced by >> default? Normally Windows checks for FILE_SHARE_DELETE before deciding >> this. Maybe other checks as well... >> >> Tom. >> >>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >>> --- >>> fs/smb/server/vfs_cache.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/fs/smb/server/vfs_cache.c b/fs/smb/server/vfs_cache.c >>> index f41f8d6108ce..f2e2a7cc24a9 100644 >>> --- a/fs/smb/server/vfs_cache.c >>> +++ b/fs/smb/server/vfs_cache.c >>> @@ -577,6 +577,11 @@ struct ksmbd_file *ksmbd_open_fd(struct ksmbd_work >>> *work, struct file *filp) >>> goto err_out; >>> } >>> >>> + if (fp->f_ci->m_flags & S_DEL_ON_CLS) { >>> + ret = -ENOENT; >>> + goto err_out; >>> + } >>> + >>> ret = __open_id(&work->sess->file_table, fp, >>> OPEN_ID_TYPE_VOLATILE_ID); >>> if (ret) { >>> ksmbd_inode_put(fp->f_ci); >> >
2023-09-29 0:23 GMT+09:00, Tom Talpey <tom@talpey.com>: > On 9/27/2023 10:30 AM, Namjae Jeon wrote: >> Cthon test fail with the following error. >> >> check for proper open/unlink operation >> nfsjunk files before unlink: >> -rwxr-xr-x 1 root root 0 9월 25 11:03 ./nfs2y8Jm9 >> ./nfs2y8Jm9 open; unlink ret = 0 >> nfsjunk files after unlink: >> -rwxr-xr-x 1 root root 0 9월 25 11:03 ./nfs2y8Jm9 >> data compare ok >> nfsjunk files after close: >> ls: cannot access './nfs2y8Jm9': No such file or directory >> special tests failed >> >> Cthon expect to second unlink failure when file is already unlinked. >> ksmbd can not allow to open file if flags of ksmbd inode is set with >> S_DEL_ON_CLS flags. >> >> Reported-by: Tom Talpey <tom@talpey.com> > > I don't remember reporting this. > > But more fundamentally, the Connectathon test is an NFS exerciser, and > specific to NFS (and Posix) semantics. Delete-on-last-close is not one > of them. > > Won't failing a new open break Windows semantics if it's enforced by > default? Normally Windows checks for FILE_SHARE_DELETE before deciding > this. Maybe other checks as well... cifs.ko send smb2 create with FILE_SHARE_DELETE to ksmbd. If sent without it, ksmbd return STATUS_SHARING_VIOLATION error to client. I have tested it against samba. ssamba handle it as STATUS_DELETE_PENDING error. > > Tom. > >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >> --- >> fs/smb/server/vfs_cache.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/fs/smb/server/vfs_cache.c b/fs/smb/server/vfs_cache.c >> index f41f8d6108ce..f2e2a7cc24a9 100644 >> --- a/fs/smb/server/vfs_cache.c >> +++ b/fs/smb/server/vfs_cache.c >> @@ -577,6 +577,11 @@ struct ksmbd_file *ksmbd_open_fd(struct ksmbd_work >> *work, struct file *filp) >> goto err_out; >> } >> >> + if (fp->f_ci->m_flags & S_DEL_ON_CLS) { >> + ret = -ENOENT; >> + goto err_out; >> + } >> + >> ret = __open_id(&work->sess->file_table, fp, >> OPEN_ID_TYPE_VOLATILE_ID); >> if (ret) { >> ksmbd_inode_put(fp->f_ci); >
2023-09-29 1:38 GMT+09:00, Tom Talpey <tom@talpey.com>: > On 9/28/2023 11:51 AM, Namjae Jeon wrote: >> 2023-09-29 0:23 GMT+09:00, Tom Talpey <tom@talpey.com>: >>> On 9/27/2023 10:30 AM, Namjae Jeon wrote: >>>> Cthon test fail with the following error. >>>> >>>> check for proper open/unlink operation >>>> nfsjunk files before unlink: >>>> -rwxr-xr-x 1 root root 0 9월 25 11:03 ./nfs2y8Jm9 >>>> ./nfs2y8Jm9 open; unlink ret = 0 >>>> nfsjunk files after unlink: >>>> -rwxr-xr-x 1 root root 0 9월 25 11:03 ./nfs2y8Jm9 >>>> data compare ok >>>> nfsjunk files after close: >>>> ls: cannot access './nfs2y8Jm9': No such file or directory >>>> special tests failed >>>> >>>> Cthon expect to second unlink failure when file is already unlinked. >>>> ksmbd can not allow to open file if flags of ksmbd inode is set with >>>> S_DEL_ON_CLS flags. >>>> >>>> Reported-by: Tom Talpey <tom@talpey.com> >>> >>> I don't remember reporting this. >> You told me basic test of cthon run fine but special test fail. > > Well yes but I didn't say the failure was incorrect. Connectathon is > a useful test, but it's not a protocol test. What makes it handy for > me is that it's easy to deploy and run, and it is not a synthetic > client, that is, it makes ordinary syscalls and exercices the local > in-kernel client code. > > The "special" tests in particular are about NFS client quirks, and > specifically quirks that were interesting in, like, 1999. They need > to be taken with a huge lump of salt, and an even larger lump today. > > It's ok, I'm not concerned that you added my Reported-by. But it needs > a lot more research before pushing this upstream. I started looking into this problem because you described it as if ksmbd was having problems even on TCP when running a special test. And the op_unk test, one of the special tests in cthon, passed against samba but failed against ksmbd, so I was trying to figure out the cause. The cthon test does not appear to be test for only NFS. Because these tests exclude NFS (and Posix) semantics depending on the WIN32 and CIFS configure. > > Tom. > >>> But more fundamentally, the Connectathon test is an NFS exerciser, and >>> specific to NFS (and Posix) semantics. Delete-on-last-close is not one >>> of them. >>> >>> Won't failing a new open break Windows semantics if it's enforced by >>> default? Normally Windows checks for FILE_SHARE_DELETE before deciding >>> this. Maybe other checks as well... >>> >>> Tom. >>> >>>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >>>> --- >>>> fs/smb/server/vfs_cache.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/fs/smb/server/vfs_cache.c b/fs/smb/server/vfs_cache.c >>>> index f41f8d6108ce..f2e2a7cc24a9 100644 >>>> --- a/fs/smb/server/vfs_cache.c >>>> +++ b/fs/smb/server/vfs_cache.c >>>> @@ -577,6 +577,11 @@ struct ksmbd_file *ksmbd_open_fd(struct ksmbd_work >>>> *work, struct file *filp) >>>> goto err_out; >>>> } >>>> >>>> + if (fp->f_ci->m_flags & S_DEL_ON_CLS) { >>>> + ret = -ENOENT; >>>> + goto err_out; >>>> + } >>>> + >>>> ret = __open_id(&work->sess->file_table, fp, >>>> OPEN_ID_TYPE_VOLATILE_ID); >>>> if (ret) { >>>> ksmbd_inode_put(fp->f_ci); >>> >> >
On Thu, 28 Sept 2023 at 00:44, Namjae Jeon <linkinjeon@kernel.org> wrote: > > Cthon test fail with the following error. > > check for proper open/unlink operation > nfsjunk files before unlink: > -rwxr-xr-x 1 root root 0 9월 25 11:03 ./nfs2y8Jm9 > ./nfs2y8Jm9 open; unlink ret = 0 > nfsjunk files after unlink: > -rwxr-xr-x 1 root root 0 9월 25 11:03 ./nfs2y8Jm9 > data compare ok > nfsjunk files after close: > ls: cannot access './nfs2y8Jm9': No such file or directory > special tests failed > > Cthon expect to second unlink failure when file is already unlinked. > ksmbd can not allow to open file if flags of ksmbd inode is set with > S_DEL_ON_CLS flags. > > Reported-by: Tom Talpey <tom@talpey.com> > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> > --- > fs/smb/server/vfs_cache.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/fs/smb/server/vfs_cache.c b/fs/smb/server/vfs_cache.c > index f41f8d6108ce..f2e2a7cc24a9 100644 > --- a/fs/smb/server/vfs_cache.c > +++ b/fs/smb/server/vfs_cache.c > @@ -577,6 +577,11 @@ struct ksmbd_file *ksmbd_open_fd(struct ksmbd_work *work, struct file *filp) > goto err_out; > } > > + if (fp->f_ci->m_flags & S_DEL_ON_CLS) { > + ret = -ENOENT; > + goto err_out; > + } > + Is enoent the right error here? I assume that the file will still show in a directory listing so maybe eacces would be better? > ret = __open_id(&work->sess->file_table, fp, OPEN_ID_TYPE_VOLATILE_ID); > if (ret) { > ksmbd_inode_put(fp->f_ci); > -- > 2.25.1 >
2023-09-29 10:13 GMT+09:00, ronnie sahlberg <ronniesahlberg@gmail.com>: > On Thu, 28 Sept 2023 at 00:44, Namjae Jeon <linkinjeon@kernel.org> wrote: >> >> Cthon test fail with the following error. >> >> check for proper open/unlink operation >> nfsjunk files before unlink: >> -rwxr-xr-x 1 root root 0 9월 25 11:03 ./nfs2y8Jm9 >> ./nfs2y8Jm9 open; unlink ret = 0 >> nfsjunk files after unlink: >> -rwxr-xr-x 1 root root 0 9월 25 11:03 ./nfs2y8Jm9 >> data compare ok >> nfsjunk files after close: >> ls: cannot access './nfs2y8Jm9': No such file or directory >> special tests failed >> >> Cthon expect to second unlink failure when file is already unlinked. >> ksmbd can not allow to open file if flags of ksmbd inode is set with >> S_DEL_ON_CLS flags. >> >> Reported-by: Tom Talpey <tom@talpey.com> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >> --- >> fs/smb/server/vfs_cache.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/fs/smb/server/vfs_cache.c b/fs/smb/server/vfs_cache.c >> index f41f8d6108ce..f2e2a7cc24a9 100644 >> --- a/fs/smb/server/vfs_cache.c >> +++ b/fs/smb/server/vfs_cache.c >> @@ -577,6 +577,11 @@ struct ksmbd_file *ksmbd_open_fd(struct ksmbd_work >> *work, struct file *filp) >> goto err_out; >> } >> >> + if (fp->f_ci->m_flags & S_DEL_ON_CLS) { >> + ret = -ENOENT; >> + goto err_out; >> + } >> + > > Is enoent the right error here? I assume that the file will still show No, It should be STATUS_DELETE_PENDING error. > in a directory listing so maybe eacces would be better? Right. but it should not be STATUS_ACCESS_DENIED error. So I will change the patch to return STATUS_DELETE_PENDING error to client on v2. Thanks for your review! > > >> ret = __open_id(&work->sess->file_table, fp, >> OPEN_ID_TYPE_VOLATILE_ID); >> if (ret) { >> ksmbd_inode_put(fp->f_ci); >> -- >> 2.25.1 >> >
diff --git a/fs/smb/server/vfs_cache.c b/fs/smb/server/vfs_cache.c index f41f8d6108ce..f2e2a7cc24a9 100644 --- a/fs/smb/server/vfs_cache.c +++ b/fs/smb/server/vfs_cache.c @@ -577,6 +577,11 @@ struct ksmbd_file *ksmbd_open_fd(struct ksmbd_work *work, struct file *filp) goto err_out; } + if (fp->f_ci->m_flags & S_DEL_ON_CLS) { + ret = -ENOENT; + goto err_out; + } + ret = __open_id(&work->sess->file_table, fp, OPEN_ID_TYPE_VOLATILE_ID); if (ret) { ksmbd_inode_put(fp->f_ci);
Cthon test fail with the following error. check for proper open/unlink operation nfsjunk files before unlink: -rwxr-xr-x 1 root root 0 9월 25 11:03 ./nfs2y8Jm9 ./nfs2y8Jm9 open; unlink ret = 0 nfsjunk files after unlink: -rwxr-xr-x 1 root root 0 9월 25 11:03 ./nfs2y8Jm9 data compare ok nfsjunk files after close: ls: cannot access './nfs2y8Jm9': No such file or directory special tests failed Cthon expect to second unlink failure when file is already unlinked. ksmbd can not allow to open file if flags of ksmbd inode is set with S_DEL_ON_CLS flags. Reported-by: Tom Talpey <tom@talpey.com> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> --- fs/smb/server/vfs_cache.c | 5 +++++ 1 file changed, 5 insertions(+)