From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-f172.google.com (mail-pl1-f172.google.com [209.85.214.172]) by sourceware.org (Postfix) with ESMTPS id 736733857809 for ; Fri, 14 Jan 2022 09:37:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 736733857809 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-pl1-f172.google.com with SMTP id e19so12801206plc.10 for ; Fri, 14 Jan 2022 01:37:47 -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=g3HB/llqMFyauX4DcV2aqU9jAlgPnZKB1pRB32Opz+A=; b=a180PPZdXkBwX7cxX12GAdOyXa05fs0trw8LXSpic/A24N3WmNC3OoN69UYBN2SD0Z 3xF2AIOztf7IJv4IxDYrw+8ahsDEH2yfKNJIf4qKr5cndL19boowm7Qq3jSppdYsyW2C T/gW04ZYjLpPiTLyvLPqdD70aSq9OskGjME07AkT00gtojaQEm86UQ9CzQVQ4DXT+ezd 08EB1jAK7wQ6iroJD9S0aICEOZMGVUiEMv4faWao5QcrgeomBWXPEyetUgfeO24Y65nL BOh8YEdnJObAKRWNTSDA6vUbqk+tTFsu6n6xa92xAqKaO59BaJ0M2/t/wQ4JdtnArfoM oqFg== X-Gm-Message-State: AOAM531wnZCNbj59utJsAJHZ+UqbX/AJ3qLrhT5+1BEgTQ+f4SDXVI0k /ZeSI76aP9ABDhSjs4K7Q6A= X-Google-Smtp-Source: ABdhPJzpL9SVdDI4qOj0XQlP3DHwEH0ZPyoFznaGteMjGQmUt3sGyklSWT/JuKecZ6cjsYo+AeySOQ== X-Received: by 2002:a17:90b:4ad1:: with SMTP id mh17mr3798276pjb.160.1642153066523; Fri, 14 Jan 2022 01:37:46 -0800 (PST) Received: from localhost ([2601:647:6300:b760:d1cf:13f5:410f:b3a8]) by smtp.gmail.com with ESMTPSA id o187sm4914337pfg.17.2022.01.14.01.37.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 14 Jan 2022 01:37:46 -0800 (PST) Date: Fri, 14 Jan 2022 01:37:45 -0800 From: Fangrui Song To: Alan Modra Cc: "H.J. Lu" , Binutils Subject: Re: [PATCH] elf: Remove the 1-page gap before the RELRO segment Message-ID: <20220114093745.rjjquetgtqtdlu3w@gmail.com> References: <20220111021241.1937265-1-hjl.tools@gmail.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=-1.6 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_INFOUSMEBIZ, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=no 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 09:37:49 -0000 On 2022-01-14, Alan Modra via Binutils 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. How about the 2-RW design? |RW(RELRO) |RW(non-RELRO) The bar | indicates an alignment. Only alignment the start of a segment, never the end of a segment. It may avoid the unusual "otherwise DATA_SEGMENT_ALIGN is padded so that exp + offset is aligned to the commonpagesize argument given to DATA_SEGMENT_ALIGN" semantics on https://sourceware.org/binutils/docs/ld/Builtin-Functions.html I thought about the layout in the past and did not find cases where it used more pages than the single-RW layout, but I did not think very hard. (I haven't thought about max-page-size > common-page-size cases.)