| Message ID | 20230819094806.14965-11-kariem.taha2.7@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Implement the mmap system call for FreeBSD. | expand |
On Sat, Aug 19, 2023 at 3:48 AM Karim Taha <kariem.taha2.7@gmail.com> 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.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > Reviewed-by: Warner Losh <imp@bsdimp.com> diff --git a/bsd-user/bsd-mem.c b/bsd-user/bsd-mem.c > index e69250cc0d..4446c94725 100644 > --- a/bsd-user/bsd-mem.c > +++ b/bsd-user/bsd-mem.c > @@ -77,3 +77,49 @@ abi_long host_to_target_ipc_perm(abi_ulong target_addr, > return 0; > } > > +abi_long target_to_host_shmid_ds(struct shmid_ds *host_sd, > + abi_ulong target_addr) > +{ > + struct target_shmid_ds *target_sd; > + > + if (!lock_user_struct(VERIFY_READ, target_sd, target_addr, 1)) { > + return -TARGET_EFAULT; > + } > + if (target_to_host_ipc_perm(&(host_sd->shm_perm), target_addr)) { > + return -TARGET_EFAULT; > + } > + __get_user(host_sd->shm_segsz, &target_sd->shm_segsz); > + __get_user(host_sd->shm_lpid, &target_sd->shm_lpid); > + __get_user(host_sd->shm_cpid, &target_sd->shm_cpid); > + __get_user(host_sd->shm_nattch, &target_sd->shm_nattch); > + __get_user(host_sd->shm_atime, &target_sd->shm_atime); > + __get_user(host_sd->shm_dtime, &target_sd->shm_dtime); > + __get_user(host_sd->shm_ctime, &target_sd->shm_ctime); > + unlock_user_struct(target_sd, target_addr, 0); > + > + return 0; > +} > + > +abi_long host_to_target_shmid_ds(abi_ulong target_addr, > + struct shmid_ds *host_sd) > +{ > + struct target_shmid_ds *target_sd; > + > + if (!lock_user_struct(VERIFY_WRITE, target_sd, target_addr, 0)) { > + return -TARGET_EFAULT; > + } > + if (host_to_target_ipc_perm(target_addr, &(host_sd->shm_perm))) { > + return -TARGET_EFAULT; > + } > + __put_user(host_sd->shm_segsz, &target_sd->shm_segsz); > + __put_user(host_sd->shm_lpid, &target_sd->shm_lpid); > + __put_user(host_sd->shm_cpid, &target_sd->shm_cpid); > + __put_user(host_sd->shm_nattch, &target_sd->shm_nattch); > + __put_user(host_sd->shm_atime, &target_sd->shm_atime); > + __put_user(host_sd->shm_dtime, &target_sd->shm_dtime); > + __put_user(host_sd->shm_ctime, &target_sd->shm_ctime); > + unlock_user_struct(target_sd, target_addr, 1); > + > + return 0; > +} > + > -- > 2.40.0 > >
On 8/19/23 02:47, Karim Taha wrote: > + if (!lock_user_struct(VERIFY_WRITE, target_sd, target_addr, 0)) { > + return -TARGET_EFAULT; > + } > + if (host_to_target_ipc_perm(target_addr, &(host_sd->shm_perm))) { > + return -TARGET_EFAULT; > + } While it works, ideally you wouldn't double-lock a memory range, once here and once in host_to_target_ipc_perm. You could split out the middle of the function as host_to_target_ipc_perm__locked. Anyway, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 8/19/23 02:47, Karim Taha wrote: >> + if (!lock_user_struct(VERIFY_WRITE, target_sd, target_addr, 0)) { >> + return -TARGET_EFAULT; >> + } >> + if (host_to_target_ipc_perm(target_addr, &(host_sd->shm_perm))) { >> + return -TARGET_EFAULT; >> + } > > While it works, ideally you wouldn't double-lock a memory range, once here and once in > host_to_target_ipc_perm. You could split out the middle of the function as > host_to_target_ipc_perm__locked. Hi Richard, Can you please verify the correctness of the following refactoring? void host_to_target_ipc_perm__locked(abi_ulong target_addr, struct ipc_perm *host_ip) { struct target_ipc_perm *target_ip = g2h_untagged(target_addr); __put_user(host_ip->cuid, &target_ip->cuid); __put_user(host_ip->cgid, &target_ip->cgid); __put_user(host_ip->uid, &target_ip->uid); __put_user(host_ip->gid, &target_ip->gid); __put_user(host_ip->mode, &target_ip->mode); __put_user(host_ip->seq, &target_ip->seq); __put_user(host_ip->key, &target_ip->key); } abi_long host_to_target_shmid_ds(abi_ulong target_addr, struct shmid_ds *host_sd) { struct target_shmid_ds *target_sd; target_sd = lock_user(VERIFY_WRITE, target_addr, sizeof(*target_sd), 0); if (!target_sd){ return -TARGET_EFAULT; } host_to_target_ipc_perm__locked(target_addr, &(host_sd->shm_perm)); __put_user(host_sd->shm_segsz, &target_sd->shm_segsz); __put_user(host_sd->shm_lpid, &target_sd->shm_lpid); __put_user(host_sd->shm_cpid, &target_sd->shm_cpid); __put_user(host_sd->shm_nattch, &target_sd->shm_nattch); __put_user(host_sd->shm_atime, &target_sd->shm_atime); __put_user(host_sd->shm_dtime, &target_sd->shm_dtime); __put_user(host_sd->shm_ctime, &target_sd->shm_ctime); unlock_user_struct(target_sd, target_addr, 1); return 0; } As far as I understood the `page_check_range` function, defined at accel/tcg/user-exec.c::523: -The locked range is (target_addr, target_addr + sizeof(target_ipc_perm) -1) in case of host_to_target_ipc_perm function. -The locked range is (target_addr, taregt_addr + sizeof(target_shmid_ds) -1) in case of host_to_target_shmid_ds function. Since `host_to_target_shmid_ds` struct has larger size, in the original code, is the sucess of the first lock guarantees the sucess of the second? If I got it wrong, please elaborate further. If I'm correct, do you think I should call g2h_untagged in `host_to_target_ipc_perm__locked` directly, or should I receive it as a paremeter? -- Kariiem Taha
On 9/3/23 01:45, Kariiem Taha wrote: > Richard Henderson <richard.henderson@linaro.org> writes: > >> On 8/19/23 02:47, Karim Taha wrote: >>> + if (!lock_user_struct(VERIFY_WRITE, target_sd, target_addr, 0)) { >>> + return -TARGET_EFAULT; >>> + } >>> + if (host_to_target_ipc_perm(target_addr, &(host_sd->shm_perm))) { >>> + return -TARGET_EFAULT; >>> + } >> >> While it works, ideally you wouldn't double-lock a memory range, once here and once in >> host_to_target_ipc_perm. You could split out the middle of the function as >> host_to_target_ipc_perm__locked. > > Hi Richard, > > Can you please verify the correctness of the following refactoring? > void host_to_target_ipc_perm__locked(abi_ulong target_addr, > struct ipc_perm *host_ip) > { > struct target_ipc_perm *target_ip = g2h_untagged(target_addr); > __put_user(host_ip->cuid, &target_ip->cuid); > __put_user(host_ip->cgid, &target_ip->cgid); > __put_user(host_ip->uid, &target_ip->uid); > __put_user(host_ip->gid, &target_ip->gid); > __put_user(host_ip->mode, &target_ip->mode); > __put_user(host_ip->seq, &target_ip->seq); > __put_user(host_ip->key, &target_ip->key); > } > > abi_long host_to_target_shmid_ds(abi_ulong target_addr, > struct shmid_ds *host_sd) > { > struct target_shmid_ds *target_sd; > target_sd = lock_user(VERIFY_WRITE, target_addr, sizeof(*target_sd), 0); > if (!target_sd){ > return -TARGET_EFAULT; > } > > host_to_target_ipc_perm__locked(target_addr, &(host_sd->shm_perm)); No. You'd pass &target_sd->shm_perm, not target_addr, and you don't use g2h at all. r~
diff --git a/bsd-user/bsd-mem.c b/bsd-user/bsd-mem.c index e69250cc0d..4446c94725 100644 --- a/bsd-user/bsd-mem.c +++ b/bsd-user/bsd-mem.c @@ -77,3 +77,49 @@ abi_long host_to_target_ipc_perm(abi_ulong target_addr, return 0; } +abi_long target_to_host_shmid_ds(struct shmid_ds *host_sd, + abi_ulong target_addr) +{ + struct target_shmid_ds *target_sd; + + if (!lock_user_struct(VERIFY_READ, target_sd, target_addr, 1)) { + return -TARGET_EFAULT; + } + if (target_to_host_ipc_perm(&(host_sd->shm_perm), target_addr)) { + return -TARGET_EFAULT; + } + __get_user(host_sd->shm_segsz, &target_sd->shm_segsz); + __get_user(host_sd->shm_lpid, &target_sd->shm_lpid); + __get_user(host_sd->shm_cpid, &target_sd->shm_cpid); + __get_user(host_sd->shm_nattch, &target_sd->shm_nattch); + __get_user(host_sd->shm_atime, &target_sd->shm_atime); + __get_user(host_sd->shm_dtime, &target_sd->shm_dtime); + __get_user(host_sd->shm_ctime, &target_sd->shm_ctime); + unlock_user_struct(target_sd, target_addr, 0); + + return 0; +} + +abi_long host_to_target_shmid_ds(abi_ulong target_addr, + struct shmid_ds *host_sd) +{ + struct target_shmid_ds *target_sd; + + if (!lock_user_struct(VERIFY_WRITE, target_sd, target_addr, 0)) { + return -TARGET_EFAULT; + } + if (host_to_target_ipc_perm(target_addr, &(host_sd->shm_perm))) { + return -TARGET_EFAULT; + } + __put_user(host_sd->shm_segsz, &target_sd->shm_segsz); + __put_user(host_sd->shm_lpid, &target_sd->shm_lpid); + __put_user(host_sd->shm_cpid, &target_sd->shm_cpid); + __put_user(host_sd->shm_nattch, &target_sd->shm_nattch); + __put_user(host_sd->shm_atime, &target_sd->shm_atime); + __put_user(host_sd->shm_dtime, &target_sd->shm_dtime); + __put_user(host_sd->shm_ctime, &target_sd->shm_ctime); + unlock_user_struct(target_sd, target_addr, 1); + + return 0; +} +