From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-f173.google.com (mail-pf1-f173.google.com [209.85.210.173]) by sourceware.org (Postfix) with ESMTPS id 9604A3858D28 for ; Sat, 29 Jan 2022 09:06:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9604A3858D28 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=maskray.me Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-pf1-f173.google.com with SMTP id i17so8291470pfq.13 for ; Sat, 29 Jan 2022 01:06:49 -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=L9beatJH8777uIzrBGpGRpGGTd+Ial6NpT5rLGfFc6Q=; b=qi9Mgzh0gdRJ5H39aOxdeonTcyr6vJpT7mixvpm1TDxl6iK4z/wuKCRbD3bWpDc6fK cocS0zt/o6P6YDRrwomKKQ2Bm3dEgK5WfeKV9BGomt59O70x3OnTMb5Q349qMz/MZfHY 9Hxl94pJ969fqdHc9krSr553kzw6G3oL1sF9iyPxGKUF4p+A7MMJtNVXhRv1ZOEq6r+5 F/xy5/P6/gJzGG3/ipdKXsBIBULyYB6OIUe3GsbTFE4QzE5jzmpUEYKuUEbLsw2aOHWd A5OHtc0qzKvK3Sf5XjpIWQWMsY+48eb4NtJVNpKghaj9LVZxlQRuLXQr7Q1gAJal8MKe vskQ== X-Gm-Message-State: AOAM530+ERNMHxeCG0xhxGoI/T0OPhUL7DWny7jyOxpbvyEUqJqugMMX x+qw21hXObC5mLwQyRJwEtwW3MqBxLc= X-Google-Smtp-Source: ABdhPJzG6bY7OSBuQyT0pjhsQzArem+KqZYFzeamOiZhuY35e4xQY23Ul/c0dr/PbmvQ88o5WHpzBQ== X-Received: by 2002:aa7:99c9:: with SMTP id v9mr12116489pfi.8.1643447208606; Sat, 29 Jan 2022 01:06:48 -0800 (PST) Received: from localhost ([2601:647:6300:b760:2aaf:bc70:67e7:6a5e]) by smtp.gmail.com with ESMTPSA id d8sm4663686pjz.32.2022.01.29.01.06.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 29 Jan 2022 01:06:48 -0800 (PST) Date: Sat, 29 Jan 2022 01:06:47 -0800 From: Fangrui Song To: Alan Modra Cc: "H.J. Lu" , Binutils Subject: Re: [PATCH v4] ld: Rewrite lang_size_relro_segment_1 Message-ID: <20220129090647.rizizoj6muegtttg@gmail.com> References: <5448901b-167d-4423-c99e-557d8178e56d@redhat.com> <63e7ccbb-a69e-f38e-6e36-778b5fd39ad5@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-8.0 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_INFOUSMEBIZ, KAM_NUMSUBJECT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE 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: Sat, 29 Jan 2022 09:06:52 -0000 On 2022-01-29, Alan Modra via Binutils wrote: >On Wed, Jan 26, 2022 at 06:10:28PM -0800, H.J. Lu wrote: >> On Wed, Jan 26, 2022 at 4:48 PM Alan Modra wrote: >> > >> > On Wed, Jan 26, 2022 at 10:55:35AM +0000, Nick Clifton via Binutils wrote: >> > > Hi H.J. >> > > >> > > > Here is the v4 patch to align the PT_GNU_RELRO segment first and subtract >> > > > the maximum page size if therer is still a 1-page gap. This fixes: >> > > > >> > > > https://sourceware.org/bugzilla/show_bug.cgi?id=28819 >> > > >> > > Looks good to me. Please apply. Branch and mainline. >> > >> > How was this patch tested? If it has not been tested on targets other >> > than x86 then it is not appropriate for the branch. Instead, I >> >> What kinds of tests are you looking for? Do you have a testcase to show >> there is still an issue? > >Can you answer my question? > >> > believe commit 2f83249c13d ought to be reverted on the branch. A >> > wasted page in memory layout is not an important problem. We have >> > more serious relro issues on targets other than x86. See pr28824. >> > >> > I'm speaking up because I raised concerns over the design of the >> > previous patch, not just the implementation, and had those concerns >> > overridden by Nick. Or possibly Nick didn't see my email before the >> > previous patch was approved. Regardless of how it happened, I fear >> > we might have even worse relro support in 2.38. >> > >> > FWIW, I also think that any fix I might develop for pr28824 will >> > likely not be release branch quality until is has been tested for some >> > time on mainline. > >This is the patch I'm playing with. It reverts your changes to >lang_size_relro_segment_1 and fixes PR2844. > >The end of the PT_GNU_RELRO segment must be aligned to maxpagesize in >order for the last page in the segment to be write protected after >relocation. Unfortunately we currently only align to commonpagesize, >which works for x86 but no other architecture that actually uses >maxpagesize memory pages. I've kept the commonpage alignment for x86, >other targets get maxpagesize. Wouldn't aligning to maxpagesize punish the architectures with 64KiB maxpagesize? These binaries will incur size bloat due the PT_GNU_RELRO end alignment. I think my idea of two PF_R|PF_W PT_LOAD will be worth implementing at some point :) There is no alignment. The glibc strict check https://sourceware.org/bugzilla/show_bug.cgi?id=28688 affects glibc<2.35 so I don't think ld making p_align smaller than maxpagesize is reasonable in the near term, at least before objcopy/strip supporting the small p_align becomes widespread. (I need to confess I am not closely subscribed to the sudden p_align movement in glibc) >There are of course testsuite changes that are needed, multiple x86 >and one s390x test. The only fail of any significance is pr18176, but >there is a fundamental tension between pr18176 and pr28743. You >either create holes or you waste disk space, both conditions can't be >met. Worse IMO, the pr18176 testcase not only has a hole but also a >gap in memory layout after the last section in the relro segment. >That's not ideal for powerpc where sections after .got (the last relro >section) are in some cases accessed via the got/toc pointer reg. On >x86 you'd see a gap between .got and .got.plt when SEPARATE_GOTPLT. > >diff --git a/bfd/elfxx-x86.c b/bfd/elfxx-x86.c >index da8a488db36..c9b3e7c4d9f 100644 >--- a/bfd/elfxx-x86.c >+++ b/bfd/elfxx-x86.c >@@ -4205,4 +4205,5 @@ _bfd_elf_linker_x86_set_options (struct bfd_link_info * info, > = elf_x86_hash_table (info, bed->target_id); > if (htab != NULL) > htab->params = params; >+ info->relro_use_commonpagesize = true; > } >diff --git a/include/bfdlink.h b/include/bfdlink.h >index 69fc9d33ff4..8d5561b0b28 100644 >--- a/include/bfdlink.h >+++ b/include/bfdlink.h >@@ -413,6 +413,10 @@ struct bfd_link_info > /* TRUE if PT_GNU_RELRO segment should be created. */ > unsigned int relro: 1; > >+ /* TRUE if the relro segment should be aligned to commonpagesize >+ rather than maxpagesize. */ >+ unsigned int relro_use_commonpagesize : 1; >+ > /* TRUE if DT_RELR should be enabled for compact relative > relocations. */ > unsigned int enable_dt_relr: 1; >diff --git a/ld/ldexp.c b/ld/ldexp.c >index 5f904aaf8ae..65618912043 100644 >--- a/ld/ldexp.c >+++ b/ld/ldexp.c >@@ -469,7 +469,8 @@ fold_segment_align (seg_align_type *seg, etree_value_type *lhs) > } > else > { >- expld.result.value += expld.dot & (maxpage - 1); >+ if (!link_info.relro) >+ expld.result.value += expld.dot & (maxpage - 1); > if (seg->phase == exp_seg_done) > { > /* OK. */ >@@ -480,6 +481,10 @@ fold_segment_align (seg_align_type *seg, etree_value_type *lhs) > seg->base = expld.result.value; > seg->pagesize = commonpage; > seg->maxpagesize = maxpage; >+ if (link_info.relro_use_commonpagesize) >+ seg->relropagesize = commonpage; >+ else >+ seg->relropagesize = maxpage; > seg->relro_end = 0; > } > else >@@ -508,10 +513,10 @@ fold_segment_relro_end (seg_align_type *seg, etree_value_type *lhs) > seg->relro_end = lhs->value + expld.result.value; > > if (seg->phase == exp_seg_relro_adjust >- && (seg->relro_end & (seg->pagesize - 1))) >+ && (seg->relro_end & (seg->relropagesize - 1))) > { >- seg->relro_end += seg->pagesize - 1; >- seg->relro_end &= ~(seg->pagesize - 1); >+ seg->relro_end += seg->relropagesize - 1; >+ seg->relro_end &= ~(seg->relropagesize - 1); > expld.result.value = seg->relro_end - expld.result.value; > } > else >diff --git a/ld/ldexp.h b/ld/ldexp.h >index ac4fa7e82b0..f1701b66330 100644 >--- a/ld/ldexp.h >+++ b/ld/ldexp.h >@@ -136,7 +136,8 @@ enum relro_enum { > typedef struct { > enum phase_enum phase; > >- bfd_vma base, relro_offset, relro_end, end, pagesize, maxpagesize; >+ bfd_vma base, relro_offset, relro_end, end; >+ bfd_vma pagesize, maxpagesize, relropagesize; > > enum relro_enum relro; > >diff --git a/ld/ldlang.c b/ld/ldlang.c >index 5dd3df12a0f..1a09b988fbe 100644 >--- a/ld/ldlang.c >+++ b/ld/ldlang.c >@@ -6370,101 +6370,40 @@ lang_size_segment (seg_align_type *seg) > static bfd_vma > lang_size_relro_segment_1 (seg_align_type *seg) > { >- bfd_vma relro_end, desired_relro_base; >- asection *sec, *relro_sec = NULL; >- unsigned int max_alignment_power = 0; >- bool seen_reloc_section = false; >- bool desired_relro_base_reduced = false; >+ bfd_vma relro_end, desired_end; >+ asection *sec; > > /* Compute the expected PT_GNU_RELRO/PT_LOAD segment end. */ >- relro_end = ((seg->relro_end + seg->pagesize - 1) >- & ~(seg->pagesize - 1)); >+ relro_end = (seg->relro_end + seg->relropagesize - 1) & -seg->relropagesize; > > /* Adjust by the offset arg of XXX_SEGMENT_RELRO_END. */ >- desired_relro_base = relro_end - seg->relro_offset; >+ desired_end = relro_end - seg->relro_offset; > >- /* For sections in the relro segment. */ >+ /* For sections in the relro segment.. */ > for (sec = link_info.output_bfd->section_last; sec; sec = sec->prev) >- if ((sec->flags & SEC_ALLOC) != 0) >+ if ((sec->flags & SEC_ALLOC) != 0 >+ && sec->vma >= seg->base >+ && sec->vma < seg->relro_end - seg->relro_offset) > { >- /* Record the maximum alignment for all sections starting from >- the relro segment. */ >- if (sec->alignment_power > max_alignment_power) >- max_alignment_power = sec->alignment_power; >- >- if (sec->vma >= seg->base >- && sec->vma < seg->relro_end - seg->relro_offset) >- { >- /* Where do we want to put the relro section so that the >- relro segment ends on the page bounary? */ >- bfd_vma start, end, bump; >- >- end = start = sec->vma; >- if (!IS_TBSS (sec)) >- end += TO_ADDR (sec->size); >- bump = desired_relro_base - 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_relro_base = start; >- relro_sec = sec; >- seen_reloc_section = true; >- } >- else if (seen_reloc_section) >- { >- /* Stop searching if we see a non-relro section after seeing >- relro sections. */ >- break; >- } >+ /* 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; > } > >- if (relro_sec != NULL >- && seg->maxpagesize >= (1U << max_alignment_power)) >- { >- asection *prev_sec; >- bfd_vma prev_sec_end_plus_1_page; >- >- /* Find the first preceding load section. */ >- for (prev_sec = relro_sec->prev; >- prev_sec != NULL; >- prev_sec = prev_sec->prev) >- if ((prev_sec->flags & SEC_ALLOC) != 0) >- break; >- >- prev_sec_end_plus_1_page = (prev_sec->vma + prev_sec->size >- + seg->maxpagesize); >- if (prev_sec_end_plus_1_page < desired_relro_base) >- { >- bfd_vma aligned_relro_base; >- >- desired_relro_base_reduced = true; >- >- /* Don't add the 1-page gap before the relro segment. Align >- the relro segment first. */ >- aligned_relro_base = (desired_relro_base >- & ~(seg->maxpagesize - 1)); >- if (prev_sec_end_plus_1_page < aligned_relro_base) >- { >- /* Subtract the maximum page size if therer is still a >- 1-page gap. */ >- desired_relro_base -= seg->maxpagesize; >- relro_end -= seg->maxpagesize; >- } >- else >- { >- /* Align the relro segment. */ >- desired_relro_base = aligned_relro_base; >- relro_end &= ~(seg->maxpagesize - 1); >- } >- } >- } >- > seg->phase = exp_seg_relro_adjust; >- ASSERT (desired_relro_base_reduced >- || desired_relro_base >= seg->base); >- seg->base = desired_relro_base; >+ ASSERT (desired_end >= seg->base); >+ seg->base = desired_end; > return relro_end; > } > > >-- >Alan Modra >Australia Development Lab, IBM