Re: [PATCH v2 1/5] mips: Do not malloc on getdents64 fallback

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

Ping.

On 31/07/2019 15:31, Adhemerval Zanella wrote:
> This patch changes how the fallback getdents64 implementation calls
> non-LFS getdents by replacing the scratch_buffer with static buffer
> plus a loop on getdents calls.  This avoids the potential malloc
> call on scratch_buffer_set_array_size for large input buffer size
> at the cost of more getdents syscalls.
> 
> It also adds a small optimization for older kernels, where the first
> ENOSYS failure for getdents64 disable subsequent calls.
> 
> Check the dirent tests on a mips64-linux-gnu with getdents64 code
> disabled.
> 
> 	* sysdeps/unix/sysv/linux/mips/mips64/getdents64.c (__getdents64):
> 	Add small optimization for older kernel to avoid issuing
> 	__NR_getdents64 on each call and replace scratch_buffer usage with
> 	a static allocated buffer.
> ---
>  .../unix/sysv/linux/mips/mips64/getdents64.c  | 122 ++++++++----------
>  1 file changed, 54 insertions(+), 68 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
> index 8bf3abb0e0..3b5afd9324 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
> +++ b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
> @@ -22,98 +22,84 @@
>  #include <assert.h>
>  #include <sys/param.h>
>  #include <unistd.h>
> -#include <scratch_buffer.h>
>  #include <limits.h>
>  
>  ssize_t
> -__getdents64 (int fd, void *buf0, size_t nbytes)
> +__getdents64 (int fd, void *buf, size_t nbytes)
>  {
> -  char *buf = buf0;
> -
>    /* The system call takes an unsigned int argument, and some length
>       checks in the kernel use an int type.  */
>    if (nbytes > INT_MAX)
>      nbytes = INT_MAX;
>  
>  #ifdef __NR_getdents64
> -  ssize_t ret = INLINE_SYSCALL_CALL (getdents64, fd, buf, nbytes);
> -  if (ret != -1)
> -    return ret;
> +  static bool getdents64_supportted = true;
> +  if (atomic_load_relaxed (&getdents64_supportted))
> +    {
> +      ssize_t ret = INLINE_SYSCALL_CALL (getdents64, fd, buf, nbytes);
> +      if (ret >= 0 || errno != ENOSYS)
> +	return ret;
> +
> +      atomic_store_relaxed (&getdents64_supportted, false);
> +    }
>  #endif
>  
>    /* Unfortunately getdents64 was only wire-up for MIPS n64 on Linux 3.10.
> -     If syscall is not available it need to fallback to non-LFS one.  */
> +     If the syscall is not available it need to fallback to the non-LFS one.
> +     Also to avoid an unbounded allocation through VLA/alloca or malloc (which
> +     would make the syscall non async-signal-safe) it uses a limited buffer.
> +     This is sub-optimal for large NBYTES, however this is a fallback
> +     mechanism to emulate a syscall that kernel should provide.   */
>  
> +  enum { KBUF_SIZE = 1024 };
>    struct kernel_dirent
> -    {
> -      unsigned long d_ino;
> -      unsigned long d_off;
> -      unsigned short int d_reclen;
> -      char d_name[256];
> -    };
> -
> -  const size_t size_diff = (offsetof (struct dirent64, d_name)
> -			   - offsetof (struct kernel_dirent, d_name));
> -
> -  size_t red_nbytes = MIN (nbytes
> -			   - ((nbytes / (offsetof (struct dirent64, d_name)
> -					 + 14)) * size_diff),
> -			   nbytes - size_diff);
> -
> -  struct scratch_buffer tmpbuf;
> -  scratch_buffer_init (&tmpbuf);
> -  if (!scratch_buffer_set_array_size (&tmpbuf, red_nbytes, sizeof (uint8_t)))
> -    INLINE_SYSCALL_ERROR_RETURN_VALUE (ENOMEM);
> -
> -  struct kernel_dirent *skdp, *kdp;
> -  skdp = kdp = tmpbuf.data;
> -
> -  ssize_t retval = INLINE_SYSCALL_CALL (getdents, fd, kdp, red_nbytes);
> -  if (retval == -1)
> -    {
> -      scratch_buffer_free (&tmpbuf);
> -      return -1;
> -    }
> +  {
> +    unsigned long d_ino;
> +    unsigned long d_off;
> +    unsigned short int d_reclen;
> +    char d_name[1];
> +  } kbuf[KBUF_SIZE / sizeof (struct kernel_dirent)];
> +  size_t kbuf_size = nbytes < KBUF_SIZE ? nbytes : KBUF_SIZE;
>  
> -  off64_t last_offset = -1;
>    struct dirent64 *dp = (struct dirent64 *) buf;
> -  while ((char *) kdp < (char *) skdp + retval)
> +
> +  size_t nb = 0;
> +  off64_t last_offset = -1;
> +
> +  ssize_t r;
> +  while ((r = INLINE_SYSCALL_CALL (getdents, fd, kbuf, kbuf_size)) > 0)
>      {
> -      const size_t alignment = _Alignof (struct dirent64);
> -      /* Since kdp->d_reclen is already aligned for the kernel structure
> -	 this may compute a value that is bigger than necessary.  */
> -      size_t new_reclen = ((kdp->d_reclen + size_diff + alignment - 1)
> -			   & ~(alignment - 1));
> -      if ((char *) dp + new_reclen > buf + nbytes)
> -        {
> -	  /* Our heuristic failed.  We read too many entries.  Reset
> -	     the stream.  */
> -	  assert (last_offset != -1);
> -	  __lseek64 (fd, last_offset, SEEK_SET);
> -
> -	  if ((char *) dp == buf)
> +      struct kernel_dirent *skdp, *kdp;
> +      skdp = kdp = kbuf;
> +
> +      while ((char *) kdp < (char *) skdp + r)
> +	{
> +	  const size_t alignment = _Alignof (struct dirent64);
> +	  size_t new_reclen = ((kdp->d_reclen + alignment - 1)
> +			      & ~(alignment - 1));
> +	  if (nb + new_reclen > nbytes)
>  	    {
> -	      scratch_buffer_free (&tmpbuf);
> -	      return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
> +		/* The new entry will overflow the input buffer, rewind to
> +		   last obtained entry and return.  */
> +	       __lseek64 (fd, last_offset, SEEK_SET);
> +	       goto out;
>  	    }
> +	  nb += new_reclen;
>  
> -	  break;
> -	}
> -
> -      last_offset = kdp->d_off;
> -      dp->d_ino = kdp->d_ino;
> -      dp->d_off = kdp->d_off;
> -      dp->d_reclen = new_reclen;
> -      dp->d_type = *((char *) kdp + kdp->d_reclen - 1);
> -      memcpy (dp->d_name, kdp->d_name,
> -	      kdp->d_reclen - offsetof (struct kernel_dirent, d_name));
> +	  dp->d_ino = kdp->d_ino;
> +	  dp->d_off = last_offset = kdp->d_off;
> +	  dp->d_reclen = new_reclen;
> +	  dp->d_type = *((char *) kdp + kdp->d_reclen - 1);
> +	  memcpy (dp->d_name, kdp->d_name,
> +		  kdp->d_reclen - offsetof (struct kernel_dirent, d_name));
>  
> -      dp = (struct dirent64 *) ((char *) dp + new_reclen);
> -      kdp = (struct kernel_dirent *) (((char *) kdp) + kdp->d_reclen);
> +	  dp = (struct dirent64 *) ((char *) dp + new_reclen);
> +	  kdp = (struct kernel_dirent *) (((char *) kdp) + kdp->d_reclen);
> +	}
>      }
>  
> -  scratch_buffer_free (&tmpbuf);
> -  return (char *) dp - buf;
> +out:
> +  return (char *) dp - (char *) buf;
>  }
>  libc_hidden_def (__getdents64)
>  weak_alias (__getdents64, getdents64)
>