Message ID | 20201116211743.2228063-1-adhemerval.zanella@linaro.org |
---|---|
State | Committed |
Headers |
Return-Path: <libc-alpha-bounces@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C5B953833022; Mon, 16 Nov 2020 21:18:02 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C5B953833022 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1605561482; bh=xlelLk7kAqc5hzys3KQdgH811aoamTEa2wgixJ7ZHbs=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=rbSBucaGfz5dG8G62Jt4eV8WI3tigS0qVFjkJ4jh1BYILNYyjONrDMNCqNfBHaXDq p0nwVVySn9dV5D4XuHwDEyZHUkW0sa8CeXEwsF5dBQy1NeTYO4jpUCWlVKSI745DWd sYEPkm1Yg7V7Yprwr63t72GmLKe3WoberuqzWpTs= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-qt1-x844.google.com (mail-qt1-x844.google.com [IPv6:2607:f8b0:4864:20::844]) by sourceware.org (Postfix) with ESMTPS id 8EC293833022 for <libc-alpha@sourceware.org>; Mon, 16 Nov 2020 21:17:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8EC293833022 Received: by mail-qt1-x844.google.com with SMTP id 3so14181801qtx.3 for <libc-alpha@sourceware.org>; Mon, 16 Nov 2020 13:17:48 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id:mime-version :content-transfer-encoding; bh=xlelLk7kAqc5hzys3KQdgH811aoamTEa2wgixJ7ZHbs=; b=p56FZ8K6iatG8CAJv3xjiD6mU3UfEy1Y/7Mhbhd6z9BqbCBzyNkgCKWqrBQAZTRfkh QdDGwPrVFz+H5PtcaRuU9i3C/CTB+wWLJ3tHhYSr8lba1DGYnC9E3vns0vz0oShjyfen tpRm43Lly4pJlE++y7PtHzPAnSM6QzLGWm8BqvbdpEJO3Q13ZcKNVH+Cc0k+Xr/xLmg/ ashAzs02laDE8CDvAtYfOeFLLTMX7hSMnuh1RMFsx/yPS2L7TKJKa8x6Jgnxj9VUneFI se3bjGikQyCNYQH8l6RDkloRgZrIT9ieKqANay77prS52SUBw75/Gk5CvC3Dq7HHefUB uSSg== X-Gm-Message-State: AOAM532+MyGYTfeTERRV9Drncjbp6CxdKNj11LbxU7//kNiYfYXP7jbL 6UuMhkTw8vcZf6PGEI0YB/khc+3190N8uA== X-Google-Smtp-Source: ABdhPJxiZH+ba9pC20OO3h7FFnMFkdBicQsZWKly2wRid+DVZTackW8QIIS4klx3yXuEIxFI8wvOZQ== X-Received: by 2002:aed:2662:: with SMTP id z89mr16307414qtc.70.1605561467850; Mon, 16 Nov 2020 13:17:47 -0800 (PST) Received: from localhost.localdomain ([177.194.48.209]) by smtp.googlemail.com with ESMTPSA id s71sm6752946qka.102.2020.11.16.13.17.46 for <libc-alpha@sourceware.org> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 16 Nov 2020 13:17:47 -0800 (PST) To: libc-alpha@sourceware.org Subject: [PATCH] linux: mips: Fix getdents64 fallback on mips64-n32 Date: Mon, 16 Nov 2020 18:17:43 -0300 Message-Id: <20201116211743.2228063-1-adhemerval.zanella@linaro.org> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-13.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_ASCII_DIVIDERS, KAM_NUMSUBJECT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list <libc-alpha.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> From: Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> Reply-To: Adhemerval Zanella <adhemerval.zanella@linaro.org> Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" <libc-alpha-bounces@sourceware.org> |
Series |
linux: mips: Fix getdents64 fallback on mips64-n32
|
|
Commit Message
Adhemerval Zanella Netto
Nov. 16, 2020, 9:17 p.m. UTC
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(-)
Comments
* 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.)
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.
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; }
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.
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.
* 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.
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
* 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
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; }
* 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
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; }