| Message ID | 20230909193704.1827-18-kariem.taha2.7@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | bsd-user: Implement mmap related system calls for FreeBSD. | expand |
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~ > + > + 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) */ > + 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);
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);
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~
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); + + 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) */ + 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);