Re: [RFC PATCH] sys_read: add a compat_sys_read for 64bit system

Hi,

On 2016/6/8 9:33, Weidong Wang wrote:

Test 32 progress and 64 progress on the 64bit system with
this progress:

int main(int argc, char **argv)
{
         int fd = 0;
         int i, ret = 0;
         char buf[512];
         unsigned long count = -1;

         fd = open("/tmp", O_RDONLY);
         if (fd < -1) {
                 printf("Pls check the directory is exist?\n");
                 return -1;
         }
         errno = 0;
         ret = read(fd, NULL, count);
         printf("Ret is %d errno %d\n", ret, errno);
         close(fd);

         return 0;
}

we get the different errno. The 64 progress we get errno is -14 while
the 32 progress is -21.

The reason is that, the user progress would use a 32bit count, while
the sys_read size_t in kernel is 64bit.  When the uesrspace count is
-1(0xffffffff), it goes to the sys_read, it would be change to a positive
number.

So I think we should add a compat_sys_read for the read syscall. I test it
on x86 or arm64 platform. The patch works well.

As weidong said, we tested on x86, x86_64, aarch64 ilp32, aarch64 lp64.
We do not familiar with other architecture, cc linux-api, hope could get more
input.

Regards

Bamvor

32 more quoted lines

On June 7, 2016 7:14:41 PM PDT, "Zhangjian (Bamvor)" [off-list ref] wrote:

Hi,

On 2016/6/8 9:33, Weidong Wang wrote:

quoted

Test 32 progress and 64 progress on the 64bit system with
this progress:

int main(int argc, char **argv)
{
         int fd = 0;
         int i, ret = 0;
         char buf[512];
         unsigned long count = -1;

         fd = open("/tmp", O_RDONLY);
         if (fd < -1) {
                 printf("Pls check the directory is exist?\n");
                 return -1;
         }
         errno = 0;
         ret = read(fd, NULL, count);
         printf("Ret is %d errno %d\n", ret, errno);
         close(fd);

         return 0;
}

we get the different errno. The 64 progress we get errno is -14 while
the 32 progress is -21.

The reason is that, the user progress would use a 32bit count, while
the sys_read size_t in kernel is 64bit.  When the uesrspace count is
-1(0xffffffff), it goes to the sys_read, it would be change to a

positive

quoted

number.

So I think we should add a compat_sys_read for the read syscall. I

test it

quoted

on x86 or arm64 platform. The patch works well.

As weidong said, we tested on x86, x86_64, aarch64 ilp32, aarch64 lp64.
We do not familiar with other architecture, cc linux-api, hope could

83 more quoted lines

Does this cause any actual problems?  Also, it seems extremely unlikely read() would be the only system call so affected.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Hi, Peter

On 2016/6/8 15:33, H. Peter Anvin wrote:

On June 7, 2016 7:14:41 PM PDT, "Zhangjian (Bamvor)" [off-list ref] wrote:

quoted

Hi,

On 2016/6/8 9:33, Weidong Wang wrote:

quoted

Test 32 progress and 64 progress on the 64bit system with
this progress:

int main(int argc, char **argv)
{
          int fd = 0;
          int i, ret = 0;
          char buf[512];
          unsigned long count = -1;

          fd = open("/tmp", O_RDONLY);
          if (fd < -1) {
                  printf("Pls check the directory is exist?\n");
                  return -1;
          }
          errno = 0;
          ret = read(fd, NULL, count);
          printf("Ret is %d errno %d\n", ret, errno);
          close(fd);

          return 0;
}

we get the different errno. The 64 progress we get errno is -14 while
the 32 progress is -21.

The reason is that, the user progress would use a 32bit count, while
the sys_read size_t in kernel is 64bit.  When the uesrspace count is
-1(0xffffffff), it goes to the sys_read, it would be change to a

positive

quoted

number.

So I think we should add a compat_sys_read for the read syscall. I

test it

quoted

on x86 or arm64 platform. The patch works well.

As weidong said, we tested on x86, x86_64, aarch64 ilp32, aarch64 lp64.

86 more quoted lines

It do not cause any actual problems. And write may be affected too.
Without this patch, the errno is different when 32 application migrate
from 32bit kernel to 64bit kernel.

Regards

Bamvor

On Tue, Jun 7, 2016 at 7:14 PM, Zhangjian (Bamvor)
[off-list ref] wrote:

Hi,

On 2016/6/8 9:33, Weidong Wang wrote:

quoted

Test 32 progress and 64 progress on the 64bit system with
this progress:

int main(int argc, char **argv)
{
         int fd = 0;
         int i, ret = 0;
         char buf[512];
         unsigned long count = -1;

         fd = open("/tmp", O_RDONLY);
         if (fd < -1) {
                 printf("Pls check the directory is exist?\n");
                 return -1;
         }
         errno = 0;
         ret = read(fd, NULL, count);
         printf("Ret is %d errno %d\n", ret, errno);
         close(fd);

         return 0;
}

we get the different errno. The 64 progress we get errno is -14 while
the 32 progress is -21.

On 64-bit, you get -14 == -EFAULT.  Seems reasonable: you passed a bad pointer.

On 32-bit, you get -21 == -EISDIR.  Also seems reasonable: fd is a directory.

quoted

The reason is that, the user progress would use a 32bit count, while
the sys_read size_t in kernel is 64bit.  When the uesrspace count is
-1(0xffffffff), it goes to the sys_read, it would be change to a positive
number.

That parameter is size_t, which is unsigned.  It's a positive number
in both cases.

I don't think there's a bug here.

On 2016/6/10 1:08, Andy Lutomirski wrote:

On Tue, Jun 7, 2016 at 7:14 PM, Zhangjian (Bamvor)
[off-list ref] wrote:

quoted

Hi,

On 2016/6/8 9:33, Weidong Wang wrote:

quoted

Test 32 progress and 64 progress on the 64bit system with
this progress:

int main(int argc, char **argv)
{
         int fd = 0;
         int i, ret = 0;
         char buf[512];
         unsigned long count = -1;

         fd = open("/tmp", O_RDONLY);
         if (fd < -1) {
                 printf("Pls check the directory is exist?\n");
                 return -1;
         }
         errno = 0;
         ret = read(fd, NULL, count);
         printf("Ret is %d errno %d\n", ret, errno);
         close(fd);

         return 0;
}

we get the different errno. The 64 progress we get errno is -14 while
the 32 progress is -21.

On 64-bit, you get -14 == -EFAULT.  Seems reasonable: you passed a bad pointer.

On 32-bit, you get -21 == -EISDIR.  Also seems reasonable: fd is a directory.

quoted

quoted

The reason is that, the user progress would use a 32bit count, while
the sys_read size_t in kernel is 64bit.  When the uesrspace count is
-1(0xffffffff), it goes to the sys_read, it would be change to a positive

7 more quoted lines

Yep.
In the progress open the '/tmp' is a directory. If we do open a file '/tmp/files' (exist file),
the result would be different on x86-64bit machine.

On 64-bit, we get -14 == -EFAULT.
On 32-bit, we get the length of the file, the errno is 0.

Regards,
Weidong

.