Re: [PATCH] CIFS: Fix bug which the return value by asynchronous read is error

[ronnie sahlberg]

Hi Yilu,

I think your reasoning makes sense.
Do you have a small reproducer for this? A small C program that triggers this?

I am asking because if you do we would like to add it to our buildbot
to make  sure we don't get regressions.


regards
ronnie sahlberg

On Wed, Mar 18, 2020 at 1:59 PM Yilu Lin <linyilu@xxxxxxxxxx> wrote:
>
> This patch is used to fix the bug in collect_uncached_read_data()
> that rc is automatically converted from a signed number to an
> unsigned number when the CIFS asynchronous read fails.
> It will cause ctx->rc is error.
>
> Example:
> Share a directory and create a file on the Windows OS.
> Mount the directory to the Linux OS using CIFS.
> On the CIFS client of the Linux OS, invoke the pread interface to
> deliver the read request.
>
> The size of the read length plus offset of the read request is greater
> than the maximum file size.
>
> In this case, the CIFS server on the Windows OS returns a failure
> message (for example, the return value of
> smb2.nt_status is STATUS_INVALID_PARAMETER).
>
> After receiving the response message, the CIFS client parses
> smb2.nt_status to STATUS_INVALID_PARAMETER
> and converts it to the Linux error code (rdata->result=-22).
>
> Then the CIFS client invokes the collect_uncached_read_data function to
> assign the value of rdata->result to rc, that is, rc=rdata->result=-22.
>
> The type of the ctx->total_len variable is unsigned integer,
> the type of the rc variable is integer, and the type of
> the ctx->rc variable is ssize_t.
>
> Therefore, during the ternary operation, the value of rc is
> automatically converted to an unsigned number. The final result is
> ctx->rc=4294967274. However, the expected result is ctx->rc=-22.
>
> Signed-off-by: Yilu Lin <linyilu@xxxxxxxxxx>
> ---
>  fs/cifs/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 022029a5d..ff4ac244c 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -3323,7 +3323,7 @@ again:
>         if (rc == -ENODATA)
>                 rc = 0;
>
> -       ctx->rc = (rc == 0) ? ctx->total_len : rc;
> +       ctx->rc = (rc == 0) ? (ssize_t)ctx->total_len : rc;
>
>         mutex_unlock(&ctx->aio_mutex);
>
> --
> 2.19.1
>
>