[PATCH] linux: mips: Fix getdents64 fallback on mips64-n32

[PATCH] linux: mips: Fix getdents64 fallback on mips64-n32

[Adhemerval Zanella]

GCC mainline shows the following error:

../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c: In function '__getdents64':
../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:121:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds]
  121 |       memcpy (((char *) dp + offsetof (struct dirent64, d_ino)),
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  122 |               KDP_MEMBER (kdp, d_ino), sizeof ((struct dirent64){0}.d_ino));
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:123:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds]
  123 |       memcpy (((char *) dp + offsetof (struct dirent64, d_off)),
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  124 |               KDP_MEMBER (kdp, d_off), sizeof ((struct dirent64){0}.d_off));
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The issue is due both d_ino and d_off fields for mips64-n32
kernel_dirent are 32-bits, while this is using memcpy to copy 64 bits
from it into the glibc dirent64.

The fix is to use a temporary variable to read the correct type
from kernel_dirent.

Checked with a build-many-glibcs.py for mips64el-linux-gnu and I
also checked the tst-getdents64 on mips64el 4.1.4 kernel with
and without fallback enabled (by manually setting the
getdents64_supported).
---
 .../unix/sysv/linux/mips/mips64/getdents64.c  | 31 ++++++++++++-------
 sysdeps/unix/sysv/linux/tst-getdents64.c      | 29 ++++++++++++++---
 2 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
index d18a5297dc..5b7597c99b 100644
--- a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
+++ b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
@@ -91,15 +91,18 @@ __getdents64 (int fd, void *buf, size_t nbytes)
   while ((char *) kdp < (char *) skdp + r)
     {
       /* This macro is used to avoid aliasing violation.  */
-#define KDP_MEMBER(src, member)			     			\
-    (__typeof__((struct kernel_dirent){0}.member) *)			\
-      memcpy (&((__typeof__((struct kernel_dirent){0}.member)){0}),	\
-	      ((char *)(src) + offsetof (struct kernel_dirent, member)),\
-	      sizeof ((struct kernel_dirent){0}.member))
+#define KDP_MEMBER(member)						     \
+      ({								     \
+	__typeof ((struct kernel_dirent){0}.member) kdp_tmp;		     \
+	memcpy (&kdp_tmp,						     \
+		((char *)(kdp) + offsetof (struct kernel_dirent, member)),   \
+		sizeof (kdp_tmp));					     \
+	kdp_tmp;							     \
+      })
 
       /* This is a conservative approximation, since some of size_diff might
 	 fit into the existing padding for alignment.  */
-      unsigned short int k_reclen = *KDP_MEMBER (kdp, d_reclen);
+      unsigned short int k_reclen = KDP_MEMBER (d_reclen);
       unsigned short int new_reclen = ALIGN_UP (k_reclen + size_diff,
 						_Alignof (struct dirent64));
       if (nb + new_reclen > nbytes)
@@ -118,11 +121,17 @@ __getdents64 (int fd, void *buf, size_t nbytes)
 	}
       nb += new_reclen;
 
-      memcpy (((char *) dp + offsetof (struct dirent64, d_ino)),
-	      KDP_MEMBER (kdp, d_ino), sizeof ((struct dirent64){0}.d_ino));
-      memcpy (((char *) dp + offsetof (struct dirent64, d_off)),
-	      KDP_MEMBER (kdp, d_off), sizeof ((struct dirent64){0}.d_off));
-      last_offset = *KDP_MEMBER (kdp, d_off);
+#define COPY_MEMBER(member)					     	     \
+      ({								     \
+	__typeof ((struct dirent64){0}.member) dp_tmp		     	     \
+	  = KDP_MEMBER (member);					     \
+	memcpy ((char *) dp + offsetof (struct dirent64, member),	     \
+		&dp_tmp, sizeof (dp_tmp));				     \
+      })
+
+      COPY_MEMBER (d_ino);
+      COPY_MEMBER (d_off);
+      last_offset = KDP_MEMBER (d_off);
       memcpy (((char *) dp + offsetof (struct dirent64, d_reclen)),
 	      &new_reclen, sizeof (new_reclen));
       dp->d_type = *((char *) kdp + k_reclen - 1);
diff --git a/sysdeps/unix/sysv/linux/tst-getdents64.c b/sysdeps/unix/sysv/linux/tst-getdents64.c
index 25b4755fb9..97857d22f3 100644
--- a/sysdeps/unix/sysv/linux/tst-getdents64.c
+++ b/sysdeps/unix/sysv/linux/tst-getdents64.c
@@ -76,8 +76,18 @@ large_buffer_checks (int fd)
     }
 }
 
-static int
-do_test (void)
+static void
+do_test_large_size (void)
+{
+  int fd = xopen (".", O_RDONLY | O_DIRECTORY, 0);
+  TEST_VERIFY (fd >= 0);
+  large_buffer_checks (fd);
+
+  xclose (fd);
+}
+
+static void
+do_test_by_size (size_t buffer_size)
 {
   /* The test compares the iteration order with readdir64.  */
   DIR *reference = opendir (".");
@@ -98,7 +108,7 @@ do_test (void)
              non-existing data.  */
           struct
           {
-            char buffer[1024];
+            char buffer[buffer_size];
             struct dirent64 pad;
           } data;
 
@@ -153,10 +163,19 @@ do_test (void)
       rewinddir (reference);
     }
 
-  large_buffer_checks (fd);
-
   xclose (fd);
   closedir (reference);
+}
+
+static int
+do_test (void)
+{
+  do_test_by_size (512);
+  do_test_by_size (1024);
+  do_test_by_size (4096);
+
+  do_test_large_size ();
+
   return 0;
 }
 
-- 
2.25.1

Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> GCC mainline shows the following error:
>
> ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c: In function '__getdents64':
> ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:121:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds]
>   121 |       memcpy (((char *) dp + offsetof (struct dirent64, d_ino)),
>       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   122 |               KDP_MEMBER (kdp, d_ino), sizeof ((struct dirent64){0}.d_ino));
>       |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:123:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds]
>   123 |       memcpy (((char *) dp + offsetof (struct dirent64, d_off)),
>       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   124 |               KDP_MEMBER (kdp, d_off), sizeof ((struct dirent64){0}.d_off));
>       |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> The issue is due both d_ino and d_off fields for mips64-n32
> kernel_dirent are 32-bits, while this is using memcpy to copy 64 bits
> from it into the glibc dirent64.
>
> The fix is to use a temporary variable to read the correct type
> from kernel_dirent.

I think I suggested to cut back on the macro magic and simply have
appropriately defined structs, with a sequence like this:

  memcpy to first temporary struct
  field-by-field assignment from first temporary struct to second struct
  memcpy from second temporary struct

Would that work?

(Sorry if my message got through and this suggestion was already
considered.)

Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32

[Andreas Schwab]

On Nov 16 2020, Adhemerval Zanella via Libc-alpha wrote:

> diff --git a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
> index d18a5297dc..5b7597c99b 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
> +++ b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
> @@ -91,15 +91,18 @@ __getdents64 (int fd, void *buf, size_t nbytes)
>    while ((char *) kdp < (char *) skdp + r)
>      {
>        /* This macro is used to avoid aliasing violation.  */
> -#define KDP_MEMBER(src, member)			     			\
> -    (__typeof__((struct kernel_dirent){0}.member) *)			\
> -      memcpy (&((__typeof__((struct kernel_dirent){0}.member)){0}),	\
> -	      ((char *)(src) + offsetof (struct kernel_dirent, member)),\
> -	      sizeof ((struct kernel_dirent){0}.member))
> +#define KDP_MEMBER(member)						     \
> +      ({								     \
> +	__typeof ((struct kernel_dirent){0}.member) kdp_tmp;		     \
> +	memcpy (&kdp_tmp,						     \
> +		((char *)(kdp) + offsetof (struct kernel_dirent, member)),   \
> +		sizeof (kdp_tmp));					     \
> +	kdp_tmp;							     \
> +      })

Why can't this just be kdp->member?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32

[Adhemerval Zanella]

On 16/11/2020 18:22, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> GCC mainline shows the following error:
>>
>> ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c: In function '__getdents64':
>> ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:121:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds]
>>   121 |       memcpy (((char *) dp + offsetof (struct dirent64, d_ino)),
>>       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>   122 |               KDP_MEMBER (kdp, d_ino), sizeof ((struct dirent64){0}.d_ino));
>>       |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:123:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds]
>>   123 |       memcpy (((char *) dp + offsetof (struct dirent64, d_off)),
>>       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>   124 |               KDP_MEMBER (kdp, d_off), sizeof ((struct dirent64){0}.d_off));
>>       |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> The issue is due both d_ino and d_off fields for mips64-n32
>> kernel_dirent are 32-bits, while this is using memcpy to copy 64 bits
>> from it into the glibc dirent64.
>>
>> The fix is to use a temporary variable to read the correct type
>> from kernel_dirent.
> 
> I think I suggested to cut back on the macro magic and simply have
> appropriately defined structs, with a sequence like this:
> 
>   memcpy to first temporary struct
>   field-by-field assignment from first temporary struct to second struct
>   memcpy from second temporary struct
> 
> Would that work?
> 
> (Sorry if my message got through and this suggestion was already
> considered.)

Would it be more in line of what you have suggested?

---

[PATCH] linux: mips: Fix getdents64 fallback on mips64-n32

GCC mainline shows the following error:

../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c: In function '__getdents64':
../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:121:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds]
  121 |       memcpy (((char *) dp + offsetof (struct dirent64, d_ino)),
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  122 |               KDP_MEMBER (kdp, d_ino), sizeof ((struct dirent64){0}.d_ino));
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:123:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds]
  123 |       memcpy (((char *) dp + offsetof (struct dirent64, d_off)),
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  124 |               KDP_MEMBER (kdp, d_off), sizeof ((struct dirent64){0}.d_off));
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The issue is due both d_ino and d_off fields for mips64-n32
kernel_dirent are 32-bits, while this is using memcpy to copy 64 bits
from it into the glibc dirent64.

The fix is to use a temporary buffer to read the correct type
from kernel_dirent.

Checked with a build-many-glibcs.py for mips64el-linux-gnu and I
also checked the tst-getdents64 on mips64el 4.1.4 kernel with
and without fallback enabled (by manually setting the
getdents64_supported).
---
 .../unix/sysv/linux/mips/mips64/getdents64.c  | 37 +++++++++----------
 sysdeps/unix/sysv/linux/tst-getdents64.c      | 29 ++++++++++++---
 2 files changed, 41 insertions(+), 25 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
index d18a5297dc..368128244e 100644
--- a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
+++ b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
@@ -90,17 +90,14 @@ __getdents64 (int fd, void *buf, size_t nbytes)
 
   while ((char *) kdp < (char *) skdp + r)
     {
-      /* This macro is used to avoid aliasing violation.  */
-#define KDP_MEMBER(src, member)			     			\
-    (__typeof__((struct kernel_dirent){0}.member) *)			\
-      memcpy (&((__typeof__((struct kernel_dirent){0}.member)){0}),	\
-	      ((char *)(src) + offsetof (struct kernel_dirent, member)),\
-	      sizeof ((struct kernel_dirent){0}.member))
-
       /* This is a conservative approximation, since some of size_diff might
 	 fit into the existing padding for alignment.  */
-      unsigned short int k_reclen = *KDP_MEMBER (kdp, d_reclen);
-      unsigned short int new_reclen = ALIGN_UP (k_reclen + size_diff,
+
+      /* Obtain the d_ino, d_off, and d_reclen from kernel filled buffer.  */
+      struct kernel_dirent kdirent;
+      memcpy (&kdirent, kdp, sizeof (struct kernel_dirent));
+
+      unsigned short int new_reclen = ALIGN_UP (kdirent.d_reclen + size_diff,
 						_Alignof (struct dirent64));
       if (nb + new_reclen > nbytes)
 	{
@@ -118,19 +115,19 @@ __getdents64 (int fd, void *buf, size_t nbytes)
 	}
       nb += new_reclen;
 
-      memcpy (((char *) dp + offsetof (struct dirent64, d_ino)),
-	      KDP_MEMBER (kdp, d_ino), sizeof ((struct dirent64){0}.d_ino));
-      memcpy (((char *) dp + offsetof (struct dirent64, d_off)),
-	      KDP_MEMBER (kdp, d_off), sizeof ((struct dirent64){0}.d_off));
-      last_offset = *KDP_MEMBER (kdp, d_off);
-      memcpy (((char *) dp + offsetof (struct dirent64, d_reclen)),
-	      &new_reclen, sizeof (new_reclen));
-      dp->d_type = *((char *) kdp + k_reclen - 1);
-      memcpy (dp->d_name, kdp->d_name,
-	      k_reclen - offsetof (struct kernel_dirent, d_name));
+      struct dirent64 d64;
+      d64.d_ino = kdirent.d_ino;
+      d64.d_off = kdirent.d_off;
+      d64.d_reclen = new_reclen;
+      d64.d_type = *((char *) kdp + kdirent.d_reclen - 1);
+      memcpy (d64.d_name, kdp->d_name,
+	      kdirent.d_reclen - offsetof (struct kernel_dirent, d_name));
+
+      memcpy (dp, &d64, new_reclen);
+      last_offset = kdirent.d_off;
 
       dp = (struct dirent64 *) ((char *) dp + new_reclen);
-      kdp = (struct kernel_dirent *) (((char *) kdp) + k_reclen);
+      kdp = (struct kernel_dirent *) (((char *) kdp) + kdirent.d_reclen);
     }
 
   return (char *) dp - (char *) buf;
diff --git a/sysdeps/unix/sysv/linux/tst-getdents64.c b/sysdeps/unix/sysv/linux/tst-getdents64.c
index 25b4755fb9..97857d22f3 100644
--- a/sysdeps/unix/sysv/linux/tst-getdents64.c
+++ b/sysdeps/unix/sysv/linux/tst-getdents64.c
@@ -76,8 +76,18 @@ large_buffer_checks (int fd)
     }
 }
 
-static int
-do_test (void)
+static void
+do_test_large_size (void)
+{
+  int fd = xopen (".", O_RDONLY | O_DIRECTORY, 0);
+  TEST_VERIFY (fd >= 0);
+  large_buffer_checks (fd);
+
+  xclose (fd);
+}
+
+static void
+do_test_by_size (size_t buffer_size)
 {
   /* The test compares the iteration order with readdir64.  */
   DIR *reference = opendir (".");
@@ -98,7 +108,7 @@ do_test (void)
              non-existing data.  */
           struct
           {
-            char buffer[1024];
+            char buffer[buffer_size];
             struct dirent64 pad;
           } data;
 
@@ -153,10 +163,19 @@ do_test (void)
       rewinddir (reference);
     }
 
-  large_buffer_checks (fd);
-
   xclose (fd);
   closedir (reference);
+}
+
+static int
+do_test (void)
+{
+  do_test_by_size (512);
+  do_test_by_size (1024);
+  do_test_by_size (4096);
+
+  do_test_large_size ();
+
   return 0;
 }
 
-- 
2.25.1

Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32

[Andreas Schwab]

On Nov 17 2020, Adhemerval Zanella wrote:

> diff --git a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
> index d18a5297dc..368128244e 100644
> --- a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
> +++ b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
> @@ -90,17 +90,14 @@ __getdents64 (int fd, void *buf, size_t nbytes)
>  
>    while ((char *) kdp < (char *) skdp + r)
>      {
> -      /* This macro is used to avoid aliasing violation.  */
> -#define KDP_MEMBER(src, member)			     			\
> -    (__typeof__((struct kernel_dirent){0}.member) *)			\
> -      memcpy (&((__typeof__((struct kernel_dirent){0}.member)){0}),	\
> -	      ((char *)(src) + offsetof (struct kernel_dirent, member)),\
> -	      sizeof ((struct kernel_dirent){0}.member))
> -
>        /* This is a conservative approximation, since some of size_diff might
>  	 fit into the existing padding for alignment.  */
> -      unsigned short int k_reclen = *KDP_MEMBER (kdp, d_reclen);
> -      unsigned short int new_reclen = ALIGN_UP (k_reclen + size_diff,
> +
> +      /* Obtain the d_ino, d_off, and d_reclen from kernel filled buffer.  */
> +      struct kernel_dirent kdirent;
> +      memcpy (&kdirent, kdp, sizeof (struct kernel_dirent));

How good is the compiler at eliding the memcpy?  What would change if
kdp would just be void *?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32

[Adhemerval Zanella]

On 17/11/2020 10:38, Andreas Schwab wrote:
> On Nov 17 2020, Adhemerval Zanella wrote:
> 
>> diff --git a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
>> index d18a5297dc..368128244e 100644
>> --- a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
>> +++ b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
>> @@ -90,17 +90,14 @@ __getdents64 (int fd, void *buf, size_t nbytes)
>>  
>>    while ((char *) kdp < (char *) skdp + r)
>>      {
>> -      /* This macro is used to avoid aliasing violation.  */
>> -#define KDP_MEMBER(src, member)			     			\
>> -    (__typeof__((struct kernel_dirent){0}.member) *)			\
>> -      memcpy (&((__typeof__((struct kernel_dirent){0}.member)){0}),	\
>> -	      ((char *)(src) + offsetof (struct kernel_dirent, member)),\
>> -	      sizeof ((struct kernel_dirent){0}.member))
>> -
>>        /* This is a conservative approximation, since some of size_diff might
>>  	 fit into the existing padding for alignment.  */
>> -      unsigned short int k_reclen = *KDP_MEMBER (kdp, d_reclen);
>> -      unsigned short int new_reclen = ALIGN_UP (k_reclen + size_diff,
>> +
>> +      /* Obtain the d_ino, d_off, and d_reclen from kernel filled buffer.  */
>> +      struct kernel_dirent kdirent;
>> +      memcpy (&kdirent, kdp, sizeof (struct kernel_dirent));
> 
> How good is the compiler at eliding the memcpy?  What would change if
> kdp would just be void *?

Not that good, I see a large stack usage (1456 vs 1152 from my initial
version) and some more memory load/store memory instructions.  The
kdp type change does not change the code generation.

I don't have a strong preference here, in any case this should be
used only as fallback on older kernels.

Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> Not that good, I see a large stack usage (1456 vs 1152 from my initial
> version) and some more memory load/store memory instructions.  The
> kdp type change does not change the code generation.

That's why I suggested to use two separately defined struct types for
this.

Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32

[Adhemerval Zanella]

On 17/11/2020 14:51, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> Not that good, I see a large stack usage (1456 vs 1152 from my initial
>> version) and some more memory load/store memory instructions.  The
>> kdp type change does not change the code generation.
> 
> That's why I suggested to use two separately defined struct types for
> this.
> 

How different than my second attempt [1] is your suggestion? I tried
to follow your description on this one.

[1] https://sourceware.org/pipermail/libc-alpha/2020-November/119688.html

Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> Would it be more in line of what you have suggested?

Yes, thanks.
>    while ((char *) kdp < (char *) skdp + r)
>      {
> +
> +      /* Obtain the d_ino, d_off, and d_reclen from kernel filled buffer.  */
> +      struct kernel_dirent kdirent;
> +      memcpy (&kdirent, kdp, sizeof (struct kernel_dirent));

I believe this should use offsetof (struct kernel_dirent, d_name)
for the size of the copy.  Technically, the padding at the end of the
struct may not be present in the buffer.

> @@ -118,19 +115,19 @@ __getdents64 (int fd, void *buf, size_t nbytes)
>  	}
>        nb += new_reclen;
>  
> +      struct dirent64 d64;
> +      d64.d_ino = kdirent.d_ino;
> +      d64.d_off = kdirent.d_off;
> +      d64.d_reclen = new_reclen;
> +      d64.d_type = *((char *) kdp + kdirent.d_reclen - 1);
> +      memcpy (d64.d_name, kdp->d_name,
> +	      kdirent.d_reclen - offsetof (struct kernel_dirent, d_name));
> +
> +      memcpy (dp, &d64, new_reclen);
> +      last_offset = kdirent.d_off;
>  
>        dp = (struct dirent64 *) ((char *) dp + new_reclen);
> +      kdp = (struct kernel_dirent *) (((char *) kdp) + kdirent.d_reclen);
>      }

I think the first memcpy is wrong: It has to go into the result buffer
because only that will be large enough.  256 bytes for d_name may not be
enough.  The second memcpy should only copy the header (before d_name).

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32

[Adhemerval Zanella]

On 22/01/2021 09:49, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> Would it be more in line of what you have suggested?
> 
> Yes, thanks.
>>    while ((char *) kdp < (char *) skdp + r)
>>      {
>> +
>> +      /* Obtain the d_ino, d_off, and d_reclen from kernel filled buffer.  */
>> +      struct kernel_dirent kdirent;
>> +      memcpy (&kdirent, kdp, sizeof (struct kernel_dirent));
> 
> I believe this should use offsetof (struct kernel_dirent, d_name)
> for the size of the copy.  Technically, the padding at the end of the
> struct may not be present in the buffer.

Right, I was working the assumption that kernel always returned at least
one byte do d_name ('\0').

> 
>> @@ -118,19 +115,19 @@ __getdents64 (int fd, void *buf, size_t nbytes)
>>  	}
>>        nb += new_reclen;
>>  
>> +      struct dirent64 d64;
>> +      d64.d_ino = kdirent.d_ino;
>> +      d64.d_off = kdirent.d_off;
>> +      d64.d_reclen = new_reclen;
>> +      d64.d_type = *((char *) kdp + kdirent.d_reclen - 1);
>> +      memcpy (d64.d_name, kdp->d_name,
>> +	      kdirent.d_reclen - offsetof (struct kernel_dirent, d_name));
>> +
>> +      memcpy (dp, &d64, new_reclen);
>> +      last_offset = kdirent.d_off;
>>  
>>        dp = (struct dirent64 *) ((char *) dp + new_reclen);
>> +      kdp = (struct kernel_dirent *) (((char *) kdp) + kdirent.d_reclen);
>>      }
> 
> I think the first memcpy is wrong: It has to go into the result buffer
> because only that will be large enough.  256 bytes for d_name may not be
> enough.  The second memcpy should only copy the header (before d_name).
> 

Indeed, I fixed on the patch below.  The tst-getdents64 does pass on 
mips64 and mips64-n32 on gccfarm machine (I explicit disabled the
getdents64 call for the test).

--

[PATCH] linux: mips: Fix getdents64 fallback on mips64-n32

GCC mainline shows the following error:

../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c: In function '__getdents64':
../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:121:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds]
  121 |       memcpy (((char *) dp + offsetof (struct dirent64, d_ino)),
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  122 |               KDP_MEMBER (kdp, d_ino), sizeof ((struct dirent64){0}.d_ino));
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../sysdeps/unix/sysv/linux/mips/mips64/getdents64.c:123:7: error: 'memcpy' forming offset [4, 7] is out of the bounds [0, 4] [-Werror=array-bounds]
  123 |       memcpy (((char *) dp + offsetof (struct dirent64, d_off)),
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  124 |               KDP_MEMBER (kdp, d_off), sizeof ((struct dirent64){0}.d_off));
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The issue is due both d_ino and d_off fields for mips64-n32
kernel_dirent are 32-bits, while this is using memcpy to copy 64 bits
from it into the glibc dirent64.

The fix is to use a temporary buffer to read the correct type
from kernel_dirent.

Checked with a build-many-glibcs.py for mips64el-linux-gnu and I
also checked the tst-getdents64 on mips64el 4.1.4 kernel with
and without fallback enabled (by manually setting the
getdents64_supported).
---
 .../unix/sysv/linux/mips/mips64/getdents64.c  | 37 +++++++++----------
 sysdeps/unix/sysv/linux/tst-getdents64.c      | 29 ++++++++++++---
 2 files changed, 42 insertions(+), 24 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
index a218f68104..ed6589ab94 100644
--- a/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
+++ b/sysdeps/unix/sysv/linux/mips/mips64/getdents64.c
@@ -90,17 +90,14 @@ __getdents64 (int fd, void *buf, size_t nbytes)
 
   while ((char *) kdp < (char *) skdp + r)
     {
-      /* This macro is used to avoid aliasing violation.  */
-#define KDP_MEMBER(src, member)			     			\
-    (__typeof__((struct kernel_dirent){0}.member) *)			\
-      memcpy (&((__typeof__((struct kernel_dirent){0}.member)){0}),	\
-	      ((char *)(src) + offsetof (struct kernel_dirent, member)),\
-	      sizeof ((struct kernel_dirent){0}.member))
-
       /* This is a conservative approximation, since some of size_diff might
 	 fit into the existing padding for alignment.  */
-      unsigned short int k_reclen = *KDP_MEMBER (kdp, d_reclen);
-      unsigned short int new_reclen = ALIGN_UP (k_reclen + size_diff,
+
+      /* Obtain the d_ino, d_off, and d_reclen from kernel filled buffer.  */
+      struct kernel_dirent kdirent;
+      memcpy (&kdirent, kdp, offsetof (struct kernel_dirent, d_name));
+
+      unsigned short int new_reclen = ALIGN_UP (kdirent.d_reclen + size_diff,
 						_Alignof (struct dirent64));
       if (nb + new_reclen > nbytes)
 	{
@@ -118,19 +115,21 @@ __getdents64 (int fd, void *buf, size_t nbytes)
 	}
       nb += new_reclen;
 
-      memcpy (((char *) dp + offsetof (struct dirent64, d_ino)),
-	      KDP_MEMBER (kdp, d_ino), sizeof ((struct dirent64){0}.d_ino));
-      memcpy (((char *) dp + offsetof (struct dirent64, d_off)),
-	      KDP_MEMBER (kdp, d_off), sizeof ((struct dirent64){0}.d_off));
-      last_offset = *KDP_MEMBER (kdp, d_off);
-      memcpy (((char *) dp + offsetof (struct dirent64, d_reclen)),
-	      &new_reclen, sizeof (new_reclen));
-      dp->d_type = *((char *) kdp + k_reclen - 1);
+      struct dirent64 d64;
+      d64.d_ino = kdirent.d_ino;
+      d64.d_off = kdirent.d_off;
+      d64.d_reclen = new_reclen;
+      d64.d_type = *((char *) kdp + kdirent.d_reclen - 1);
+      /* First copy only the header.  */
+      memcpy (dp, &d64, offsetof (struct dirent64, d_name));
+      /* And then the d_name.  */
       memcpy (dp->d_name, kdp->d_name,
-	      k_reclen - offsetof (struct kernel_dirent, d_name));
+	      kdirent.d_reclen - offsetof (struct kernel_dirent, d_name));
+
+      last_offset = kdirent.d_off;
 
       dp = (struct dirent64 *) ((char *) dp + new_reclen);
-      kdp = (struct kernel_dirent *) (((char *) kdp) + k_reclen);
+      kdp = (struct kernel_dirent *) (((char *) kdp) + kdirent.d_reclen);
     }
 
   return (char *) dp - (char *) buf;
diff --git a/sysdeps/unix/sysv/linux/tst-getdents64.c b/sysdeps/unix/sysv/linux/tst-getdents64.c
index 379ecbbcc6..691444d56e 100644
--- a/sysdeps/unix/sysv/linux/tst-getdents64.c
+++ b/sysdeps/unix/sysv/linux/tst-getdents64.c
@@ -76,8 +76,18 @@ large_buffer_checks (int fd)
     }
 }
 
-static int
-do_test (void)
+static void
+do_test_large_size (void)
+{
+  int fd = xopen (".", O_RDONLY | O_DIRECTORY, 0);
+  TEST_VERIFY (fd >= 0);
+  large_buffer_checks (fd);
+
+  xclose (fd);
+}
+
+static void
+do_test_by_size (size_t buffer_size)
 {
   /* The test compares the iteration order with readdir64.  */
   DIR *reference = opendir (".");
@@ -98,7 +108,7 @@ do_test (void)
              non-existing data.  */
           struct
           {
-            char buffer[1024];
+            char buffer[buffer_size];
             struct dirent64 pad;
           } data;
 
@@ -153,10 +163,19 @@ do_test (void)
       rewinddir (reference);
     }
 
-  large_buffer_checks (fd);
-
   xclose (fd);
   closedir (reference);
+}
+
+static int
+do_test (void)
+{
+  do_test_by_size (512);
+  do_test_by_size (1024);
+  do_test_by_size (4096);
+
+  do_test_large_size ();
+
   return 0;
 }
 
-- 
2.25.1

Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32

[Florian Weimer]

* Adhemerval Zanella:

> Indeed, I fixed on the patch below.  The tst-getdents64 does pass on 
> mips64 and mips64-n32 on gccfarm machine (I explicit disabled the
> getdents64 call for the test).

This version looks okay to me.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill