From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x433.google.com (mail-pf1-x433.google.com [IPv6:2607:f8b0:4864:20::433]) by sourceware.org (Postfix) with ESMTPS id 4FECF3858C39 for ; Fri, 14 Jan 2022 21:55:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4FECF3858C39 Received: by mail-pf1-x433.google.com with SMTP id m1so3930827pfk.8 for ; Fri, 14 Jan 2022 13:55:30 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=7hmuSJfPwrOwltM5GjkqZBIvn1/NcGY+b4lmIKRnyE0=; b=xDPD/Pm9Lz59C6sI0MmwWWcn75lB79FO2FPjZ9pXP6vBpmwT/wDGz/+zBh213f9Dtk ykOSykHAPHt0LCnjnjTTIPh83UXAkdFN7X8lMrzOJ2Gm267BmJuZm9pr0y/2+UDt306P b6fsHhZnO/IAbyFoPHnhR6p9p5bg4wm7VqKrhGYpP6zQ2k0uA2eV+hjx53/iYbMvRpiz fXFEBtp3UF9jXanRC394xPaJBm9D4H66D3vzbDaX7igukwOeJAJZ5j+DShIyMpioLGCv 6j5aSjf/vLw5TC66zxIZY5SrXNAEXxttvXD6KBKvpS5j4v1Re6AmtuioISqPV21v/o7K EGrg== X-Gm-Message-State: AOAM533rrd2M3M5IQF1plGSNKj+kxi93f647ADItOpGjTeLKLWcPetx/ Zrv9TDYQeA0f9ALZo6BNNsWSr2Gl9jc= X-Google-Smtp-Source: ABdhPJx8mf35wYNo7ZW3a/G04s4IYGuv8joy4VPar66cwE0xaxRV8euSI/Z8Pc8i85y9HZ86AhERqg== X-Received: by 2002:a05:6a00:230d:b0:49f:b8ad:ae23 with SMTP id h13-20020a056a00230d00b0049fb8adae23mr10805264pfh.80.1642197329286; Fri, 14 Jan 2022 13:55:29 -0800 (PST) Received: from gnu-tgl-3.localdomain ([172.58.35.133]) by smtp.gmail.com with ESMTPSA id 10sm6810146pjc.6.2022.01.14.13.55.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 14 Jan 2022 13:55:28 -0800 (PST) Received: by gnu-tgl-3.localdomain (Postfix, from userid 1000) id C7217C0719; Fri, 14 Jan 2022 13:55:27 -0800 (PST) Date: Fri, 14 Jan 2022 13:55:27 -0800 From: "H.J. Lu" To: Alan Modra Cc: Binutils Subject: [PATCH] ld: Rewrite lang_size_relro_segment_1 Message-ID: References: <20220111021241.1937265-1-hjl.tools@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-3028.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_NUMSUBJECT, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: binutils@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Binutils mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 14 Jan 2022 21:55:32 -0000 On Fri, Jan 14, 2022 at 06:42:16PM +1030, Alan Modra wrote: > On Thu, Jan 13, 2022 at 05:19:22AM -0800, H.J. Lu wrote: > > On Thu, Jan 13, 2022 at 4:52 AM Alan Modra wrote: > > > > > > On Mon, Jan 10, 2022 at 06:12:41PM -0800, H.J. Lu via Binutils wrote: > > > > The existing RELRO scheme may leave a 1-page gap before the RELRO segment > > > > and align the end of the RELRO segment to the page size: > > > > > > > > [18] .eh_frame PROGBITS 408fa0 008fa0 005e80 00 A 0 0 8 > > > > [19] .init_array INIT_ARRAY 410de0 00fde0 000008 08 WA 0 0 8 > > > > [20] .fini_array FINI_ARRAY 410de8 00fde8 000008 08 WA 0 0 8 > > > > [21] .dynamic DYNAMIC 410df0 00fdf0 000200 10 WA 7 0 8 > > > > [22] .got PROGBITS 410ff0 00fff0 000010 08 WA 0 0 8 > > > > [23] .got.plt PROGBITS 411000 010000 000048 08 WA 0 0 8 > > > > > > Do you know what is going wrong with the relro section layout for this > > > to occur? > > > > > > In this particular case, the end of the read-only segment is at > > > 0x408fa0 + 0x5e80 = 0x40ee20. My guess is that layout of the > > > following rw sections starts on the next page plus current offset > > > within page, the standard choice to minimise disk pages. ie. We start > > > at 0x40fe20. Then discover that this puts .got.plt at 0x40fe20 + 8 + > > > 8 + 0x200 + 0x10 = 0x40f040. However, we want this to be on a page > > > boundary so that the relro segment ends on a page boundary. So we > > > bump 0x40f040 up to 0x411000 and calculate backwards from there to > > > arrive at .init_array with a vma of 0x410de0. Resulting in the > > > 0x40f000 page being unused. > > > > > > If instead we start relro layout on the next page, we'd start laying > > > out at 0x40f000 rather than 0x40fe20. I think that would be the > > > > But if the prior ro section size is greater than one page, we want > > to subtract 1 page: > > > > + /* If the preceding section size is greater than the maximum > > + page size, subtract the maximum page size. Otherwise, > > + align the RELRO segment to the maximum page size. */ > > + if (prev_sec->size > seg->maxpagesize) > > + { > > + desired_end -= seg->maxpagesize; > > + relro_end -= seg->maxpagesize; > > + } > > + else > > + { > > + desired_end &= ~(seg->maxpagesize - 1); > > + relro_end &= ~(seg->maxpagesize - 1); > > + } > > + } > > The above code is the major reason why I took a dislike to your > patch. I fully expected you would have rev 2 posted. Why does > anything depend on the size of a previous section?? That just doesn't > make sense. And please don't write comments that just say what the > code is doing. Any half competent programmer can see what the code is > doing. Write comments that say *why*. > > > > correct thing to do rather than fixing up afterwards as your patch > > > does. > > > > > > > I am checking in my patch as Nick has approved it. There is a > > possibility that one 1-page gap may be needed to maintain > > the section alignment. My patch checks it and includes many > > testcase adjustments to remove one 1-page gap. Can you > > update the RELRO segment algorithm to make my fixup > > unnecessary? > > Yes, I do think that is possible and desirable. My thinking last > night was that it ought to be easy too. A one-liner even. Silly me, > a little experimentation soon showed up a fail of the pr18176 > testcase, demonstrating that there are cases we can save disk space > without causing gaps. > Now I have time to rewrite lang_size_relro_segment_1. Is it OK for master? Thanks. H.J. --- 1. Find the first section in the relro segment. 2. Find the first non-empty preceding section. 3. Don't add the 1-page gap before the relro segment if the maximum page size >= the maximum section alignment. 4. Align the relro segment if there is a 1-page gap before the relro segment. PR ld/28743 * ldlang.c (lang_size_relro_segment_1): Rewrite. --- ld/ldlang.c | 103 ++++++++++++++++++++++++---------------------------- 1 file changed, 48 insertions(+), 55 deletions(-) diff --git a/ld/ldlang.c b/ld/ldlang.c index 48e40828634..90b4e868a89 100644 --- a/ld/ldlang.c +++ b/ld/ldlang.c @@ -6370,9 +6370,8 @@ lang_size_segment (seg_align_type *seg) static bfd_vma lang_size_relro_segment_1 (seg_align_type *seg) { - bfd_vma relro_end, desired_end; - asection *sec, *prev_sec = NULL; - bool remove_page_gap = false; + bfd_vma relro_end, desired_end, aligned_start = 0; + asection *sec, *prev_sec = NULL, *relro_sec = NULL; unsigned int max_alignment_power = 0; /* Compute the expected PT_GNU_RELRO/PT_LOAD segment end. */ @@ -6382,7 +6381,7 @@ lang_size_relro_segment_1 (seg_align_type *seg) /* Adjust by the offset arg of XXX_SEGMENT_RELRO_END. */ desired_end = relro_end - seg->relro_offset; - /* For sections in the relro segment.. */ + /* Find the first section in the relro segment. */ for (sec = link_info.output_bfd->section_last; sec; sec = sec->prev) if ((sec->flags & SEC_ALLOC) != 0) { @@ -6392,71 +6391,65 @@ lang_size_relro_segment_1 (seg_align_type *seg) if (sec->vma >= seg->base && sec->vma < seg->relro_end - seg->relro_offset) { - /* Where do we want to put this section so that it ends as - desired? */ - bfd_vma start, end, bump; - - end = start = sec->vma; - if (!IS_TBSS (sec)) - end += TO_ADDR (sec->size); - bump = desired_end - end; - /* We'd like to increase START by BUMP, but we must heed - alignment so the increase might be less than optimum. */ - start += bump; - start &= ~(((bfd_vma) 1 << sec->alignment_power) - 1); - /* This is now the desired end for the previous section. */ - desired_end = start; + relro_sec = sec; prev_sec = sec->prev; } } - seg->phase = exp_seg_relro_adjust; - ASSERT (desired_end >= seg->base); - - for (; prev_sec; prev_sec = prev_sec->prev) - if ((prev_sec->flags & SEC_ALLOC) != 0) - { - if (prev_sec->alignment_power > max_alignment_power) - max_alignment_power = prev_sec->alignment_power; - - if (prev_sec->size != 0) - { - /* The 1-page gap before the RELRO segment may be removed. */ - remove_page_gap = ((prev_sec->vma + prev_sec->size - + seg->maxpagesize) < desired_end); - - break; - } - } - - if (remove_page_gap) + if (relro_sec) { - /* Find the maximum section alignment. */ - for (sec = prev_sec; sec; sec = sec->prev) - if ((sec->flags & SEC_ALLOC) != 0 - && sec->alignment_power > max_alignment_power) - max_alignment_power = sec->alignment_power; + /* Where do we want to put this section so that it ends as + desired? */ + bfd_vma start, end, bump; + + /* Find the first non-empty preceding section. */ + for (; prev_sec; prev_sec = prev_sec->prev) + if (prev_sec->size != 0 && (prev_sec->flags & SEC_ALLOC) != 0) + break; - /* Remove the 1-page gap before the RELRO segment only if the - maximum page size >= the maximum section alignment. */ + end = start = relro_sec->vma; + if (!IS_TBSS (relro_sec)) + end += TO_ADDR (relro_sec->size); + bump = desired_end - end; if (seg->maxpagesize >= (1U << max_alignment_power)) { - /* If the preceding section size is greater than the maximum - page size, subtract the maximum page size. Otherwise, - align the RELRO segment to the maximum page size. */ - if (prev_sec->size > seg->maxpagesize) + /* Don't add the 1-page gap before the relro segment if the + maximum page size >= the maximum section alignment. */ + if (bump > seg->maxpagesize) { - desired_end -= seg->maxpagesize; + bump -= seg->maxpagesize; relro_end -= seg->maxpagesize; } - else + else if (prev_sec != NULL) { - desired_end &= ~(seg->maxpagesize - 1); - relro_end &= ~(seg->maxpagesize - 1); + /* Align the relro segment if there is a 1-page gap + before the relro segment. */ + aligned_start = start + bump; + aligned_start &= ~(((bfd_vma) 1 << relro_sec->alignment_power) + - 1); + if ((prev_sec->vma + prev_sec->size + seg->maxpagesize) + < aligned_start) + { + aligned_start &= ~(seg->maxpagesize - 1); + relro_end &= ~(seg->maxpagesize - 1); + } } - } - } + } + /* We'd like to increase START by BUMP, but we must heed + alignment so the increase might be less than optimum. */ + if (aligned_start != 0) + start = aligned_start; + else + { + start += bump; + start &= ~(((bfd_vma) 1 << relro_sec->alignment_power) - 1); + } + /* This is now the desired end for the previous section. */ + desired_end = start; + } + seg->phase = exp_seg_relro_adjust; + ASSERT (aligned_start != 0 || desired_end >= seg->base); seg->base = desired_end; return relro_end; } -- 2.34.1