Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <ef49e240-fc8f-9eb4-af98-26bfd39104aa@huawei.com>
In-Reply-To: <ef49e240-fc8f-9eb4-af98-26bfd39104aa@huawei.com>
From:   ronnie sahlberg <ronniesahlberg@gmail.com>
Date:   Wed, 18 Mar 2020 14:47:32 +1000
Message-ID: <CAN05THQYxPcsgiHTqMcsTgB6ZDYaBMamu-sOe428H7EwSRU2HQ@mail.gmail.com>
Subject: Re: [PATCH] CIFS: Fix bug which the return value by asynchronous read
 is error
To:     Yilu Lin <linyilu@huawei.com>
Cc:     linux-cifs <linux-cifs@vger.kernel.org>,
        Steve French <sfrench@samba.org>,
        "samba-technical@lists.samba.org" <samba-technical@lists.samba.org>,
        LKML <linux-kernel@vger.kernel.org>, alex.chen@huawei.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@huawei.com> 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@huawei.com>
> ---
>  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
>
>