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

On 2021/04/16 16:24, Greg Ungerer wrote:=0A=
> =0A=
> On 16/4/21 9:22 am, Damien Le Moal wrote:=0A=
>> On 2021/04/15 23:04, Greg Ungerer wrote:=0A=
>>> Hi Damien,=0A=
>>>=0A=
>>> On 15/4/21 4:15 pm, 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 contiguous text and data sections,=
=0A=
>>>> introduce the config option CONFIG_BINFMT_FLAT_NO_TEXT_DATA_GAP.=0A=
>>>> Using this new option, the macro DATA_GAP_WORDS is conditionally=0A=
>>>> defined in binfmt_flat.c to MAX_SHARED_LIBS for architectures=0A=
>>>> tolerating the text-to-data gap (CONFIG_BINFMT_FLAT_NO_TEXT_DATA_GAP=
=0A=
>>>> disabled case) and to 0 when CONFIG_BINFMT_FLAT_NO_TEXT_DATA_GAP is=0A=
>>>> enabled. DATA_GAP_WORDS is used in load_flat_file() to calculate the=
=0A=
>>>> data section length and start position.=0A=
>>>>=0A=
>>>> An architecture enabling CONFIG_BINFMT_FLAT_NO_TEXT_DATA_GAP also=0A=
>>>> prevents the use of the separate text/data load case (when the flat fi=
le=0A=
>>>> header flags FLAT_FLAG_RAM and FLAT_FLAG_GZIP are not set with NOMMU=
=0A=
>>>> kernels) and forces the use of a single RAM region for loading=0A=
>>>> (equivalent to FLAT_FLAG_RAM being set).=0A=
>>>=0A=
>>> So is it the case that a flat format file on RISC-V will never have=0A=
>>> relocations?=0A=
>>=0A=
>> No, it does have relocations. But there is no entry for the global point=
er=0A=
>> (__global_pointer$) location. This is because the loading of that value =
in the=0A=
>> gp register in the C-library crt1.S is done using a PC-relative instruct=
ion. The=0A=
>> value for it is resolved at compile time and does not get a relocation t=
able=0A=
>> entry. Other functions calls and symbol references do have relocation ta=
ble=0A=
>> entries, so the binary can be loaded anywhere. The missing relocation fo=
r the=0A=
>> global pointer mandates that text and data be loaded at the same positio=
ns=0A=
>> relative to each other that the linker file defines. Otherwise, loading =
of=0A=
>> __global_pointer$ into the gp register (first thing that C libraries crt=
1.S do)=0A=
>> result in a garbage value being loaded.=0A=
>>=0A=
>> I tried some tricks with the linker file and changing uclibc crt1.S to h=
ave the=0A=
>> gp loading done using a symbol address instead of a PC-relative offset. =
I could=0A=
>> then see a relocation table entry for that symbol. That still did not wo=
rk as I=0A=
>> was probably doing something wrong. Anyway, such solution requires chang=
ing a=0A=
>> lot of things in C libraries loading assembler that is common between NO=
MMU and=0A=
>> MMU code. Changing it would break MMU enabled programs.=0A=
>>=0A=
>>=0A=
>>>> Signed-off-by: Damien Le Moal <[email protected]>=0A=
>>>> Acked-by: Palmer Dabbelt <[email protected]>=0A=
>>>> ---=0A=
>>>>    fs/Kconfig.binfmt |  3 +++=0A=
>>>>    fs/binfmt_flat.c  | 21 +++++++++++++++------=0A=
>>>>    2 files changed, 18 insertions(+), 6 deletions(-)=0A=
>>>>=0A=
>>>> diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt=0A=
>>>> index c6f1c8c1934e..c6df931d5d45 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_TEXT_DATA_GAP=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..2be29bb964b8 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_TEXT_DATA_GAP=0A=
>>>> +#define DATA_GAP_WORDS			(0)=0A=
>>>> +#else=0A=
>>>> +#define DATA_GAP_WORDS			(MAX_SHARED_LIBS)=0A=
>>>> +#endif=0A=
>>>> +>   struct lib_info {=0A=
>>>>    	struct {=0A=
>>>>    		unsigned long start_code;		/* Start of text segment */=0A=
>>>> @@ -559,7 +565,10 @@ static int load_flat_file(struct linux_binprm *bp=
rm,=0A=
>>>>    	 * case,  and then the fully copied to RAM case which lumps=0A=
>>>>    	 * it all together.=0A=
>>>>    	 */=0A=
>>>> -	if (!IS_ENABLED(CONFIG_MMU) && !(flags & (FLAT_FLAG_RAM|FLAT_FLAG_GZ=
IP))) {=0A=
>>>> +	if (!IS_ENABLED(CONFIG_MMU) &&=0A=
>>>> +	    !IS_ENABLED(CONFIG_BINFMT_FLAT_NO_TEXT_DATA_GAP) &&=0A=
>>>=0A=
>>> If RISC-V flat format files must always be loaded to RAM then why don't=
=0A=
>>> they set the FLAT_FLAG_RAM when compiled/generated?=0A=
>>=0A=
>> That is done. The patch I have for elf2flt sets it. Coding it like this =
here is=0A=
>> I think safer (whatever the userspace toolchain did, the kernel assumes=
=0A=
>> FLAT_FLAG_RAM). And it also has the nice side effect to suppress the fir=
st part=0A=
>> of the if () in the final binary. Smaller code size :)=0A=
> =0A=
> My concern here is that CONFIG_BINFMT_FLAT_NO_TEXT_DATA_GA being=0A=
> enabled doesn't just in itself mean you need to force a RAM load.=0A=
> It is just in the RISC-V case it currently does.=0A=
=0A=
Good point.=0A=
=0A=
> =0A=
> And it may change in the future. The considerable RAM savings=0A=
> you get from supporting a separate data segment to code segment=0A=
> means there is motivation to create tooling and code generation=0A=
> to support it.=0A=
=0A=
Totally agree here. And I did try hard to get it to work...=0A=
=0A=
> =0A=
> I don't feel that strongly about it, but this code is obtuse enough alrea=
dy.=0A=
> No need to make it worse if we don't have too.=0A=
=0A=
I see your point. I will remove that=0A=
!IS_ENABLED(CONFIG_BINFMT_FLAT_NO_TEXT_DATA_GAP) from the top if() conditio=
n.=0A=
Without it, fixing the toolchain etc will indeed not require patching again=
 the=0A=
kernel. Sending a v4.=0A=
=0A=
> =0A=
> Regards=0A=
> Greg=0A=
> =0A=
> =0A=
> =0A=
>>>> +	    !(flags & (FLAT_FLAG_RAM|FLAT_FLAG_GZIP))) {=0A=
>>>> +=0A=
>>>>    		/*=0A=
>>>>    		 * this should give us a ROM ptr,  but if it doesn't we don't=0A=
>>>>    		 * really care=0A=
>>>> @@ -576,7 +585,7 @@ static int load_flat_file(struct linux_binprm *bpr=
m,=0A=
>>>>    			goto err;=0A=
>>>>    		}=0A=
>>>>    =0A=
>>>> -		len =3D data_len + extra + MAX_SHARED_LIBS * sizeof(unsigned long);=
=0A=
>>>> +		len =3D data_len + extra + DATA_GAP_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 +600,7 @@ static int load_flat_file(struct linux_binprm *bpr=
m,=0A=
>>>>    			goto err;=0A=
>>>>    		}=0A=
>>>>    		datapos =3D ALIGN(realdatastart +=0A=
>>>> -				MAX_SHARED_LIBS * sizeof(unsigned long),=0A=
>>>> +				DATA_GAP_WORDS * sizeof(unsigned long),=0A=
>>>>    				FLAT_DATA_ALIGN);=0A=
>>>>    =0A=
>>>>    		pr_debug("Allocated data+bss+stack (%u bytes): %lx\n",=0A=
>>>> @@ -622,7 +631,7 @@ static int load_flat_file(struct linux_binprm *bpr=
m,=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 + DATA_GAP_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 *bpr=
m,=0A=
>>>>    =0A=
>>>>    		realdatastart =3D textpos + ntohl(hdr->data_start);=0A=
>>>>    		datapos =3D ALIGN(realdatastart +=0A=
>>>> -				MAX_SHARED_LIBS * sizeof(u32),=0A=
>>>> +				DATA_GAP_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 *bpr=
m,=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_GAP_WORDS * sizeof(u32));=0A=
>>>>    			goto err;=0A=
>>>>    		}=0A=
>>>>    	}=0A=
>>>>=0A=
>>>=0A=
>>=0A=
>>=0A=
> =0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=