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

[Richard Henderson <richard.henderson@linaro.org>]

On 9/13/23 15:02, Karim Taha wrote:
> 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?

Oops, I misread this.  The lock/unlock is for the output vector,
which means you do need it.

You also need unlock_user(p, target_vec, vec_len).

With that,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~