Re: [PATCH v3 17/23] bsd-user: Implement mincore(2)

[Karim Taha <kariem.taha2.7@gmail.com>]

Richard Henderson <richard.henderson@linaro.org> wrote:

> On 9/9/23 12:36, Karim Taha wrote:
>> From: Stacey Son <sson@FreeBSD.org>
>> 
>> Signed-off-by: Stacey Son <sson@FreeBSD.org>
>> Signed-off-by: Karim Taha <kariem.taha2.7@gmail.com>
>> ---
>>   bsd-user/bsd-mem.h            | 22 ++++++++++++++++++++++
>>   bsd-user/freebsd/os-syscall.c |  4 ++++
>>   2 files changed, 26 insertions(+)
>> 
>> diff --git a/bsd-user/bsd-mem.h b/bsd-user/bsd-mem.h
>> index 0e16051418..1dabbe36e6 100644
>> --- a/bsd-user/bsd-mem.h
>> +++ b/bsd-user/bsd-mem.h
>> @@ -189,4 +189,26 @@ static inline abi_long do_bsd_minherit(abi_long addr, abi_long len,
>>       return get_errno(minherit(g2h_untagged(addr), len, inherit));
>>   }
>>   
>> +/* mincore(2) */
>> +static inline abi_long do_bsd_mincore(abi_ulong target_addr, abi_ulong len,
>> +        abi_ulong target_vec)
>> +{
>> +    abi_long ret;
>> +    void *p;
>> +    abi_ulong vec_len = DIV_ROUND_UP(len,TARGET_PAGE_SIZE);
>> +
>> +    if (!guest_range_valid_untagged(target_addr,len) || !page_check_range(target_addr, len, PAGE_VALID)) {
>> +        return -TARGET_EFAULT;
>> +    }
>> +
>> +    p = lock_user(VERIFY_WRITE, target_vec, vec_len, 0);
>> +    if (p == NULL) {
>> +        return -TARGET_EFAULT;
>> +    }
>> +    ret = get_errno(mincore(g2h_untagged(target_addr), len, p));
>> +    unlock_user(p, target_vec, 0);
>
> You don't need the lock/unlock_user at all.  It is wrongly checking for WRITE.
>
>
> r~
>
AFAIU, the host is writing to the target's memory, right?

So this is similar to IPC_STAT case from the shmctl(2) syscall,
where host_to_target_shmid_ds, which has a `lock_user(VERIFY_WRITE,...)`, writes the struct `dsarg` set by
the host syscall `shmctl` at the address `buff` in the target memory.

Is it correct if the host writes to the target without
locking? for example, `mincore(g2h_untagged(target_addr), len, p)` can be done
without locking.

The locking was suggested by you in response to the v1 implementation
which used `lock_user_string`.

>> +
>> +    return ret;
>> +}
>> +
>>   #endif /* BSD_USER_BSD_MEM_H */
>> diff --git a/bsd-user/freebsd/os-syscall.c b/bsd-user/freebsd/os-syscall.c
>> index f5d60cf902..8d1cf3b35c 100644
>> --- a/bsd-user/freebsd/os-syscall.c
>> +++ b/bsd-user/freebsd/os-syscall.c
>> @@ -527,6 +527,10 @@ static abi_long freebsd_syscall(void *cpu_env, int num, abi_long arg1,
>>           ret = do_bsd_minherit(arg1, arg2, arg3);
>>           break;
>>   
>> +    case TARGET_FREEBSD_NR_mincore: /* mincore(2) */
p>> +        ret = do_bsd_mincore(arg1, arg2, arg3);
>> +        break;
>> +
>>   #if defined(__FreeBSD_version) && __FreeBSD_version >= 1300048
>>       case TARGET_FREEBSD_NR_shm_open2: /* shm_open2(2) */
>>           ret = do_freebsd_shm_open2(arg1, arg2, arg3, arg4, arg5);