* [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32 @ 2020-11-16 21:17 Adhemerval Zanella 2020-11-16 21:22 ` Florian Weimer 2020-11-16 21:40 ` Andreas Schwab 0 siblings, 2 replies; 11+ messages in thread From: Adhemerval Zanella @ 2020-11-16 21:17 UTC (permalink / raw) To: 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. 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32 2020-11-16 21:17 [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32 Adhemerval Zanella @ 2020-11-16 21:22 ` Florian Weimer 2020-11-17 13:19 ` Adhemerval Zanella 2020-11-16 21:40 ` Andreas Schwab 1 sibling, 1 reply; 11+ messages in thread From: Florian Weimer @ 2020-11-16 21:22 UTC (permalink / raw) To: Adhemerval Zanella via Libc-alpha * 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.) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32 2020-11-16 21:22 ` Florian Weimer @ 2020-11-17 13:19 ` Adhemerval Zanella 2020-11-17 13:38 ` Andreas Schwab 2021-01-22 12:49 ` Florian Weimer 0 siblings, 2 replies; 11+ messages in thread From: Adhemerval Zanella @ 2020-11-17 13:19 UTC (permalink / raw) To: Florian Weimer, Adhemerval Zanella via Libc-alpha, Andreas Schwab 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32 2020-11-17 13:19 ` Adhemerval Zanella @ 2020-11-17 13:38 ` Andreas Schwab 2020-11-17 17:37 ` Adhemerval Zanella 2021-01-22 12:49 ` Florian Weimer 1 sibling, 1 reply; 11+ messages in thread From: Andreas Schwab @ 2020-11-17 13:38 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: Florian Weimer, Adhemerval Zanella via Libc-alpha 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." ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32 2020-11-17 13:38 ` Andreas Schwab @ 2020-11-17 17:37 ` Adhemerval Zanella 2020-11-17 17:51 ` Florian Weimer 0 siblings, 1 reply; 11+ messages in thread From: Adhemerval Zanella @ 2020-11-17 17:37 UTC (permalink / raw) To: Andreas Schwab; +Cc: Florian Weimer, Adhemerval Zanella via Libc-alpha 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32 2020-11-17 17:37 ` Adhemerval Zanella @ 2020-11-17 17:51 ` Florian Weimer 2020-11-17 18:08 ` Adhemerval Zanella 0 siblings, 1 reply; 11+ messages in thread From: Florian Weimer @ 2020-11-17 17:51 UTC (permalink / raw) To: Adhemerval Zanella via Libc-alpha; +Cc: Andreas Schwab, Adhemerval Zanella * 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32 2020-11-17 17:51 ` Florian Weimer @ 2020-11-17 18:08 ` Adhemerval Zanella 0 siblings, 0 replies; 11+ messages in thread From: Adhemerval Zanella @ 2020-11-17 18:08 UTC (permalink / raw) To: Florian Weimer, Adhemerval Zanella via Libc-alpha; +Cc: Andreas Schwab 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32 2020-11-17 13:19 ` Adhemerval Zanella 2020-11-17 13:38 ` Andreas Schwab @ 2021-01-22 12:49 ` Florian Weimer 2021-01-22 14:20 ` Adhemerval Zanella 1 sibling, 1 reply; 11+ messages in thread From: Florian Weimer @ 2021-01-22 12:49 UTC (permalink / raw) To: Adhemerval Zanella via Libc-alpha Cc: Florian Weimer, Andreas Schwab, Adhemerval Zanella * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32 2021-01-22 12:49 ` Florian Weimer @ 2021-01-22 14:20 ` Adhemerval Zanella 2021-01-22 16:02 ` Florian Weimer 0 siblings, 1 reply; 11+ messages in thread From: Adhemerval Zanella @ 2021-01-22 14:20 UTC (permalink / raw) To: Florian Weimer, Adhemerval Zanella via Libc-alpha Cc: Florian Weimer, Andreas Schwab 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32 2021-01-22 14:20 ` Adhemerval Zanella @ 2021-01-22 16:02 ` Florian Weimer 0 siblings, 0 replies; 11+ messages in thread From: Florian Weimer @ 2021-01-22 16:02 UTC (permalink / raw) To: Adhemerval Zanella Cc: Adhemerval Zanella via Libc-alpha, Florian Weimer, Andreas Schwab * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32 2020-11-16 21:17 [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32 Adhemerval Zanella 2020-11-16 21:22 ` Florian Weimer @ 2020-11-16 21:40 ` Andreas Schwab 1 sibling, 0 replies; 11+ messages in thread From: Andreas Schwab @ 2020-11-16 21:40 UTC (permalink / raw) To: Adhemerval Zanella via Libc-alpha 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." ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-01-22 16:03 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-16 21:17 [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32 Adhemerval Zanella 2020-11-16 21:22 ` Florian Weimer 2020-11-17 13:19 ` Adhemerval Zanella 2020-11-17 13:38 ` Andreas Schwab 2020-11-17 17:37 ` Adhemerval Zanella 2020-11-17 17:51 ` Florian Weimer 2020-11-17 18:08 ` Adhemerval Zanella 2021-01-22 12:49 ` Florian Weimer 2021-01-22 14:20 ` Adhemerval Zanella 2021-01-22 16:02 ` Florian Weimer 2020-11-16 21:40 ` Andreas Schwab
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 read-only IMAP folder(s) and NNTP newsgroup(s).