| Message ID | 20220819043557.26745-1-hyc.lee@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ksmbd: fix incorrect handling of iterate_dir | expand |
2022-08-19 13:35 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: > if iterate_dir() returns non-negative value, > caller has to treat it as normal and > check there is any error while populating > dentry information. ksmbd doesn't have to > do anything because ksmbd already > checks too small OutputBufferLength to > store one file information. > > And because ctx->pos is set to file->f_pos > when iterative_dir is called, remove > restart_ctx(). Shouldn't we get rid of the useless restart_ctx() ? > > This patch fixes some failure of > SMB2_QUERY_DIRECTORY, which happens when > ntfs3 is local filesystem. > > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com> > --- > fs/ksmbd/smb2pdu.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > index 53c91ab02be2..6716c4e3c16d 100644 > --- a/fs/ksmbd/smb2pdu.c > +++ b/fs/ksmbd/smb2pdu.c > @@ -3970,11 +3970,9 @@ int smb2_query_dir(struct ksmbd_work *work) > */ > if (!d_info.out_buf_len && !d_info.num_entry) > goto no_buf_len; > - if (rc == 0) > - restart_ctx(&dir_fp->readdir_data.ctx); > - if (rc == -ENOSPC) > + if (rc > 0 || rc == -ENOSPC) Do you know why -ENOSPC error is ignored ? Thanks. > rc = 0; > - if (rc) > + else if (rc) > goto err_out; > > d_info.wptr = d_info.rptr; > -- > 2.17.1 > >
2022년 8월 22일 (월) 오전 9:51, Namjae Jeon <linkinjeon@kernel.org>님이 작성: > > 2022-08-19 13:35 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: > > if iterate_dir() returns non-negative value, > > caller has to treat it as normal and > > check there is any error while populating > > dentry information. ksmbd doesn't have to > > do anything because ksmbd already > > checks too small OutputBufferLength to > > store one file information. > > > > And because ctx->pos is set to file->f_pos > > when iterative_dir is called, remove > > restart_ctx(). > Shouldn't we get rid of the useless restart_ctx() ? > There is one place to call this function. We can replace that with ctx->pos = 0 and remove this function. > > > > This patch fixes some failure of > > SMB2_QUERY_DIRECTORY, which happens when > > ntfs3 is local filesystem. > > > > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com> > > --- > > fs/ksmbd/smb2pdu.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > > index 53c91ab02be2..6716c4e3c16d 100644 > > --- a/fs/ksmbd/smb2pdu.c > > +++ b/fs/ksmbd/smb2pdu.c > > @@ -3970,11 +3970,9 @@ int smb2_query_dir(struct ksmbd_work *work) > > */ > > if (!d_info.out_buf_len && !d_info.num_entry) > > goto no_buf_len; > > - if (rc == 0) > > - restart_ctx(&dir_fp->readdir_data.ctx); > > - if (rc == -ENOSPC) > > + if (rc > 0 || rc == -ENOSPC) > Do you know why -ENOSPC error is ignored ? > I don't know why and can't find the commit history for this. > Thanks. > > rc = 0; > > - if (rc) > > + else if (rc) > > goto err_out; > > > > d_info.wptr = d_info.rptr; > > -- > > 2.17.1 > > > >
2022-08-22 11:14 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: > 2022년 8월 22일 (월) 오전 9:51, Namjae Jeon <linkinjeon@kernel.org>님이 작성: >> >> 2022-08-19 13:35 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: >> > if iterate_dir() returns non-negative value, >> > caller has to treat it as normal and >> > check there is any error while populating >> > dentry information. ksmbd doesn't have to >> > do anything because ksmbd already >> > checks too small OutputBufferLength to >> > store one file information. >> > >> > And because ctx->pos is set to file->f_pos >> > when iterative_dir is called, remove >> > restart_ctx(). >> Shouldn't we get rid of the useless restart_ctx() ? >> > > There is one place to call this function. We can > replace that with ctx->pos = 0 and remove this function. Why should we do ctx->pos = 0 there ? > >> > >> > This patch fixes some failure of >> > SMB2_QUERY_DIRECTORY, which happens when >> > ntfs3 is local filesystem. >> > >> > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com> >> > --- >> > fs/ksmbd/smb2pdu.c | 6 ++---- >> > 1 file changed, 2 insertions(+), 4 deletions(-) >> > >> > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >> > index 53c91ab02be2..6716c4e3c16d 100644 >> > --- a/fs/ksmbd/smb2pdu.c >> > +++ b/fs/ksmbd/smb2pdu.c >> > @@ -3970,11 +3970,9 @@ int smb2_query_dir(struct ksmbd_work *work) >> > */ >> > if (!d_info.out_buf_len && !d_info.num_entry) >> > goto no_buf_len; >> > - if (rc == 0) >> > - restart_ctx(&dir_fp->readdir_data.ctx); >> > - if (rc == -ENOSPC) >> > + if (rc > 0 || rc == -ENOSPC) >> Do you know why -ENOSPC error is ignored ? >> > > I don't know why and can't find the commit history > for this. After checking if -ENOSPC error is returned, there is no need to leave it if it is not needed. > >> Thanks. >> > rc = 0; >> > - if (rc) >> > + else if (rc) >> > goto err_out; >> > >> > d_info.wptr = d_info.rptr; >> > -- >> > 2.17.1 >> > >> > > > > > -- > Thanks, > Hyunchul >
2022년 8월 22일 (월) 오전 11:47, Namjae Jeon <linkinjeon@kernel.org>님이 작성: > > 2022-08-22 11:14 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: > > 2022년 8월 22일 (월) 오전 9:51, Namjae Jeon <linkinjeon@kernel.org>님이 작성: > >> > >> 2022-08-19 13:35 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: > >> > if iterate_dir() returns non-negative value, > >> > caller has to treat it as normal and > >> > check there is any error while populating > >> > dentry information. ksmbd doesn't have to > >> > do anything because ksmbd already > >> > checks too small OutputBufferLength to > >> > store one file information. > >> > > >> > And because ctx->pos is set to file->f_pos > >> > when iterative_dir is called, remove > >> > restart_ctx(). > >> Shouldn't we get rid of the useless restart_ctx() ? > >> > > > > There is one place to call this function. We can > > replace that with ctx->pos = 0 and remove this function. > Why should we do ctx->pos = 0 there ? restart_ctx has to be deleted. I misunderstood it, > > > >> > > >> > This patch fixes some failure of > >> > SMB2_QUERY_DIRECTORY, which happens when > >> > ntfs3 is local filesystem. > >> > > >> > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com> > >> > --- > >> > fs/ksmbd/smb2pdu.c | 6 ++---- > >> > 1 file changed, 2 insertions(+), 4 deletions(-) > >> > > >> > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > >> > index 53c91ab02be2..6716c4e3c16d 100644 > >> > --- a/fs/ksmbd/smb2pdu.c > >> > +++ b/fs/ksmbd/smb2pdu.c > >> > @@ -3970,11 +3970,9 @@ int smb2_query_dir(struct ksmbd_work *work) > >> > */ > >> > if (!d_info.out_buf_len && !d_info.num_entry) > >> > goto no_buf_len; > >> > - if (rc == 0) > >> > - restart_ctx(&dir_fp->readdir_data.ctx); > >> > - if (rc == -ENOSPC) > >> > + if (rc > 0 || rc == -ENOSPC) > >> Do you know why -ENOSPC error is ignored ? > >> > > > > I don't know why and can't find the commit history > > for this. > After checking if -ENOSPC error is returned, there is no need to leave > it if it is not needed. In most cases, -ENOSPC is not returned. Because the value is set to the return value from filesystems' iterate or iterate_share, and most file systems don't allocate disk space for this operation. But we cannot guarantee this. So how about changing handling iterate_dir like gendents system call. Even if an error code is returned by iterate_dir, it treats as normal if several child files are iterated and the buffer is filled with information about those. > > > >> Thanks. > >> > rc = 0; > >> > - if (rc) > >> > + else if (rc) > >> > goto err_out; > >> > > >> > d_info.wptr = d_info.rptr; > >> > -- > >> > 2.17.1 > >> > > >> > > > > > > > > > -- > > Thanks, > > Hyunchul > >
2022-08-22 15:24 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: > 2022년 8월 22일 (월) 오전 11:47, Namjae Jeon <linkinjeon@kernel.org>님이 작성: >> >> 2022-08-22 11:14 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: >> > 2022년 8월 22일 (월) 오전 9:51, Namjae Jeon <linkinjeon@kernel.org>님이 작성: >> >> >> >> 2022-08-19 13:35 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: >> >> > if iterate_dir() returns non-negative value, >> >> > caller has to treat it as normal and >> >> > check there is any error while populating >> >> > dentry information. ksmbd doesn't have to >> >> > do anything because ksmbd already >> >> > checks too small OutputBufferLength to >> >> > store one file information. >> >> > >> >> > And because ctx->pos is set to file->f_pos >> >> > when iterative_dir is called, remove >> >> > restart_ctx(). >> >> Shouldn't we get rid of the useless restart_ctx() ? >> >> >> > >> > There is one place to call this function. We can >> > replace that with ctx->pos = 0 and remove this function. >> Why should we do ctx->pos = 0 there ? > > restart_ctx has to be deleted. I misunderstood it, > >> > >> >> > >> >> > This patch fixes some failure of >> >> > SMB2_QUERY_DIRECTORY, which happens when >> >> > ntfs3 is local filesystem. >> >> > >> >> > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com> >> >> > --- >> >> > fs/ksmbd/smb2pdu.c | 6 ++---- >> >> > 1 file changed, 2 insertions(+), 4 deletions(-) >> >> > >> >> > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >> >> > index 53c91ab02be2..6716c4e3c16d 100644 >> >> > --- a/fs/ksmbd/smb2pdu.c >> >> > +++ b/fs/ksmbd/smb2pdu.c >> >> > @@ -3970,11 +3970,9 @@ int smb2_query_dir(struct ksmbd_work *work) >> >> > */ >> >> > if (!d_info.out_buf_len && !d_info.num_entry) >> >> > goto no_buf_len; >> >> > - if (rc == 0) >> >> > - restart_ctx(&dir_fp->readdir_data.ctx); >> >> > - if (rc == -ENOSPC) >> >> > + if (rc > 0 || rc == -ENOSPC) >> >> Do you know why -ENOSPC error is ignored ? >> >> >> > >> > I don't know why and can't find the commit history >> > for this. >> After checking if -ENOSPC error is returned, there is no need to leave >> it if it is not needed. > > In most cases, -ENOSPC is not returned. Because the value > is set to the return value from filesystems' iterate or iterate_share, > and most file systems don't allocate disk space for this operation. > > But we cannot guarantee this. So how about changing handling iterate_dir > like gendents system call. Even if an error code is returned by > iterate_dir, > it treats as normal if several child files are iterated and the buffer > is filled with > information about those. Among the errors of the smb2 query directory in the specification, there is a file corruption error response type(STATUS_FILE_CORRUPT_ERROR). Can you check when smb server return that error response for smb2 query directory? > >> > >> >> Thanks. >> >> > rc = 0; >> >> > - if (rc) >> >> > + else if (rc) >> >> > goto err_out; >> >> > >> >> > d_info.wptr = d_info.rptr; >> >> > -- >> >> > 2.17.1 >> >> > >> >> > >> > >> > >> > >> > -- >> > Thanks, >> > Hyunchul >> > > > > > -- > Thanks, > Hyunchul >
2022년 8월 23일 (화) 오전 9:40, Namjae Jeon <linkinjeon@kernel.org>님이 작성: > > 2022-08-22 15:24 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: > > 2022년 8월 22일 (월) 오전 11:47, Namjae Jeon <linkinjeon@kernel.org>님이 작성: > >> > >> 2022-08-22 11:14 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: > >> > 2022년 8월 22일 (월) 오전 9:51, Namjae Jeon <linkinjeon@kernel.org>님이 작성: > >> >> > >> >> 2022-08-19 13:35 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: > >> >> > if iterate_dir() returns non-negative value, > >> >> > caller has to treat it as normal and > >> >> > check there is any error while populating > >> >> > dentry information. ksmbd doesn't have to > >> >> > do anything because ksmbd already > >> >> > checks too small OutputBufferLength to > >> >> > store one file information. > >> >> > > >> >> > And because ctx->pos is set to file->f_pos > >> >> > when iterative_dir is called, remove > >> >> > restart_ctx(). > >> >> Shouldn't we get rid of the useless restart_ctx() ? > >> >> > >> > > >> > There is one place to call this function. We can > >> > replace that with ctx->pos = 0 and remove this function. > >> Why should we do ctx->pos = 0 there ? > > > > restart_ctx has to be deleted. I misunderstood it, > > > >> > > >> >> > > >> >> > This patch fixes some failure of > >> >> > SMB2_QUERY_DIRECTORY, which happens when > >> >> > ntfs3 is local filesystem. > >> >> > > >> >> > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com> > >> >> > --- > >> >> > fs/ksmbd/smb2pdu.c | 6 ++---- > >> >> > 1 file changed, 2 insertions(+), 4 deletions(-) > >> >> > > >> >> > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > >> >> > index 53c91ab02be2..6716c4e3c16d 100644 > >> >> > --- a/fs/ksmbd/smb2pdu.c > >> >> > +++ b/fs/ksmbd/smb2pdu.c > >> >> > @@ -3970,11 +3970,9 @@ int smb2_query_dir(struct ksmbd_work *work) > >> >> > */ > >> >> > if (!d_info.out_buf_len && !d_info.num_entry) > >> >> > goto no_buf_len; > >> >> > - if (rc == 0) > >> >> > - restart_ctx(&dir_fp->readdir_data.ctx); > >> >> > - if (rc == -ENOSPC) > >> >> > + if (rc > 0 || rc == -ENOSPC) > >> >> Do you know why -ENOSPC error is ignored ? > >> >> > >> > > >> > I don't know why and can't find the commit history > >> > for this. > >> After checking if -ENOSPC error is returned, there is no need to leave > >> it if it is not needed. > > > > In most cases, -ENOSPC is not returned. Because the value > > is set to the return value from filesystems' iterate or iterate_share, > > and most file systems don't allocate disk space for this operation. > > > > But we cannot guarantee this. So how about changing handling iterate_dir > > like gendents system call. Even if an error code is returned by > > iterate_dir, > > it treats as normal if several child files are iterated and the buffer > > is filled with > > information about those. > Among the errors of the smb2 query directory in the specification, > there is a file corruption error response > type(STATUS_FILE_CORRUPT_ERROR). > Can you check when smb server return that error response for smb2 > query directory? > According to MS-REREF, it means "The file or directory is corrupt and unreadable. Run the chkdsk utility". And there is no function to return the error in Samba. > > > >> > > >> >> Thanks. > >> >> > rc = 0; > >> >> > - if (rc) > >> >> > + else if (rc) > >> >> > goto err_out; > >> >> > > >> >> > d_info.wptr = d_info.rptr; > >> >> > -- > >> >> > 2.17.1 > >> >> > > >> >> > > >> > > >> > > >> > > >> > -- > >> > Thanks, > >> > Hyunchul > >> > > > > > > > > > -- > > Thanks, > > Hyunchul > >
>> >> >> > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >> >> >> > index 53c91ab02be2..6716c4e3c16d 100644 >> >> >> > --- a/fs/ksmbd/smb2pdu.c >> >> >> > +++ b/fs/ksmbd/smb2pdu.c >> >> >> > @@ -3970,11 +3970,9 @@ int smb2_query_dir(struct ksmbd_work >> >> >> > *work) >> >> >> > */ >> >> >> > if (!d_info.out_buf_len && !d_info.num_entry) >> >> >> > goto no_buf_len; >> >> >> > - if (rc == 0) >> >> >> > - restart_ctx(&dir_fp->readdir_data.ctx); >> >> >> > - if (rc == -ENOSPC) >> >> >> > + if (rc > 0 || rc == -ENOSPC) >> >> >> Do you know why -ENOSPC error is ignored ? >> >> >> >> >> > >> >> > I don't know why and can't find the commit history >> >> > for this. >> >> After checking if -ENOSPC error is returned, there is no need to leave >> >> it if it is not needed. >> > >> > In most cases, -ENOSPC is not returned. Because the value >> > is set to the return value from filesystems' iterate or iterate_share, >> > and most file systems don't allocate disk space for this operation. >> > >> > But we cannot guarantee this. So how about changing handling >> > iterate_dir >> > like gendents system call. Even if an error code is returned by >> > iterate_dir, >> > it treats as normal if several child files are iterated and the buffer >> > is filled with >> > information about those. >> Among the errors of the smb2 query directory in the specification, >> there is a file corruption error response >> type(STATUS_FILE_CORRUPT_ERROR). >> Can you check when smb server return that error response for smb2 >> query directory? >> > > According to MS-REREF, it means "The file or directory is corrupt and > unreadable. > Run the chkdsk utility". And there is no function to return the error in > Samba. Is samba not able to know corruption errors using getdents syscall as you said? There is no reason to follow it. I think that ksmbd is able to return this error.
2022년 8월 23일 (화) 오전 11:45, Namjae Jeon <linkinjeon@kernel.org>님이 작성: > > >> >> >> > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > >> >> >> > index 53c91ab02be2..6716c4e3c16d 100644 > >> >> >> > --- a/fs/ksmbd/smb2pdu.c > >> >> >> > +++ b/fs/ksmbd/smb2pdu.c > >> >> >> > @@ -3970,11 +3970,9 @@ int smb2_query_dir(struct ksmbd_work > >> >> >> > *work) > >> >> >> > */ > >> >> >> > if (!d_info.out_buf_len && !d_info.num_entry) > >> >> >> > goto no_buf_len; > >> >> >> > - if (rc == 0) > >> >> >> > - restart_ctx(&dir_fp->readdir_data.ctx); > >> >> >> > - if (rc == -ENOSPC) > >> >> >> > + if (rc > 0 || rc == -ENOSPC) > >> >> >> Do you know why -ENOSPC error is ignored ? > >> >> >> > >> >> > > >> >> > I don't know why and can't find the commit history > >> >> > for this. > >> >> After checking if -ENOSPC error is returned, there is no need to leave > >> >> it if it is not needed. > >> > > >> > In most cases, -ENOSPC is not returned. Because the value > >> > is set to the return value from filesystems' iterate or iterate_share, > >> > and most file systems don't allocate disk space for this operation. > >> > > >> > But we cannot guarantee this. So how about changing handling > >> > iterate_dir > >> > like gendents system call. Even if an error code is returned by > >> > iterate_dir, > >> > it treats as normal if several child files are iterated and the buffer > >> > is filled with > >> > information about those. > >> Among the errors of the smb2 query directory in the specification, > >> there is a file corruption error response > >> type(STATUS_FILE_CORRUPT_ERROR). > >> Can you check when smb server return that error response for smb2 > >> query directory? > >> > > > > According to MS-REREF, it means "The file or directory is corrupt and > > unreadable. > > Run the chkdsk utility". And there is no function to return the error in > > Samba. > Is samba not able to know corruption errors using getdents syscall as you said? If iterate_dir returns an error and there are files iterated, getdents syscall will succeed. But if a client requests SMB2_QUERY_DIR again due to STATUS_NO_MORE_FILES absence of the last response, iterate_dir returns an error again and there are no files iterated, getdents syscall will fail. Samba can send a response with an error. > There is no reason to follow it. I think that ksmbd is able to return > this error. Can we determine we should return a response with STATUS_FILE_CORRUPT_ERROR if iterate_dir returns -EIO?
2022-08-23 17:26 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: > 2022년 8월 23일 (화) 오전 11:45, Namjae Jeon <linkinjeon@kernel.org>님이 작성: >> >> >> >> >> > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >> >> >> >> > index 53c91ab02be2..6716c4e3c16d 100644 >> >> >> >> > --- a/fs/ksmbd/smb2pdu.c >> >> >> >> > +++ b/fs/ksmbd/smb2pdu.c >> >> >> >> > @@ -3970,11 +3970,9 @@ int smb2_query_dir(struct ksmbd_work >> >> >> >> > *work) >> >> >> >> > */ >> >> >> >> > if (!d_info.out_buf_len && !d_info.num_entry) >> >> >> >> > goto no_buf_len; >> >> >> >> > - if (rc == 0) >> >> >> >> > - restart_ctx(&dir_fp->readdir_data.ctx); >> >> >> >> > - if (rc == -ENOSPC) >> >> >> >> > + if (rc > 0 || rc == -ENOSPC) >> >> >> >> Do you know why -ENOSPC error is ignored ? >> >> >> >> >> >> >> > >> >> >> > I don't know why and can't find the commit history >> >> >> > for this. >> >> >> After checking if -ENOSPC error is returned, there is no need to >> >> >> leave >> >> >> it if it is not needed. >> >> > >> >> > In most cases, -ENOSPC is not returned. Because the value >> >> > is set to the return value from filesystems' iterate or >> >> > iterate_share, >> >> > and most file systems don't allocate disk space for this operation. >> >> > >> >> > But we cannot guarantee this. So how about changing handling >> >> > iterate_dir >> >> > like gendents system call. Even if an error code is returned by >> >> > iterate_dir, >> >> > it treats as normal if several child files are iterated and the >> >> > buffer >> >> > is filled with >> >> > information about those. >> >> Among the errors of the smb2 query directory in the specification, >> >> there is a file corruption error response >> >> type(STATUS_FILE_CORRUPT_ERROR). >> >> Can you check when smb server return that error response for smb2 >> >> query directory? >> >> >> > >> > According to MS-REREF, it means "The file or directory is corrupt and >> > unreadable. >> > Run the chkdsk utility". And there is no function to return the error >> > in >> > Samba. >> Is samba not able to know corruption errors using getdents syscall as you >> said? > > If iterate_dir returns an error and there are files iterated, getdents > syscall will succeed. > But if a client requests SMB2_QUERY_DIR again due to STATUS_NO_MORE_FILES > absence of the last response, iterate_dir returns an error again and > there are no > files iterated, getdents syscall will fail. Samba can send a response > with an error. Sorry, can't understand. What error ? > >> There is no reason to follow it. I think that ksmbd is able to return >> this error. > > Can we determine we should return a response with STATUS_FILE_CORRUPT_ERROR > if iterate_dir returns -EIO? STATUS_FILE_CORRUPT_ERROR is still not cleared. want to know how windows server handle it. > > -- > Thanks, > Hyunchul >
2022년 8월 24일 (수) 오전 9:10, Namjae Jeon <linkinjeon@kernel.org>님이 작성: > > 2022-08-23 17:26 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>: > > 2022년 8월 23일 (화) 오전 11:45, Namjae Jeon <linkinjeon@kernel.org>님이 작성: > >> > >> >> >> >> > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > >> >> >> >> > index 53c91ab02be2..6716c4e3c16d 100644 > >> >> >> >> > --- a/fs/ksmbd/smb2pdu.c > >> >> >> >> > +++ b/fs/ksmbd/smb2pdu.c > >> >> >> >> > @@ -3970,11 +3970,9 @@ int smb2_query_dir(struct ksmbd_work > >> >> >> >> > *work) > >> >> >> >> > */ > >> >> >> >> > if (!d_info.out_buf_len && !d_info.num_entry) > >> >> >> >> > goto no_buf_len; > >> >> >> >> > - if (rc == 0) > >> >> >> >> > - restart_ctx(&dir_fp->readdir_data.ctx); > >> >> >> >> > - if (rc == -ENOSPC) > >> >> >> >> > + if (rc > 0 || rc == -ENOSPC) > >> >> >> >> Do you know why -ENOSPC error is ignored ? > >> >> >> >> > >> >> >> > > >> >> >> > I don't know why and can't find the commit history > >> >> >> > for this. > >> >> >> After checking if -ENOSPC error is returned, there is no need to > >> >> >> leave > >> >> >> it if it is not needed. > >> >> > > >> >> > In most cases, -ENOSPC is not returned. Because the value > >> >> > is set to the return value from filesystems' iterate or > >> >> > iterate_share, > >> >> > and most file systems don't allocate disk space for this operation. > >> >> > > >> >> > But we cannot guarantee this. So how about changing handling > >> >> > iterate_dir > >> >> > like gendents system call. Even if an error code is returned by > >> >> > iterate_dir, > >> >> > it treats as normal if several child files are iterated and the > >> >> > buffer > >> >> > is filled with > >> >> > information about those. > >> >> Among the errors of the smb2 query directory in the specification, > >> >> there is a file corruption error response > >> >> type(STATUS_FILE_CORRUPT_ERROR). > >> >> Can you check when smb server return that error response for smb2 > >> >> query directory? > >> >> > >> > > >> > According to MS-REREF, it means "The file or directory is corrupt and > >> > unreadable. > >> > Run the chkdsk utility". And there is no function to return the error > >> > in > >> > Samba. > >> Is samba not able to know corruption errors using getdents syscall as you > >> said? > > > > If iterate_dir returns an error and there are files iterated, getdents > > syscall will succeed. > > But if a client requests SMB2_QUERY_DIR again due to STATUS_NO_MORE_FILES > > absence of the last response, iterate_dir returns an error again and > > there are no > > files iterated, getdents syscall will fail. Samba can send a response > > with an error. > Sorry, can't understand. What error ? When I set the last entry of directory FAT chain to be invalid value in exFAT, the last getdents returns -EIO, but Samba sends responses without an error > > > >> There is no reason to follow it. I think that ksmbd is able to return > >> this error. > > > > Can we determine we should return a response with STATUS_FILE_CORRUPT_ERROR > > if iterate_dir returns -EIO? > STATUS_FILE_CORRUPT_ERROR is still not cleared. want to know how > windows server handle it. In the above example, Windows server sends an empty SMB2_QUERY_DIRECTORY response with STATUS_FILE_CORRUPT_ERROR. And when executing "dir <directory>" in Windows terminal for the local exFAT filesystem, there are no files iterated and DirIOError is generated. > > > > -- > > Thanks, > > Hyunchul > > -- Thanks, Hyunchul
> > When I set the last entry of directory FAT chain to be invalid value in > exFAT, > the last getdents returns -EIO, but Samba sends responses without an error There seems to be an attempt to improve this in Samba as well. https://bugzilla.samba.org/show_bug.cgi?id=13718 > >> > >> >> There is no reason to follow it. I think that ksmbd is able to return >> >> this error. >> > >> > Can we determine we should return a response with >> > STATUS_FILE_CORRUPT_ERROR >> > if iterate_dir returns -EIO? >> STATUS_FILE_CORRUPT_ERROR is still not cleared. want to know how >> windows server handle it. > > In the above example, Windows server sends an empty SMB2_QUERY_DIRECTORY > response with STATUS_FILE_CORRUPT_ERROR. > > And when executing "dir <directory>" in Windows terminal for the local > exFAT filesystem, > there are no files iterated and DirIOError is generated. This is the behavior I was expecting. ksmbd can handle this as well. Thanks. > > >> > >> > -- >> > Thanks, >> > Hyunchul >> > > > > > -- > Thanks, > Hyunchul >
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c index 53c91ab02be2..6716c4e3c16d 100644 --- a/fs/ksmbd/smb2pdu.c +++ b/fs/ksmbd/smb2pdu.c @@ -3970,11 +3970,9 @@ int smb2_query_dir(struct ksmbd_work *work) */ if (!d_info.out_buf_len && !d_info.num_entry) goto no_buf_len; - if (rc == 0) - restart_ctx(&dir_fp->readdir_data.ctx); - if (rc == -ENOSPC) + if (rc > 0 || rc == -ENOSPC) rc = 0; - if (rc) + else if (rc) goto err_out; d_info.wptr = d_info.rptr;
if iterate_dir() returns non-negative value, caller has to treat it as normal and check there is any error while populating dentry information. ksmbd doesn't have to do anything because ksmbd already checks too small OutputBufferLength to store one file information. And because ctx->pos is set to file->f_pos when iterative_dir is called, remove restart_ctx(). This patch fixes some failure of SMB2_QUERY_DIRECTORY, which happens when ntfs3 is local filesystem. Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com> --- fs/ksmbd/smb2pdu.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)