Richard Henderson <richard.hender...@linaro.org> wrote:
> On 9/9/23 12:36, Karim Taha wrote:
>> From: Stacey Son <s...@freebsd.org>
>>
>> Signed-off-by: Stacey Son <s...@freebsd.org>
>> Signed-off-by: Karim Taha <kariem.taha...@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);
