| Message ID | 20220607201440.41464-3-imp@bsdimp.com |
|---|---|
| State | New |
| Headers | show |
| Series | bsd-user upstreaming: read, write and exit | expand |
On 6/7/22 13:14, Warner Losh wrote: > +void unlock_iovec(struct iovec *vec, abi_ulong target_addr, > + int count, int copy) > +{ > + struct target_iovec *target_vec; > + > + target_vec = lock_user(VERIFY_READ, target_addr, > + count * sizeof(struct target_iovec), 1); > + if (target_vec) { Locking the same region twice seems like a bad idea. How about something like typedef struct { struct target_iovec *target; abi_ulong target_addr; int count; struct iovec host[]; } IOVecMap; IOVecMap *lock_iovec(abi_ulong target_addr, int count, bool copy_in) { IOVecMap *map; if (count == 0) ... if (count < 0) ... map = g_try_malloc0(sizeof(IOVecNew) + sizeof(struct iovec) * count); if (!map) ... map->target = lock_user(...); if (!map->target) { g_free(map); errno = EFAULT; return NULL; } map->target_addr = target_addr; map->count = count; // lock loop fail: unlock_iovec(vec, false); errno = err; return NULL; } void unlock_iovec(IOVecMap *map, bool copy_out) { for (int i = 0, count = map->count; i < count; ++i) { if (map->host[i].iov_base) { abi_ulong target_base = tswapal(map->target[i].iov_base); unlock_user(map->host[i].iov_base, target_base, copy_out ? map->host[i].iov_len : 0); } } unlock_user(map->target, map->target_addr, 0); g_free(map); } r~
On Tue, Jun 7, 2022 at 2:28 PM Richard Henderson < richard.henderson@linaro.org> wrote: > On 6/7/22 13:14, Warner Losh wrote: > > +void unlock_iovec(struct iovec *vec, abi_ulong target_addr, > > + int count, int copy) > > +{ > > + struct target_iovec *target_vec; > > + > > + target_vec = lock_user(VERIFY_READ, target_addr, > > + count * sizeof(struct target_iovec), 1); > > + if (target_vec) { > > Locking the same region twice seems like a bad idea. > We unlock the iovec memory in the lock_iovec() > How about something like > > typedef struct { > struct target_iovec *target; > abi_ulong target_addr; > int count; > struct iovec host[]; > } IOVecMap; > > IOVecMap *lock_iovec(abi_ulong target_addr, int count, bool copy_in) > { > IOVecMap *map; > > if (count == 0) ... > if (count < 0) ... > > map = g_try_malloc0(sizeof(IOVecNew) + sizeof(struct iovec) * count); > if (!map) ... > > map->target = lock_user(...); > if (!map->target) { > g_free(map); > errno = EFAULT; > return NULL; > } > map->target_addr = target_addr; > map->count = count; > > // lock loop > > fail: > unlock_iovec(vec, false); > errno = err; > return NULL; > } > > void unlock_iovec(IOVecMap *map, bool copy_out) > { > for (int i = 0, count = map->count; i < count; ++i) { > if (map->host[i].iov_base) { > abi_ulong target_base = tswapal(map->target[i].iov_base); > unlock_user(map->host[i].iov_base, target_base, > copy_out ? map->host[i].iov_len : 0); > } > And wouldn't we want to filter out the iov_base that == 0 since we may terminate the loop before we get to the count. When the I/O is done, we'll call it not with the number we mapped, but with the original number... Or am I not understanding something here... Warner > } > unlock_user(map->target, map->target_addr, 0); > g_free(map); > } > > > r~ >
On 6/7/22 14:51, Warner Losh wrote: > void unlock_iovec(IOVecMap *map, bool copy_out) > { > for (int i = 0, count = map->count; i < count; ++i) { > if (map->host[i].iov_base) { > abi_ulong target_base = tswapal(map->target[i].iov_base); > unlock_user(map->host[i].iov_base, target_base, > copy_out ? map->host[i].iov_len : 0); > } > > > And wouldn't we want to filter out the iov_base that == 0 since > we may terminate the loop before we get to the count. When the > I/O is done, we'll call it not with the number we mapped, but with > the original number... Or am I not understanding something here... I'm not following -- when and why are you adjusting count? r~
> On Jun 7, 2022, at 3:23 PM, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 6/7/22 14:51, Warner Losh wrote: >> void unlock_iovec(IOVecMap *map, bool copy_out) >> { >> for (int i = 0, count = map->count; i < count; ++i) { >> if (map->host[i].iov_base) { >> abi_ulong target_base = tswapal(map->target[i].iov_base); >> unlock_user(map->host[i].iov_base, target_base, >> copy_out ? map->host[i].iov_len : 0); >> } >> And wouldn't we want to filter out the iov_base that == 0 since >> we may terminate the loop before we get to the count. When the >> I/O is done, we'll call it not with the number we mapped, but with >> the original number... Or am I not understanding something here... > > I'm not following -- when and why are you adjusting count? When we hit a memory range we can’t map after the first one, we effectively stop mapping in (in the current linux code we do map after, but then destroy the length). So that means we’ll have entries in the iovec that are zero, and this code doesn’t account for that. We’re not changing the count, per se, but have a scenario where they might wind up NULL. I’ll add “if I understand all this right” because I a little shaky still on these aspects of qemu’s soft mmu. Warner
On 6/7/22 16:35, Warner Losh wrote: > > >> On Jun 7, 2022, at 3:23 PM, Richard Henderson <richard.henderson@linaro.org> wrote: >> >> On 6/7/22 14:51, Warner Losh wrote: >>> void unlock_iovec(IOVecMap *map, bool copy_out) >>> { >>> for (int i = 0, count = map->count; i < count; ++i) { >>> if (map->host[i].iov_base) { >>> abi_ulong target_base = tswapal(map->target[i].iov_base); >>> unlock_user(map->host[i].iov_base, target_base, >>> copy_out ? map->host[i].iov_len : 0); >>> } >>> And wouldn't we want to filter out the iov_base that == 0 since >>> we may terminate the loop before we get to the count. When the >>> I/O is done, we'll call it not with the number we mapped, but with >>> the original number... Or am I not understanding something here... >> >> I'm not following -- when and why are you adjusting count? > > When we hit a memory range we can’t map after the first one, > we effectively stop mapping in (in the current linux code we > do map after, but then destroy the length). So that means > we’ll have entries in the iovec that are zero, and this code > doesn’t account for that. We’re not changing the count, per > se, but have a scenario where they might wind up NULL. ... and so skip them with the if. I mean, I suppose you could set map->count on error, as you say, so that we don't iterate so far, but... duh, error case. So long as you don't actively fail, there's no point in optimizing for it. r~
On Tue, Jun 7, 2022 at 7:02 PM Richard Henderson < richard.henderson@linaro.org> wrote: > On 6/7/22 16:35, Warner Losh wrote: > > > > > >> On Jun 7, 2022, at 3:23 PM, Richard Henderson < > richard.henderson@linaro.org> wrote: > >> > >> On 6/7/22 14:51, Warner Losh wrote: > >>> void unlock_iovec(IOVecMap *map, bool copy_out) > >>> { > >>> for (int i = 0, count = map->count; i < count; ++i) { > >>> if (map->host[i].iov_base) { > >>> abi_ulong target_base = > tswapal(map->target[i].iov_base); > >>> unlock_user(map->host[i].iov_base, target_base, > >>> copy_out ? map->host[i].iov_len : 0); > >>> } > >>> And wouldn't we want to filter out the iov_base that == 0 since > >>> we may terminate the loop before we get to the count. When the > >>> I/O is done, we'll call it not with the number we mapped, but with > >>> the original number... Or am I not understanding something here... > >> > >> I'm not following -- when and why are you adjusting count? > > > > When we hit a memory range we can’t map after the first one, > > we effectively stop mapping in (in the current linux code we > > do map after, but then destroy the length). So that means > > we’ll have entries in the iovec that are zero, and this code > > doesn’t account for that. We’re not changing the count, per > > se, but have a scenario where they might wind up NULL. > > ... and so skip them with the if. > > I mean, I suppose you could set map->count on error, as you say, so that > we don't iterate > so far, but... duh, error case. So long as you don't actively fail, > there's no point in > optimizing for it. > Setting the count would be hard because we'd have to allocate and free state that we're not currently doing. Better to just skip it with an if. We allocate a vector that's used in a number of places, and we'd have to change that code if we did things differently. While I'm open to suggestions here, I think that just accounting for the possible error with an if is our best bet for now. I have a lot of code to get in, and am hoping to not rewrite things unless there's some clear benefit over the existing structure (like fixing bugs, matching linux-user, or increasing performance). Warner
diff --git a/bsd-user/freebsd/os-syscall.c b/bsd-user/freebsd/os-syscall.c index c41ef0eda40..510307f29d9 100644 --- a/bsd-user/freebsd/os-syscall.c +++ b/bsd-user/freebsd/os-syscall.c @@ -186,6 +186,20 @@ fail2: return NULL; } +void unlock_iovec(struct iovec *vec, abi_ulong target_addr, + int count, int copy) +{ + struct target_iovec *target_vec; + + target_vec = lock_user(VERIFY_READ, target_addr, + count * sizeof(struct target_iovec), 1); + if (target_vec) { + helper_unlock_iovec(target_vec, target_addr, vec, count, copy); + } + + g_free(vec); +} + /* * do_syscall() should always have a single exit point at the end so that * actions, such as logging of syscall results, can be performed. All errnos
Releases the references to the iovec created by lock_iovec. Signed-off-by: Warner Losh <imp@bsdimp.com> --- bsd-user/freebsd/os-syscall.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)