| Message ID | 20230914074337.149897-4-mjt@tls.msk.ru |
|---|---|
| State | New |
| Headers | show |
| Series | linux-user/syscall.c: do_ppoll: eliminate large alloca | expand |
On Thu, Sep 14, 2023 at 10:43:37AM +0300, 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. This is definitely too much > for stack allocation. > > Use heap allocation when large number of entries is requested > (currently 32, arbitrary), and continue to use alloca() for Typo ? The code uses 64 rather than 32. > smaller allocations, to optimize small operations for small > sizes. The code for this optimization is small, I see no > reason for dropping it. > > This eliminates last large user-controlled on-stack allocation > from syscall.c. > > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> > --- > linux-user/syscall.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index eabdf50abc..1dbe28eba4 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -1489,7 +1489,7 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3, > { > struct target_pollfd *target_pfd = NULL; > unsigned int nfds = arg2; > - struct pollfd *pfd = NULL; > + struct pollfd *pfd = NULL, *heap_pfd = NULL; g_autofree struct pollfd *heap_pdf = NULL; > unsigned int i; > abi_long ret; > > @@ -1503,7 +1503,17 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3, > return -TARGET_EFAULT; > } > > - pfd = alloca(sizeof(struct pollfd) * nfds); > + /* arbitrary "small" number to limit stack usage */ > + if (nfds <= 64) { > + pfd = alloca(sizeof(struct pollfd) * nfds); > + } else { > + heap_pfd = g_try_new(struct pollfd, nfds); > + if (!heap_pfd) { > + ret = -TARGET_ENOMEM; > + goto out; > + } > + pfd = heap_pfd; > + } > for (i = 0; i < nfds; i++) { > pfd[i].fd = tswap32(target_pfd[i].fd); > pfd[i].events = tswap16(target_pfd[i].events); > @@ -1567,6 +1577,7 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3, > } > > out: > + g_free(heap_pfd); This can be dropped with g_autofree usage > unlock_user(target_pfd, arg1, sizeof(struct target_pollfd) * nfds); > return ret; > } > -- > 2.39.2 > > With regards, Daniel
14.09.2023 11:18, Daniel P. Berrangé wrote: > On Thu, Sep 14, 2023 at 10:43:37AM +0300, 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. This is definitely too much >> for stack allocation. >> >> Use heap allocation when large number of entries is requested >> (currently 32, arbitrary), and continue to use alloca() for > > Typo ? The code uses 64 rather than 32. Yeah, it's a typo, after a few iterations trying to split this all into pieces and editing in the process. >> - struct pollfd *pfd = NULL; >> + struct pollfd *pfd = NULL, *heap_pfd = NULL; > > g_autofree struct pollfd *heap_pdf = NULL; > ... >> >> out: >> + g_free(heap_pfd); > > This can be dropped with g_autofree usage Yes, I know this, - this was deliberate choice. Personally I'm just too used to old-school explicit resource deallocations. Here, there's a single place where everything gets freed, so there's little reason to use fancy modern automatic deallocations. To my taste anyway. Maybe some future modifications adding some future ppoll3.. :) Sure thing I can drop that and change it to autofree. Thanks, /mjt
14.09.2023 11:26, Michael Tokarev wrote: > 14.09.2023 11:18, Daniel P. Berrangé wrote: .. >>> - struct pollfd *pfd = NULL; >>> + struct pollfd *pfd = NULL, *heap_pfd = NULL; >> >> g_autofree struct pollfd *heap_pdf = NULL; >> > ... >>> out: >>> + g_free(heap_pfd); >> >> This can be dropped with g_autofree usage > > Yes, I know this, - this was deliberate choice. > Personally I'm just too used to old-school explicit resource deallocations. > Here, there's a single place where everything gets freed, so there's little > reason to use fancy modern automatic deallocations. To my taste anyway. > Maybe some future modifications adding some future ppoll3.. :) > > Sure thing I can drop that and change it to autofree. Should I? If that's easier in todays world :) /mjt
On Thu, Sep 14, 2023 at 02:05:21PM +0300, Michael Tokarev wrote: > 14.09.2023 11:26, Michael Tokarev wrote: > > 14.09.2023 11:18, Daniel P. Berrangé wrote: > .. > > > > - struct pollfd *pfd = NULL; > > > > + struct pollfd *pfd = NULL, *heap_pfd = NULL; > > > > > > g_autofree struct pollfd *heap_pdf = NULL; > > > > > ... > > > > out: > > > > + g_free(heap_pfd); > > > > > > This can be dropped with g_autofree usage > > > > Yes, I know this, - this was deliberate choice. > > Personally I'm just too used to old-school explicit resource deallocations. > > Here, there's a single place where everything gets freed, so there's little > > reason to use fancy modern automatic deallocations. To my taste anyway. > > Maybe some future modifications adding some future ppoll3.. :) > > > > Sure thing I can drop that and change it to autofree. > > Should I? If that's easier in todays world :) I prefer auto-free, but I'm fine with this commit either way, so Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel
14.09.2023 14:07, Daniel P. Berrangé wrote: .. >>>> This can be dropped with g_autofree usage >>> >>> Yes, I know this, - this was deliberate choice. >>> Personally I'm just too used to old-school explicit resource deallocations. >>> Here, there's a single place where everything gets freed, so there's little >>> reason to use fancy modern automatic deallocations. To my taste anyway. > I prefer auto-free, but I'm fine with this commit either way, so After thinking about it more. Once these automatic helpers such as g_autofree pointers slip into peoples minds, there's much less attention being paid for freeing resources. So I'll go with the autofree variant, without explicit free. > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Thank you for the review! /mjt
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index eabdf50abc..1dbe28eba4 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -1489,7 +1489,7 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3, { struct target_pollfd *target_pfd = NULL; unsigned int nfds = arg2; - struct pollfd *pfd = NULL; + struct pollfd *pfd = NULL, *heap_pfd = NULL; unsigned int i; abi_long ret; @@ -1503,7 +1503,17 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3, return -TARGET_EFAULT; } - pfd = alloca(sizeof(struct pollfd) * nfds); + /* arbitrary "small" number to limit stack usage */ + if (nfds <= 64) { + pfd = alloca(sizeof(struct pollfd) * nfds); + } else { + heap_pfd = g_try_new(struct pollfd, nfds); + if (!heap_pfd) { + ret = -TARGET_ENOMEM; + goto out; + } + pfd = heap_pfd; + } for (i = 0; i < nfds; i++) { pfd[i].fd = tswap32(target_pfd[i].fd); pfd[i].events = tswap16(target_pfd[i].events); @@ -1567,6 +1577,7 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3, } out: + g_free(heap_pfd); unlock_user(target_pfd, arg1, sizeof(struct target_pollfd) * nfds); return ret; }
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. This is definitely too much for stack allocation. Use heap allocation when large number of entries is requested (currently 32, arbitrary), and continue to use alloca() for smaller allocations, to optimize small operations for small sizes. The code for this optimization is small, I see no reason for dropping it. This eliminates last large user-controlled on-stack allocation from syscall.c. Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> --- linux-user/syscall.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)