diff mbox

linux-user/syscall.c: Notice about lock bitmask translation for fcntl

Message ID 5673ACFA.6060900@emindsoft.com.cn
State New
Headers show

Commit Message

Chen Gang Dec. 18, 2015, 6:51 a.m. UTC 
I found this issue during my working time, it is about sw_64 (almost the
same as alpha) host running i386 wine programs.

I also found another issue, but I am not quite sure whether it is worth
enough for our upstream: The related fix patch is below, which will let
the initialization slower, but for most archs, they have no this issue.

    linux-user/mmap.c: Always zero MAP_ANONYMOUS memory in target_mmap()
    
    In some architectures, they have no policy to zero MAP_ANONYMOUS memory,
    which will cause issue for qemu target_mmap.
    
    Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>



Thanks.

On 2015年12月18日 14:26, Chen Gang wrote:
> 
> For fcntl, it always needs to notice about it, just like do_fcntl() has
> done, or it will cause issue (e.g. alpha host run i386 guest).
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  linux-user/syscall.c |   18 ++++++++++++------
>  1 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 0f8adeb..1a60e6f 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -9007,7 +9007,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>              if (((CPUARMState *)cpu_env)->eabi) {
>                  if (!lock_user_struct(VERIFY_READ, target_efl, arg3, 1)) 
>                      goto efault;
> -                fl.l_type = tswap16(target_efl->l_type);
> +                fl.l_type = target_to_host_bitmask(tswap16(target_fl->l_type),
> +                                                   flock_tbl);
>                  fl.l_whence = tswap16(target_efl->l_whence);
>                  fl.l_start = tswap64(target_efl->l_start);
>                  fl.l_len = tswap64(target_efl->l_len);
> @@ -9018,7 +9019,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>              {
>                  if (!lock_user_struct(VERIFY_READ, target_fl, arg3, 1)) 
>                      goto efault;
> -                fl.l_type = tswap16(target_fl->l_type);
> +                fl.l_type = target_to_host_bitmask(tswap16(target_fl->l_type),
> +                                                   flock_tbl);
>                  fl.l_whence = tswap16(target_fl->l_whence);
>                  fl.l_start = tswap64(target_fl->l_start);
>                  fl.l_len = tswap64(target_fl->l_len);
> @@ -9031,7 +9033,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                  if (((CPUARMState *)cpu_env)->eabi) {
>                      if (!lock_user_struct(VERIFY_WRITE, target_efl, arg3, 0)) 
>                          goto efault;
> -                    target_efl->l_type = tswap16(fl.l_type);
> +                    target_efl->l_type = host_to_target_bitmask(
> +                                                 tswap16(fl.l_type), flock_tbl);
>                      target_efl->l_whence = tswap16(fl.l_whence);
>                      target_efl->l_start = tswap64(fl.l_start);
>                      target_efl->l_len = tswap64(fl.l_len);
> @@ -9042,7 +9045,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                  {
>                      if (!lock_user_struct(VERIFY_WRITE, target_fl, arg3, 0)) 
>                          goto efault;
> -                    target_fl->l_type = tswap16(fl.l_type);
> +                    target_fl->l_type = host_to_target_bitmask(
> +                                                 tswap16(fl.l_type), flock_tbl);
>                      target_fl->l_whence = tswap16(fl.l_whence);
>                      target_fl->l_start = tswap64(fl.l_start);
>                      target_fl->l_len = tswap64(fl.l_len);
> @@ -9058,7 +9062,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>              if (((CPUARMState *)cpu_env)->eabi) {
>                  if (!lock_user_struct(VERIFY_READ, target_efl, arg3, 1)) 
>                      goto efault;
> -                fl.l_type = tswap16(target_efl->l_type);
> +                fl.l_type = target_to_host_bitmask(tswap16(target_fl->l_type),
> +                                                   flock_tbl);
>                  fl.l_whence = tswap16(target_efl->l_whence);
>                  fl.l_start = tswap64(target_efl->l_start);
>                  fl.l_len = tswap64(target_efl->l_len);
> @@ -9069,7 +9074,8 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>              {
>                  if (!lock_user_struct(VERIFY_READ, target_fl, arg3, 1)) 
>                      goto efault;
> -                fl.l_type = tswap16(target_fl->l_type);
> +                fl.l_type = target_to_host_bitmask(tswap16(target_fl->l_type),
> +                                                   flock_tbl);
>                  fl.l_whence = tswap16(target_fl->l_whence);
>                  fl.l_start = tswap64(target_fl->l_start);
>                  fl.l_len = tswap64(target_fl->l_len);
>

Comments

Laurent Vivier Dec. 18, 2015, 9:41 a.m. UTC | #1 
Le 18/12/2015 07:51, Chen Gang a écrit :
> 
> I found this issue during my working time, it is about sw_64 (almost the
> same as alpha) host running i386 wine programs.
> 
> I also found another issue, but I am not quite sure whether it is worth
> enough for our upstream: The related fix patch is below, which will let
> the initialization slower, but for most archs, they have no this issue.
> 
>     linux-user/mmap.c: Always zero MAP_ANONYMOUS memory in target_mmap()
>     
>     In some architectures, they have no policy to zero MAP_ANONYMOUS memory,
>     which will cause issue for qemu target_mmap.
>     
>     Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> 
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index 7b459d5..9c9152d 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -567,6 +567,10 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>      printf("\n");
>  #endif
>      tb_invalidate_phys_range(start, start + len);
> +    if ((prot & PROT_WRITE) && (flags & MAP_ANONYMOUS)
> +        && ((flags & MAP_PRIVATE) || (fd == -1))) {
> +        memset(g2h(start), 0, len);
> +    }

IMHO, their kernel needs a fix, mmap(2):

MAP_ANONYMOUS
        The mapping is not backed by any file; its contents are initial‐
        ized to zero.


>      mmap_unlock();
>      return start;
>  fail:
> 
> 
> Thanks.

Laurent
Chen Gang Dec. 18, 2015, 10:47 a.m. UTC | #2 
On 2015年12月18日 17:41, Laurent Vivier wrote:
> 
> 
> Le 18/12/2015 07:51, Chen Gang a écrit :
>>
>> I found this issue during my working time, it is about sw_64 (almost the
>> same as alpha) host running i386 wine programs.
>>
>> I also found another issue, but I am not quite sure whether it is worth
>> enough for our upstream: The related fix patch is below, which will let
>> the initialization slower, but for most archs, they have no this issue.
>>
>>     linux-user/mmap.c: Always zero MAP_ANONYMOUS memory in target_mmap()
>>     
>>     In some architectures, they have no policy to zero MAP_ANONYMOUS memory,
>>     which will cause issue for qemu target_mmap.
>>     
>>     Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>
>> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
>> index 7b459d5..9c9152d 100644
>> --- a/linux-user/mmap.c
>> +++ b/linux-user/mmap.c
>> @@ -567,6 +567,10 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>>      printf("\n");
>>  #endif
>>      tb_invalidate_phys_range(start, start + len);
>> +    if ((prot & PROT_WRITE) && (flags & MAP_ANONYMOUS)
>> +        && ((flags & MAP_PRIVATE) || (fd == -1))) {
>> +        memset(g2h(start), 0, len);
>> +    }
> 
> IMHO, their kernel needs a fix, mmap(2):
> 
> MAP_ANONYMOUS
>         The mapping is not backed by any file; its contents are initial‐
>         ized to zero.
> 

OK, Thanks.
Chen Gang Dec. 21, 2015, 2:47 a.m. UTC | #3 
On 2015年12月18日 18:47, Chen Gang wrote:
> 
> On 2015年12月18日 17:41, Laurent Vivier wrote:
>>
>> Le 18/12/2015 07:51, Chen Gang a écrit :

[...]

>>>
>>> +++ b/linux-user/mmap.c
>>> @@ -567,6 +567,10 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>>>      printf("\n");
>>>  #endif
>>>      tb_invalidate_phys_range(start, start + len);
>>> +    if ((prot & PROT_WRITE) && (flags & MAP_ANONYMOUS)
>>> +        && ((flags & MAP_PRIVATE) || (fd == -1))) {
>>> +        memset(g2h(start), 0, len);
>>> +    }
>>
>> IMHO, their kernel needs a fix, mmap(2):
>>
>> MAP_ANONYMOUS
>>         The mapping is not backed by any file; its contents are initial‐
>>         ized to zero.
>>

Oh, sorry, after check again, it is not kernel's issue: mmap() will
always zero the ANONYMOUS memory for kernel 3.8 in sw_64 arch.

It is still our qemu's issue in mmap_frag (do not zero ANONYMOUS memory
fragments), so I sent patch v2 for it, please help check.

Thanks.

> 
> OK, Thanks.
> 
>
diff mbox

Patch

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 7b459d5..9c9152d 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -567,6 +567,10 @@  abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
     printf("\n");
 #endif
     tb_invalidate_phys_range(start, start + len);
+    if ((prot & PROT_WRITE) && (flags & MAP_ANONYMOUS)
+        && ((flags & MAP_PRIVATE) || (fd == -1))) {
+        memset(g2h(start), 0, len);
+    }
     mmap_unlock();
     return start;
 fail: