[PATCH v3 0/3] linux-user/syscall.c: do_ppoll: eliminate large alloca

[PATCH v3 0/3] linux-user/syscall.c: do_ppoll: eliminate large alloca

[Michael Tokarev]

This is a v3 patch (now patchset) which eliminates guest-controlled
alloca from linux-user:poll.  I now split out 2 unrelated preparational
changes into its own patches, for easy review.

The small optmization which were here in v1 is still there.  In huge
number of use cases, poll() et all are called with just one file
descriptor, there's no need to use heap allocation for this, and the
code to avoid this heap allocation is already there, is short and is
easy to read too (YMMV).

This patchset passes poll- and ppoll-related LTSP tests, except of
the ppoll_time64 case (tested on ppc64):

ppoll01.c:174: TCONF: syscall(414) __NR_ppoll_time64 not supported on your arch

which was here before.

Michael Tokarev (3):
  linux-user/syscall.c: do_ppoll: simplify time64 host<=>target
    conversion expressions
  linux-user/syscall.c: do_ppoll: consolidate and fix the forgotten
    unlock_user
  linux-user/syscall.c: do_ppoll: eliminate large alloca

 linux-user/syscall.c | 52 +++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

-- 
2.39.2

[PATCH v3 1/3] linux-user/syscall.c: do_ppoll: simplify time64 host<=>target conversion expressions

[Michael Tokarev]

replace if (time64) { time64-expr } else { time32-expr }
expressions which are difficult to read with a much shorter
and easier to read arithmetic-if constructs.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 linux-user/syscall.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 3521a2d70b..33bf84c205 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1516,16 +1516,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)) {
+                unlock_user(target_pfd, arg1, 0);
+                return -TARGET_EFAULT;
             }
         } else {
             timeout_ts = NULL;
@@ -1546,14 +1541,10 @@ 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)) {
+                return -TARGET_EFAULT;
             }
         }
     } else {
-- 
2.39.2

[PATCH v3 2/3] linux-user/syscall.c: do_ppoll: consolidate and fix the forgotten unlock_user

[Michael Tokarev]

in do_ppoll we've one place where unlock_user isn't done
at all, while other places use 0 for the size of the area
being unlocked instead of the actual size.

Instead of open-coding calls to unlock_user(), jump to the
end of this function and do a single call to unlock there.

Note: original code calls unlock_user() with target_pfd being
NULL in one case (when nfds == 0).   Move initializers to
variable declarations, - I wondered a few times if target_pfd
isn't being initialized at all for unlock_user.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 linux-user/syscall.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 33bf84c205..eabdf50abc 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;
@@ -1519,8 +1517,8 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3,
             if (time64
                 ? target_to_host_timespec64(timeout_ts, arg3)
                 : target_to_host_timespec(timeout_ts, arg3)) {
-                unlock_user(target_pfd, arg1, 0);
-                return -TARGET_EFAULT;
+                ret = -TARGET_EFAULT;
+                goto out;
             }
         } else {
             timeout_ts = NULL;
@@ -1529,8 +1527,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;
             }
         }
 
@@ -1544,7 +1541,8 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3,
             if (time64
                 ? host_to_target_timespec64(arg3, timeout_ts)
                 : host_to_target_timespec(arg3, timeout_ts)) {
-                return -TARGET_EFAULT;
+                ret = -TARGET_EFAULT;
+                goto out;
             }
         }
     } else {
@@ -1567,6 +1565,8 @@ static abi_long do_ppoll(abi_long arg1, abi_long arg2, abi_long arg3,
             target_pfd[i].revents = tswap16(pfd[i].revents);
         }
     }
+
+out:
     unlock_user(target_pfd, arg1, sizeof(struct target_pollfd) * nfds);
     return ret;
 }
-- 
2.39.2

[PATCH v3 3/3] linux-user/syscall.c: do_ppoll: eliminate large alloca

[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.  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(-)

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;
 }
-- 
2.39.2

Re: [PATCH v3 3/3] linux-user/syscall.c: do_ppoll: eliminate large alloca

[Daniel P. Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 3/3] linux-user/syscall.c: do_ppoll: eliminate large alloca

[Michael Tokarev]

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

Re: [PATCH v3 3/3] linux-user/syscall.c: do_ppoll: eliminate large alloca

[Michael Tokarev]

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

Re: [PATCH v3 3/3] linux-user/syscall.c: do_ppoll: eliminate large alloca

[Daniel P. Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 3/3] linux-user/syscall.c: do_ppoll: eliminate large alloca

[Michael Tokarev]

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