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

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