* [PATCH v2] linux-user: ppoll: eliminate large alloca
@ 2023-08-24 17:51 Michael Tokarev
2023-08-25 5:07 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 2+ messages in thread
From: Michael Tokarev @ 2023-08-24 17:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Laurent Vivier, Michael Tokarev
do_ppoll() in linux-user/syscall.c uses alloca() to
allocate an array of struct pullfds on the stack.
The only upper boundary for number of entries for this
array is so that whole thing fits in INT_MAX. But this
is definitely too much for a stack allocation. Use
heap allocation instead.
This, together with previous patch for getgroups(),
eliminates all large on-stack allocations from
qemu-user/syscall.c. What's left are actually small
ones.
While at it, also fix missing unlock_user() in two
places, and consolidate target_to_host_timespec*()
calls into time64?_timespec():_timespec64() construct.
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
v1: https://patchwork.ozlabs.org/project/qemu-devel/patch/20221216192220.2881898-1-mjt@msgid.tls.msk.ru/
v2: remove alloca() optimization for smaller number of fds
linux-user/syscall.c | 45 ++++++++++++++++++++------------------------
1 file changed, 20 insertions(+), 25 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 9353268cc1..e79594bcd5 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1487,14 +1487,12 @@ static abi_long do_pselect6(abi_long arg1, abi_long arg2, abi_long arg3,
static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3,
abi_long arg4, abi_long arg5, bool ppoll, bool time64)
{
- struct target_pollfd *target_pfd;
+ struct target_pollfd *target_pfd = NULL;
unsigned int nfds = arg2;
- struct pollfd *pfd;
+ struct pollfd *pfd = NULL;
unsigned int i;
abi_long ret;
- pfd = NULL;
- target_pfd = NULL;
if (nfds) {
if (nfds > (INT_MAX / sizeof(struct target_pollfd))) {
return -TARGET_EINVAL;
@@ -1505,7 +1503,11 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3,
return -TARGET_EFAULT;
}
- pfd = alloca(sizeof(struct pollfd) * nfds);
+ pfd = g_try_new(struct pollfd, nfds);
+ if (!pfd) {
+ ret = -TARGET_ENOMEM;
+ goto out;
+ }
for (i = 0; i < nfds; i++) {
pfd[i].fd = tswap32(target_pfd[i].fd);
pfd[i].events = tswap16(target_pfd[i].events);
@@ -1516,16 +1518,11 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3,
sigset_t *set = NULL;
if (arg3) {
- if (time64) {
- if (target_to_host_timespec64(timeout_ts, arg3)) {
- unlock_user(target_pfd, arg1, 0);
- return -TARGET_EFAULT;
- }
- } else {
- if (target_to_host_timespec(timeout_ts, arg3)) {
- unlock_user(target_pfd, arg1, 0);
- return -TARGET_EFAULT;
- }
+ if (time64
+ ? target_to_host_timespec64(timeout_ts, arg3)
+ : target_to_host_timespec(timeout_ts, arg3)) {
+ ret = -TARGET_EFAULT;
+ goto out;
}
} else {
timeout_ts = NULL;
@@ -1534,8 +1531,7 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3,
if (arg4) {
ret = process_sigsuspend_mask(&set, arg4, arg5);
if (ret != 0) {
- unlock_user(target_pfd, arg1, 0);
- return ret;
+ goto out;
}
}
@@ -1546,14 +1542,11 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3,
finish_sigsuspend_mask(ret);
}
if (!is_error(ret) && arg3) {
- if (time64) {
- if (host_to_target_timespec64(arg3, timeout_ts)) {
- return -TARGET_EFAULT;
- }
- } else {
- if (host_to_target_timespec(arg3, timeout_ts)) {
- return -TARGET_EFAULT;
- }
+ if (time64
+ ? host_to_target_timespec64(arg3, timeout_ts)
+ : host_to_target_timespec(arg3, timeout_ts)) {
+ ret = -TARGET_EFAULT;
+ goto out;
}
}
} else {
@@ -1576,6 +1569,8 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3,
target_pfd[i].revents = tswap16(pfd[i].revents);
}
}
+out:
+ g_free(pfd);
unlock_user(target_pfd, arg1, sizeof(struct target_pollfd) * nfds);
return ret;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] linux-user: ppoll: eliminate large alloca
2023-08-24 17:51 [PATCH v2] linux-user: ppoll: eliminate large alloca Michael Tokarev
@ 2023-08-25 5:07 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 2+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-08-25 5:07 UTC (permalink / raw)
To: Michael Tokarev, qemu-devel; +Cc: Laurent Vivier
On 24/8/23 19:51, Michael Tokarev wrote:
> do_ppoll() in linux-user/syscall.c uses alloca() to
> allocate an array of struct pullfds on the stack.
> The only upper boundary for number of entries for this
> array is so that whole thing fits in INT_MAX. But this
> is definitely too much for a stack allocation. Use
> heap allocation instead.
>
> This, together with previous patch for getgroups(),
> eliminates all large on-stack allocations from
> qemu-user/syscall.c. What's left are actually small
> ones.
>
> While at it, also fix missing unlock_user() in two
> places, and consolidate target_to_host_timespec*()
> calls into time64?_timespec():_timespec64() construct.
>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
> v1: https://patchwork.ozlabs.org/project/qemu-devel/patch/20221216192220.2881898-1-mjt@msgid.tls.msk.ru/
> v2: remove alloca() optimization for smaller number of fds
>
> linux-user/syscall.c | 45 ++++++++++++++++++++------------------------
> 1 file changed, 20 insertions(+), 25 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 9353268cc1..e79594bcd5 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1487,14 +1487,12 @@ static abi_long do_pselect6(abi_long arg1, abi_long arg2, abi_long arg3,
> static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3,
> abi_long arg4, abi_long arg5, bool ppoll, bool time64)
> {
> - struct target_pollfd *target_pfd;
> + struct target_pollfd *target_pfd = NULL;
> unsigned int nfds = arg2;
> - struct pollfd *pfd;
> + struct pollfd *pfd = NULL;
> unsigned int i;
> abi_long ret;
>
> - pfd = NULL;
> - target_pfd = NULL;
> if (nfds) {
> if (nfds > (INT_MAX / sizeof(struct target_pollfd))) {
> return -TARGET_EINVAL;
> @@ -1505,7 +1503,11 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3,
> return -TARGET_EFAULT;
> }
>
> - pfd = alloca(sizeof(struct pollfd) * nfds);
> + pfd = g_try_new(struct pollfd, nfds);
> + if (!pfd) {
> + ret = -TARGET_ENOMEM;
> + goto out;
> + }
> for (i = 0; i < nfds; i++) {
> pfd[i].fd = tswap32(target_pfd[i].fd);
> pfd[i].events = tswap16(target_pfd[i].events);
> @@ -1516,16 +1518,11 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3,
> sigset_t *set = NULL;
>
> if (arg3) {
> - if (time64) {
> - if (target_to_host_timespec64(timeout_ts, arg3)) {
> - unlock_user(target_pfd, arg1, 0);
So this changes unlock(sz=0) ...
> - return -TARGET_EFAULT;
> - }
> - } else {
> - if (target_to_host_timespec(timeout_ts, arg3)) {
> - unlock_user(target_pfd, arg1, 0);
> - return -TARGET_EFAULT;
> - }
> + if (time64
> + ? target_to_host_timespec64(timeout_ts, arg3)
> + : target_to_host_timespec(timeout_ts, arg3)) {
> + ret = -TARGET_EFAULT;
> + goto out;
... to unlock(sz=pollfd*nfds). Is that also part of your
"While at it" comment?
Having one patch for each logical change eases review /
cherry-pick / backport.
> }
> } else {
> timeout_ts = NULL;
> @@ -1534,8 +1531,7 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3,
> if (arg4) {
> ret = process_sigsuspend_mask(&set, arg4, arg5);
> if (ret != 0) {
> - unlock_user(target_pfd, arg1, 0);
(Ditto)
> - return ret;
> + goto out;
> }
> }
>
> @@ -1546,14 +1542,11 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3,
> finish_sigsuspend_mask(ret);
> }
> if (!is_error(ret) && arg3) {
> - if (time64) {
> - if (host_to_target_timespec64(arg3, timeout_ts)) {
> - return -TARGET_EFAULT;
> - }
> - } else {
> - if (host_to_target_timespec(arg3, timeout_ts)) {
> - return -TARGET_EFAULT;
> - }
> + if (time64
> + ? host_to_target_timespec64(arg3, timeout_ts)
> + : host_to_target_timespec(arg3, timeout_ts)) {
> + ret = -TARGET_EFAULT;
> + goto out;
> }
> }
> } else {
> @@ -1576,6 +1569,8 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3,
> target_pfd[i].revents = tswap16(pfd[i].revents);
> }
> }
> +out:
> + g_free(pfd);
> unlock_user(target_pfd, arg1, sizeof(struct target_pollfd) * nfds);
> return ret;
> }
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-08-25 5:08 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-24 17:51 [PATCH v2] linux-user: ppoll: eliminate large alloca Michael Tokarev
2023-08-25 5:07 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).