CIFS: Fix bug which the return value by asynchronous read is error

Commit Message

Yilu Lin March 18, 2020, 3:59 a.m. UTC

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 <[e-mail]>
---
 fs/cifs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

ronnie sahlberg March 18, 2020, 4:47 a.m. UTC | #1

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 <[e-mail]> 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 <[e-mail]>
> ---
>  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
>
>

Yilu Lin March 19, 2020, 2:50 a.m. UTC | #2

Hi,
The bug is trigerred by the following test program.
First, run the command to create one file on the share directory。COMMAND：dd if=/dev/zero of=/home/temp/cifs/sys.zero bs=512 count=1000 oflag=direct
And then run the c program by the command: ./pread /home/temp/cifs/sys.zero
The program will return "pread -22 bytes".However, the expected result is "pread -1 bytes".

C program code is showed below:

    #include<stdio.h>
    #include<stdlib.h>
    #include<unistd.h>
    #include <sys/types.h>
    #include <sys/stat.h>
    #define __USE_GNU 1
    #include <fcntl.h>
    int main(int argc, char *argv[])
    {
            int fd;
            ssize_t len;
            int ret;
            int size = 512;
            char *file=argv[1];

            unsigned char *buf2;
            ret = posix_memalign((void **)&buf2, 512, size);
            if (ret) {
                    printf("malloc err!\n");
                    return 0;
            }

            fd = open(file, O_RDWR | O_DIRECT);
            if(fd == -1) {
                    printf("open error!\n");
                    free(buf2);
                    return 0;
            }

            len = pread(fd, buf2, 504, 511992);
            printf("pread %d bytes\n", len);
            free(buf2);
            close(fd);
            return 0;
    }

regards
Yilu Lin

在 2020/3/18 12:47, 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 <[e-mail]> 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 <[e-mail]>
>> ---
>>  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
>>
>>
> 
> .
>

Patch

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