From:   Song Liu <songliubraving@meta.com>
To:     Thomas Gleixner <tglx@linutronix.de>
CC:     Song Liu <song@kernel.org>,
        "linux-modules@vger.kernel.org" <linux-modules@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Christoph Hellwig <hch@lst.de>,
        Kernel Team <kernel-team@meta.com>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Guenter Roeck <linux@roeck-us.net>,
        Christophe Leroy <christophe.leroy@csgroup.eu>
Subject: Re: [PATCH v9] module: replace module_layout with module_memory
Thread-Topic: [PATCH v9] module: replace module_layout with module_memory
Thread-Index: AQHZOBjM9Um86gAGJEyMnpzD+3mYlq7CeI0AgAAcZIA=
Date:   Mon, 6 Feb 2023 23:27:36 +0000
Message-ID: <09C6F0A2-5AE5-4D8D-87DE-BFEC2C642A49@fb.com>
References: <20230203214500.745276-1-song@kernel.org> <87cz6mxyb7.ffs@tglx>
In-Reply-To: <87cz6mxyb7.ffs@tglx>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ueDxwttg/d2EjmgPZ8hYm6xhxaEfEAcLr3KLuEJtt1IXA98iYAZjW7Hi8ZOl?=
 =?us-ascii?Q?95rbQiOrQjNpS47tEm4j1btSFkRghKKUCVuTIXCisEgC9zQlIGrH9fOyfsF6?=
 =?us-ascii?Q?fi1U+7bETT4e+YQshBS2oZwlrC3OGrYBDGrLEiloHZJTlj72Rgmq4ip5yqaL?=
 =?us-ascii?Q?dLhm1UDWlclaMB4lNDCA53Qj9dyzxj2JNmgDfLov3bJ7ckLy/BqQCckURra3?=
 =?us-ascii?Q?hxgUUK0gYyI8DvHsKEa4p8VU6qIHnu3VIhZNw543kFK1Y4yO1ZEOc477bUh1?=
 =?us-ascii?Q?ceEJ6luJqwri3tn3ybbfX4zGpjOVO9KHUC2Nyfa1lBKVmtgcu/ac7HIPDNIM?=
 =?us-ascii?Q?HNP4/gbjt9UAMbARFCWsyqlj8Q1nwI0icHxLjE6BwYN1n6GJEsoKcPFYXUVH?=
 =?us-ascii?Q?iUgGZsZ73e0RVVxFGxsvhlp3sCUAEW4EMkxkJzEARgmwgCDDnj+P/+8EpvNb?=
 =?us-ascii?Q?bZPHdCAd5XVNfazKY21IGC92KXf+eLeuuuf9VFolcedn1euMmZ3e0RrLdxII?=
 =?us-ascii?Q?57/o5YAvJMaUwRLAKY3RAo7opvfyX8RtSkB8V68o8JEs/a7qPOVtQI8ovarA?=
 =?us-ascii?Q?tUkmuOhvuJm8KbCNsyQaIFh5dThCkjkH2ODPbi2RxHTw5zFioRT4bVmq+Ted?=
 =?us-ascii?Q?wnnGrghfiI7jCDc76UGMtjzClYFEQu8o2wv8PrmHt+/3Rwcgzc7o6/GVARyu?=
 =?us-ascii?Q?QskD1R8kJur873uilnURWZiRci450qhzQzQvLZkSPvYBNCPWvkaK9vYQ7HfP?=
 =?us-ascii?Q?vAZZxdaRQgRujJyAeq6ejo23918p4s/QqxxyrUPGclU+VRhGvQHe3t+DmSz2?=
 =?us-ascii?Q?JZ2krAOnuQ6YK6YbeCm1zkcB/hLqoKOjwKcn/LXyhq2hZYaBUcz5aW482Wzx?=
 =?us-ascii?Q?OSvZBEOxzWmJfs+/4oxlW4CiOaG4Wo+pSFWeU4rNTiiwMzuUkCHxgaHoAtAd?=
 =?us-ascii?Q?T2XbN9mFdbVnCtKw3wOTiHO3FtoZdt3IJfEaMc2+9F6JTvFsBHZjats5faCp?=
 =?us-ascii?Q?YN4bbOP3dAMhUGUEp8mESVJTfNt1b5aUrMYY2uP26u9rV5iUQjFTjWVo5dMs?=
 =?us-ascii?Q?uvDROp2/Bluy4GUGX8Cnd/TbqbmbcI+CoCq99vy23fubEohxgRnhrUCWA+E/?=
 =?us-ascii?Q?VX/ohEEMzCIdNP752FwAtvCgGtXzh7usP61cTA2yN4ZagZf1YKG2Sbng0i3g?=
 =?us-ascii?Q?RwUhJUEEb/iHPWEo6v5PWtMNiukDKNGiq4uefyLs0DFD9NTbNYoxCdqAKjcV?=
 =?us-ascii?Q?jHpkbAOdsTsFlHbfLWWGwPL2ffdK4V9lACbPU27uFcz3YJOWr5ciupS6mdyK?=
 =?us-ascii?Q?0z8H1XFD/q4eqHurtieCNnw899lrXU0Oq+ZgBPqPSucBurLm4mSq3SEsi8pt?=
 =?us-ascii?Q?R4as0OouDP3NMkqhdLq28Ywu8ZJmisyHVD5ofnTStikzn0RpaHjtRiWBCo0w?=
 =?us-ascii?Q?DE/3gQ8KzAugfFSG1tAZ6Rd0D2wOKO2fDlpOfmw1Sz+QbrM16e+5rmErmEXM?=
 =?us-ascii?Q?mtB8YlPk5REN4dZbnjG/Ku3Rz5Jpeq4ASIu8Dc2tPjzCRmPLpqcVTUBc7yTv?=
 =?us-ascii?Q?RMRo5DeNOe3N345ZzxxjJwP1XMblPWFX+tlOYpTSN2luuaeMa/7gyOYUCOIY?=
 =?us-ascii?Q?gaC/OYWG1UrY0dDwqQdiySVzxd+9dPSYUN5dgHbCeBrz?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <E8E12BADFEBDF547AE6911E70978D293@namprd15.prod.outlook.com>
MIME-Version: 1.0
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR15MB5109.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 390ea087-c496-47ba-6dff-08db0899bafe
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Feb 2023 23:27:36.3155
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: CPbta0iy7BWXN0CMTekqjy7D6y5ZeMQJGMdQkTKOqKvysOaJ5BlFgTTjUMoiMVAv1b4MxHfWVN4tVgf/IjCUKw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR15MB4338
X-Proofpoint-ORIG-GUID: EHwukFP3tkTgXOWGOSzBgKmg88HQ6knv
X-Proofpoint-GUID: EHwukFP3tkTgXOWGOSzBgKmg88HQ6knv
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1
 definitions=2023-02-06_07,2023-02-06_03,2022-06-22_01
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Feb 6, 2023, at 1:45 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
[...]
>> @@ -67,7 +67,7 @@ static bool plt_entries_equal(const struct plt_entry *a,
>> 
>> static bool in_init(const struct module *mod, void *loc)
>> {
>> - return (u64)loc - (u64)mod->init_layout.base < mod->init_layout.size;
>> + return within_module_init((unsigned long)loc, mod);
>> }
> 
> Wouldn't it make sense to get rid of these indirections in arm[64]
> completely ?

Yeah, we can remove them. 

> 
>> struct mod_kallsyms {
>> @@ -418,12 +448,8 @@ struct module {
>> /* Startup function. */
>> int (*init)(void);
>> 
>> - /* Core layout: rbtree is accessed frequently, so keep together. */
>> - struct module_layout core_layout __module_layout_align;
>> - struct module_layout init_layout;
>> -#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
>> - struct module_layout data_layout;
>> -#endif
>> + /* rbtree is accessed frequently, so keep together. */
> 
> I'm confused about the rbtree comment here.

Let me remove it in v10. 

> 
>> + struct module_memory mem[MOD_MEM_NUM_TYPES] __module_memory_align;

[...]

>> +static void free_mod_mem(struct module *mod)
>> +{
>> + /* free the memory in the right order to avoid use-after-free */
> 
> How do we end up with a UAF when the ordering is different?

IIUC, we need remove MOD_DATA at last, which hosts "mod".

> 
>> + static enum mod_mem_type mod_mem_free_order[MOD_MEM_NUM_TYPES] = {
>> + /* first free init sections */
>> + MOD_INIT_TEXT,
>> + MOD_INIT_DATA,
>> + MOD_INIT_RODATA,
>> +
>> + /* then core sections, except rw data */
>> + MOD_TEXT,
>> + MOD_RODATA,
>> + MOD_RO_AFTER_INIT,
>> +
>> + /* last, rw data */
>> + MOD_DATA,
>> + };
> 
> That's fragile when we ever add a new section type.
> 
> static const enum mod_mem_type mod_mem_free_order[] = {
>               ....
>        };
> 
>        BUILD_BUG_ON(ARRAY_SIZE(mod_mem_free_order) != MOD_MEM_NUM_TYPES);
> 
> Hmm?

Will add this in v10. 

> 
>> 
>> static bool module_init_layout_section(const char *sname)
>> @@ -1428,6 +1506,20 @@ static void layout_sections(struct module *mod, struct load_info *info)
>> { SHF_WRITE | SHF_ALLOC, ARCH_SHF_SMALL },
>> { ARCH_SHF_SMALL | SHF_ALLOC, 0 }
>> };
>> + static int core_m_to_mem_type[] = {
> 
> const?
> 
>> + MOD_TEXT,
>> + MOD_RODATA,
>> + MOD_RO_AFTER_INIT,
>> + MOD_DATA,
>> + MOD_INVALID,
> 
> What's the point of this MOD_INVALID here?
> 
>> + };
>> + static int init_m_to_mem_type[] = {
>> + MOD_INIT_TEXT,
>> + MOD_INIT_RODATA,
>> + MOD_INVALID,
>> + MOD_INIT_DATA,
>> + MOD_INVALID,
>> + };
>> unsigned int m, i;
>> 
>> for (i = 0; i < info->hdr->e_shnum; i++)
>> @@ -1435,41 +1527,30 @@ static void layout_sections(struct module *mod, struct load_info *info)
>> 
>> pr_debug("Core section allocation order:\n");
>> for (m = 0; m < ARRAY_SIZE(masks); ++m) {
>> + enum mod_mem_type type = core_m_to_mem_type[m];
> 
> Oh. This deals with ARRAY_SIZE(masks) being larger than the
> *_to_mem_type[] ones. A comment on the *to_mem_type arrays would be
> appreciated.

Will add this in v10. 

Thanks,
Song

[...]