From:   Song Liu <songliubraving@meta.com>
To:     Christophe Leroy <christophe.leroy@csgroup.eu>
CC:     Song Liu <song@kernel.org>,
        "linux-modules@vger.kernel.org" <linux-modules@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "hch@lst.de" <hch@lst.de>, Kernel Team <kernel-team@meta.com>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH v10] module: replace module_layout with module_memory
Thread-Topic: [PATCH v10] module: replace module_layout with module_memory
Thread-Index: AQHZOosWgrFk94mbpUiAn23Fr3awBK7FVgEAgABscoA=
Date:   Thu, 9 Feb 2023 00:16:46 +0000
Message-ID: <70DCC6C0-53E2-4C18-B147-B2F600C66E54@fb.com>
References: <20230207002802.2514802-1-song@kernel.org>
 <b40ec330-8c9e-0265-19b9-d82b516c95c1@csgroup.eu>
In-Reply-To: <b40ec330-8c9e-0265-19b9-d82b516c95c1@csgroup.eu>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?a0IUJ3oKHsOdeyGrdjpICREo4bms3GPopRtqvBJfhdnrvii6GEZI82ZsV94P?=
 =?us-ascii?Q?RxSAPuQfoHvWWBu39TM7tgj75E8bJu8RChNmOEGhWvz6zNRTm7Ei6jGcKuWw?=
 =?us-ascii?Q?i/95rY4lR8ibFUSlofA0QaWB1YghAayQAwOC86ukkawijfa74XC/fkf2Gwl4?=
 =?us-ascii?Q?2y6rsZTVC2f52MoFf2xJhtXznAt2jzyqs0lhY+84DaR6Yu21cTyo9g8mkCh9?=
 =?us-ascii?Q?vKQqtegZcYBlsnBuUEY4j16miNrlJIpj+Sn1b4b8F5m4lXmvsyHn/wCqzQps?=
 =?us-ascii?Q?PGRPNFjg9GDB8gygmYKeoAZbPPiNUckboVkYgaCIVOphIMhKY0o0615Pw7q3?=
 =?us-ascii?Q?gzTfLt6B9fT9/AeGH/eQOM2gT+29O6CYzVjrntuhmgjG2ILucOy129hUYrMr?=
 =?us-ascii?Q?3/BfA28JdgfXs4PdegEoF+Z+f9fRrp5uKPAKxZ4s3oWQkfjZ09bFAixxFsbc?=
 =?us-ascii?Q?SpYgvgDI24NbgaFJSxU4xFQWgD+50ZuRJ+ArpaTpXXP8qtt8GJn4GT/3CpZM?=
 =?us-ascii?Q?xRA3exOjIBjUt41L6sc2mSjFk6DVeXk7szCgswkwbBEJhsaN7aFWP6rUAa4G?=
 =?us-ascii?Q?SVWQaz6CYUyUicksg2kubd81Vaw4YDWYsfS/a+qLSI1M9sWmE7ArrAR+CeI8?=
 =?us-ascii?Q?5zMdRip77/XOHePFFYzrEgvlwi9zipTG/1ZEMcJ+hk5fafnteoRQJ7Ex4cWB?=
 =?us-ascii?Q?ugIgHZLUDtVPNL+mxqgIizCSLCaC7P5zIfoBRPquTfgXk8d2/H1Qj/GZYKF4?=
 =?us-ascii?Q?q0xGCgp+n21vLR/HOhR2yf0s47C18x7D4fGNU52FPJv6A5l3nBSDjpSacVEd?=
 =?us-ascii?Q?FekFXRWG9Jl4H0ZXaOHVXVpweaTW3A+DCsQSlZS8B42w4t3I5/psEqL/hE2s?=
 =?us-ascii?Q?iq0y39n/P4vRzoxTuvUEa4zs1cuJ59mFG7StlE3YxKE6iRC3TNowvedggi4f?=
 =?us-ascii?Q?WgKPcO9EYd2YE3VkwMf9ZzBmsDGpuX8V6JrdPCOW2gpfst7BwdUC7Rrb2QEO?=
 =?us-ascii?Q?XJVq4ib+I4PCB3ZWbCFj8FNnFz5yjZz5i5uUKDRWhp1qPvnfhR2rQzsGyAxI?=
 =?us-ascii?Q?0iaIO8NRzqYyJ14+3qPIGQGYZi6LmNtrnhtny6eSTNVofGiAGOTnk1HxlIq4?=
 =?us-ascii?Q?6/nn6i6hpCOT4+bOs+lps7XvKKYikI7CG4ScKDBm3fqL2xudjoIeFNuqePQb?=
 =?us-ascii?Q?GA3xeL4gPghNiAyWcC4P2zgbg9Rd+aGLPfNZe0GcaSxGok4BzL1ekvH1AMO8?=
 =?us-ascii?Q?zFflg9qe3WU0BK2ETrfHUlzlBBxw+z6qHOdc6mNT9EOBCMnuQQKlWFdPp79h?=
 =?us-ascii?Q?5Lr/I6VghWiK18BgxsupFddBuGYydvsnCWKLRZo6VJzpkTnp/RC2TJKABrh4?=
 =?us-ascii?Q?nRPsJCT1isknFx3GtZSKsj7enofPUVL0zr3+Ay7CpENsesHCBwuzeGIe5x23?=
 =?us-ascii?Q?F3MS0wXP18bLXNdeS6kZVIyjL25RWYXcoEDHJSZ8XTj3kO1Luc7A+USiXYRU?=
 =?us-ascii?Q?jyK7CKLn3GCmxbdnuRmILYoFpptMkdmolII5LNDuCJl1o/slH5QI8dhGserb?=
 =?us-ascii?Q?CMdVJmWJzJyJPG2ItgzaFpvwA/b2zPZ6e+BXqhGfxvNExiepCqVBNKkNe0W9?=
 =?us-ascii?Q?/IeMUb2uep9M2cLGviVGNlhY7e14N/j8b7VwXCaaCSIN?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <5E55961A65FE9F478901CB17298339A3@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: 48a48297-eb53-463a-97fd-08db0a32ee2d
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Feb 2023 00:16:46.3040
 (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: TZGp9AngZuZh1dznm7KDYFzkvyGtAPBV/sFzoVU0n/iI8bfGzaCrULL7YL7u18iS6Ne3uA+/V4htuCHtRwXTsw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR15MB3750
X-Proofpoint-GUID: bXzVL_WUv-YJAK4LQ1kbaIdHIEnIKqz5
X-Proofpoint-ORIG-GUID: bXzVL_WUv-YJAK4LQ1kbaIdHIEnIKqz5
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-08_10,2023-02-08_02,2022-06-22_01
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Feb 8, 2023, at 9:48 AM, Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

[...]

>>  {
>>   unsigned long min = (unsigned long)base;
>>   unsigned long max = min + size;
>> 
>> +#ifdef CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC
> 
> A #ifdef shouldn't be required. You can use IS_ENABLED() instead:
> 
> 
> 
>> + if (mod_mem_type_is_core_data(type)) {
> 
> if (IS_ENABLED(CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC) &&
>    mod_mem_type_is_core_data(type))
> 
>> + if (min < tree->data_addr_min)
>> + tree->data_addr_min = min;
>> + if (max > tree->data_addr_max)
>> + tree->data_addr_max = max;
>> + return;
>> + }
>> +#endif

To use IS_ENABLED() here, we also need to keep data_addr_[min|max] 
around. Do we really want them? 

Btw, we will think about all these details again with the type aware 
module_alloc(). 

[...]

>> 
>> +
>> +static void free_mod_mem(struct module *mod)
>> +{
>> + /* free the memory in the right order to avoid use-after-free */
> 
> Instead of 'right order', explain what the right order is.
> As far as I understand it is only to free MOD_DATA last. Everything else 
> doesn't matter.

I rewrote the function as:

static void free_mod_mem(struct module *mod)
{
        for_each_mod_mem_type(type) {
                struct module_memory *mod_mem = &mod->mem[type];

                /* free MOD_DATA at the end, as it hosts mod */
                if (type == MOD_DATA)
                        continue;

                /* Free lock-classes; relies on the preceding sync_rcu(). */
                lockdep_free_key_range(mod_mem->base, mod_mem->size);
                if (mod_mem->size)
                        module_memory_free(mod_mem->base, type);
        }

        /* free MOD_DATA at the end, as it hosts mod */
        lockdep_free_key_range(mod->mem[MOD_DATA].base, mod->mem[MOD_DATA].size);
        module_memory_free(mod->mem[MOD_DATA].base, MOD_DATA);
}

Does this look good?

Thanks,
Song

[...]