Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 22 Apr 2021 21:04:59 -0700 (PDT)
Subject:     Re: [PATCH v4 1/2] binfmt_flat: allow not offsetting data start
In-Reply-To: <BL0PR04MB651434FAFF5566A575FDBE76E7469@BL0PR04MB6514.namprd04.prod.outlook.com>
CC:     gerg@linux-m68k.org, uclinux-dev@uclinux.org,
        linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
        Anup Patel <Anup.Patel@wdc.com>, Christoph Hellwig <hch@lst.de>
From:   Palmer Dabbelt <palmer@dabbelt.com>
To:     Damien Le Moal <Damien.LeMoal@wdc.com>
Message-ID: <mhng-5208d0c7-acc3-48ac-823d-ffef9f1123e0@palmerdabbelt-glaptop>
Mime-Version: 1.0 (MHng)
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Thu, 22 Apr 2021 05:00:32 PDT (-0700), Damien Le Moal wrote:
>> On 2021/04/18 11:38, Greg Ungerer wrote:
>> 
>> 
>> On 17/4/21 2:54 pm, Damien Le Moal wrote:
>>> On 2021/04/17 13:52, Greg Ungerer wrote:
>>>>
>>>> On 17/4/21 11:10 am, Damien Le Moal wrote:
>>>>> Commit 2217b9826246 ("binfmt_flat: revert "binfmt_flat: don't offset
>>>>> the data start"") restored offsetting the start of the data section by
>>>>> a number of words defined by MAX_SHARED_LIBS. As a result, since
>>>>> MAX_SHARED_LIBS is never 0, a gap between the text and data sections
>>>>> always exists. For architectures which cannot support a such gap
>>>>> between the text and data sections (e.g. riscv nommu), flat binary
>>>>> programs cannot be executed.
>>>>>
>>>>> To allow an architecture to request no data start offset to allow for
>>>>> contiguous text and data sections for binaries flagged with
>>>>> FLAT_FLAG_RAM, introduce the new config option
>>>>> CONFIG_BINFMT_FLAT_NO_DATA_START_OFFSET. Using this new option, the
>>>>> macro DATA_START_OFFSET_WORDS is conditionally defined in binfmt_flat.c
>>>>> to MAX_SHARED_LIBS for architectures tolerating or needing the data
>>>>> start offset (CONFIG_BINFMT_FLAT_NO_DATA_START_OFFSET disabled case)
>>>>> and to 0 when CONFIG_BINFMT_FLAT_NO_DATA_START_OFFSET is enabled.
>>>>> DATA_START_OFFSET_WORDS is used in load_flat_file() to calculate the
>>>>> data section length and start position.
>>>>>
>>>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>>>>> ---
>>>>>    fs/Kconfig.binfmt |  3 +++
>>>>>    fs/binfmt_flat.c  | 19 ++++++++++++++-----
>>>>>    2 files changed, 17 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
>>>>> index c6f1c8c1934e..06fb7a93a1bd 100644
>>>>> --- a/fs/Kconfig.binfmt
>>>>> +++ b/fs/Kconfig.binfmt
>>>>> @@ -112,6 +112,9 @@ config BINFMT_FLAT_ARGVP_ENVP_ON_STACK
>>>>>    config BINFMT_FLAT_OLD_ALWAYS_RAM
>>>>>    	bool
>>>>>    
>>>>> +config BINFMT_FLAT_NO_DATA_START_OFFSET
>>>>> +	bool
>>>>> +
>>>>>    config BINFMT_FLAT_OLD
>>>>>    	bool "Enable support for very old legacy flat binaries"
>>>>>    	depends on BINFMT_FLAT
>>>>> diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
>>>>> index b9c658e0548e..1dc68dfba3e0 100644
>>>>> --- a/fs/binfmt_flat.c
>>>>> +++ b/fs/binfmt_flat.c
>>>>> @@ -74,6 +74,12 @@
>>>>>    #define	MAX_SHARED_LIBS			(1)
>>>>>    #endif
>>>>>    
>>>>> +#ifdef CONFIG_BINFMT_FLAT_NO_DATA_START_OFFSET
>>>>> +#define DATA_START_OFFSET_WORDS		(0)
>>>>> +#else
>>>>> +#define DATA_START_OFFSET_WORDS		(MAX_SHARED_LIBS)
>>>>> +#endif
>>>>> +
>>>>>    struct lib_info {
>>>>>    	struct {
>>>>>    		unsigned long start_code;		/* Start of text segment */
>>>>> @@ -560,6 +566,7 @@ static int load_flat_file(struct linux_binprm *bprm,
>>>>>    	 * it all together.
>>>>>    	 */
>>>>>    	if (!IS_ENABLED(CONFIG_MMU) && !(flags & (FLAT_FLAG_RAM|FLAT_FLAG_GZIP))) {
>>>>> +
>>>>
>>>> Random white space change...
>>>> Don't worry about re-spinning though, I will just edit this chunk out.
>>>
>>> Oops. Sorry about that. I should have better checked :)
>>>
>>>>
>>>>
>>>>>    		/*
>>>>>    		 * this should give us a ROM ptr,  but if it doesn't we don't
>>>>>    		 * really care
>>>>> @@ -576,7 +583,8 @@ static int load_flat_file(struct linux_binprm *bprm,
>>>>>    			goto err;
>>>>>    		}
>>>>>    
>>>>> -		len = data_len + extra + MAX_SHARED_LIBS * sizeof(unsigned long);
>>>>> +		len = data_len + extra +
>>>>> +			DATA_START_OFFSET_WORDS * sizeof(unsigned long);
>>>>>    		len = PAGE_ALIGN(len);
>>>>>    		realdatastart = vm_mmap(NULL, 0, len,
>>>>>    			PROT_READ|PROT_WRITE|PROT_EXEC, MAP_PRIVATE, 0);
>>>>> @@ -591,7 +599,7 @@ static int load_flat_file(struct linux_binprm *bprm,
>>>>>    			goto err;
>>>>>    		}
>>>>>    		datapos = ALIGN(realdatastart +
>>>>> -				MAX_SHARED_LIBS * sizeof(unsigned long),
>>>>> +				DATA_START_OFFSET_WORDS * sizeof(unsigned long),
>>>>>    				FLAT_DATA_ALIGN);
>>>>>    
>>>>>    		pr_debug("Allocated data+bss+stack (%u bytes): %lx\n",
>>>>> @@ -622,7 +630,8 @@ static int load_flat_file(struct linux_binprm *bprm,
>>>>>    		memp_size = len;
>>>>>    	} else {
>>>>>    
>>>>> -		len = text_len + data_len + extra + MAX_SHARED_LIBS * sizeof(u32);
>>>>> +		len = text_len + data_len + extra +
>>>>> +			DATA_START_OFFSET_WORDS * sizeof(u32);
>>>>>    		len = PAGE_ALIGN(len);
>>>>>    		textpos = vm_mmap(NULL, 0, len,
>>>>>    			PROT_READ | PROT_EXEC | PROT_WRITE, MAP_PRIVATE, 0);
>>>>> @@ -638,7 +647,7 @@ static int load_flat_file(struct linux_binprm *bprm,
>>>>>    
>>>>>    		realdatastart = textpos + ntohl(hdr->data_start);
>>>>>    		datapos = ALIGN(realdatastart +
>>>>> -				MAX_SHARED_LIBS * sizeof(u32),
>>>>> +				DATA_START_OFFSET_WORDS * sizeof(u32),
>>>>>    				FLAT_DATA_ALIGN);
>>>>>    
>>>>>    		reloc = (__be32 __user *)
>>>>> @@ -714,7 +723,7 @@ static int load_flat_file(struct linux_binprm *bprm,
>>>>>    			ret = result;
>>>>>    			pr_err("Unable to read code+data+bss, errno %d\n", ret);
>>>>>    			vm_munmap(textpos, text_len + data_len + extra +
>>>>> -				MAX_SHARED_LIBS * sizeof(u32));
>>>>> +				  DATA_START_OFFSET_WORDS * sizeof(u32));
>>>>>    			goto err;
>>>>>    		}
>>>>>    	}
>>>>>
>>>>
>>>> Thanks, otherwise looks good.
>>>>
>>>> Acked-by: Greg Ungerer <gerg@linux-m68k.org>
>>>>
>>>> I will push this into my m68knommu tree, for-next branch.
>>>> I just carry the flat format changes in that tree now to make my life easier.
>>>
>>> Great. Thanks !
>>> Are you taking both patches or should Plamer take the riscv Kconfig change
>>> through his tree ?
>> 
>> I am happy to take both.
>> Palmer?
>
>Palmer,
>
>Ping !

I already Ack'd it, in the thread where we couldn't find the maintainer.  
Keeping these togther is fine with me, and it seems easiest.

>
>
>-- 
>Damien Le Moal
>Western Digital Research
>