[PATCH] ksmbd: fix incorrect handling of iterate_dir

[PATCH] ksmbd: fix incorrect handling of iterate_dir

[Hyunchul Lee]

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(-)

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;
-- 
2.17.1

Re: [PATCH] ksmbd: fix incorrect handling of iterate_dir

[Namjae Jeon]

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
>
>

Re: [PATCH] ksmbd: fix incorrect handling of iterate_dir

[Hyunchul Lee]

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
> >
> >



-- 
Thanks,
Hyunchul

Re: [PATCH] ksmbd: fix incorrect handling of iterate_dir

[Namjae Jeon]

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
>

Re: [PATCH] ksmbd: fix incorrect handling of iterate_dir

[Hyunchul Lee]

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
> >



-- 
Thanks,
Hyunchul

Re: [PATCH] ksmbd: fix incorrect handling of iterate_dir

[Namjae Jeon]

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
>

Re: [PATCH] ksmbd: fix incorrect handling of iterate_dir

[Hyunchul Lee]

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
> >



-- 
Thanks,
Hyunchul

Re: [PATCH] ksmbd: fix incorrect handling of iterate_dir

[Namjae Jeon]

>> >> >> > 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.

Re: [PATCH] ksmbd: fix incorrect handling of iterate_dir

[Hyunchul Lee]

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?

-- 
Thanks,
Hyunchul

Re: [PATCH] ksmbd: fix incorrect handling of iterate_dir

[Namjae Jeon]

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
>

Re: [PATCH] ksmbd: fix incorrect handling of iterate_dir

[Hyunchul Lee]

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

Re: [PATCH] ksmbd: fix incorrect handling of iterate_dir

[Namjae Jeon]

>
> 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
>