Re: [PATCH v4 1/2] binfmt_flat: allow not offsetting data start

From: Damien Le Moal
Date: Thu Apr 22 2021 - 08:00:39 EST


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@xxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxxxxxx>
>>>
>>> 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 !


--
Damien Le Moal
Western Digital Research