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;
IronPort-SDR: d35N0lk8nGiQYTfChzo9vYRbAiqA2SXxNODMdAdTzTwxEuvope2JVJP++kM0k0S4N77+1VZk9e
 nlLQt2LpLKClIsmhdZZaPneC9OoCdHXRIyUO8GRDV1VXkxN5/F8ol3AInU+lrOipRdVWC2jL9R
 XNF6GPr1z5A5KeJ+4453RG1C+loh96BzZgQwsaMg32T7MixpoiVZYIMVo1DwkokME2K1QKYizA
 3BBYcJhHLA0Bld8TlyUI+sNm9KRt5jiX/eGK6nV7h+bVtdVsJnFv82ZJnxZyHXik+gZs+X5/0N
 kHs=
From:   Damien Le Moal <Damien.LeMoal@wdc.com>
To:     Greg Ungerer <gerg@linux-m68k.org>,
        "uclinux-dev@uclinux.org" <uclinux-dev@uclinux.org>,
        "ugerg@linux-m68k.org" <ugerg@linux-m68k.org>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        "linux-riscv@lists.infradead.org" <linux-riscv@lists.infradead.org>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC:     Max Filippov <jcmvbkbc@gmail.com>, Anup Patel <Anup.Patel@wdc.com>,
        Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH v3 1/2] binfmt_flat: allow not offsetting data start
Thread-Topic: [PATCH v3 1/2] binfmt_flat: allow not offsetting data start
Thread-Index: AQHXMb61DCVMbvZIeUC4C2ar1xW5kg==
Date:   Thu, 15 Apr 2021 23:22:02 +0000
Message-ID: <BL0PR04MB6514D6C35731EA1CA581095EE74D9@BL0PR04MB6514.namprd04.prod.outlook.com>
References: <20210415061502.7248-1-damien.lemoal@wdc.com>
 <20210415061502.7248-2-damien.lemoal@wdc.com>
 <8ff801ad-6e17-fdd2-6186-5443540947a9@linux-m68k.org>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: dfa1a36b-bdc0-428a-e616-08d9006546af
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Apr 2021 23:22:02.5025
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: gxHg3st7nz6UXuNjlaWUWwA7kaVQXPwQCwSAn7tCr2uUGPFTWX6NHOsaikY80QRU+wavks2XjOcnuwMpw44klg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR04MB4916
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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 file=
=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 pointer=
=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 instruction=
. The=0A=
value for it is resolved at compile time and does not get a relocation tabl=
e=0A=
entry. Other functions calls and symbol references do have relocation table=
=0A=
entries, so the binary can be loaded anywhere. The missing relocation for t=
he=0A=
global pointer mandates that text and data be loaded at the same positions=
=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 crt1.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 have=
 the=0A=
gp loading done using a symbol address instead of a PC-relative offset. I c=
ould=0A=
then see a relocation table entry for that symbol. That still did not work =
as I=0A=
was probably doing something wrong. Anyway, such solution requires changing=
 a=0A=
lot of things in C libraries loading assembler that is common between NOMMU=
 and=0A=
MMU code. Changing it would break MMU enabled programs.=0A=
=0A=
=0A=
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>=0A=
>> Acked-by: Palmer Dabbelt <palmerdabbelt@google.com>=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 *bprm=
,=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_GZIP=
))) {=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 her=
e 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 first =
part=0A=
of the if () in the final binary. Smaller code size :)=0A=
=0A=
> =0A=
> Regards=0A=
> Greg=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 *bprm,=
=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 *bprm,=
=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 *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 + 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 *bprm,=
=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 *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_GAP_WORDS * sizeof(u32));=0A=
>>   			goto err;=0A=
>>   		}=0A=
>>   	}=0A=
>>=0A=
> =0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=