From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 72002C0218B for ; Fri, 24 Jan 2025 13:35:03 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id BC9536B00AE; Fri, 24 Jan 2025 08:35:02 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B79DD280053; Fri, 24 Jan 2025 08:35:02 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A19DA6B00B0; Fri, 24 Jan 2025 08:35:02 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 845E56B00AE for ; Fri, 24 Jan 2025 08:35:02 -0500 (EST) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id E9A29B183D for ; Fri, 24 Jan 2025 13:35:01 +0000 (UTC) X-FDA: 83042441202.23.37EB32B Received: from mail-wr1-f50.google.com (mail-wr1-f50.google.com [209.85.221.50]) by imf30.hostedemail.com (Postfix) with ESMTP id E5E7180008 for ; Fri, 24 Jan 2025 13:34:59 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=suse.com header.s=google header.b=RNLhDzzD; spf=pass (imf30.hostedemail.com: domain of pmladek@suse.com designates 209.85.221.50 as permitted sender) smtp.mailfrom=pmladek@suse.com; dmarc=pass (policy=quarantine) header.from=suse.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1737725700; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=UQWWJ4GUjFEA0dJ61qiJ0w7V+q7n1+jTTHCZ/wJ2aNI=; b=CpH0l34XKYQuz2SMjRdQRft5Xg4gFLCiSH1qboHvyKFxX5pP023HUO/k4FZDIg5vdfLgOq fF97BUk3kwiwXlknEtFsz9PzGb+KxoqgczHcQkw+XekooarcxjDVNFRfqRe41NzxJilUJc tpwnEN6UUvWEnnVUJQwkHw68L2FnIRg= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=suse.com header.s=google header.b=RNLhDzzD; spf=pass (imf30.hostedemail.com: domain of pmladek@suse.com designates 209.85.221.50 as permitted sender) smtp.mailfrom=pmladek@suse.com; dmarc=pass (policy=quarantine) header.from=suse.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1737725700; a=rsa-sha256; cv=none; b=JD720wBpEZJ9z4Q+P/7Wydx+UWcverBBXKd2sl2fSFBV+g6s8EdS+dIJTNCpjR7dqwIjgh ZExWmHmcYfxswOUWvTF+ASXos9tp4chs4aIQ1t9RQfubM8kLi71JTMY0e3thV+asyT9Fbz XBuimP6xG1AYAkbDAg9LXN6RUUnieAM= Received: by mail-wr1-f50.google.com with SMTP id ffacd0b85a97d-385d7b4da2bso1978989f8f.1 for ; Fri, 24 Jan 2025 05:34:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1737725698; x=1738330498; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=UQWWJ4GUjFEA0dJ61qiJ0w7V+q7n1+jTTHCZ/wJ2aNI=; b=RNLhDzzDcCwJihMBZsi7s1rgiXkH+i/VBOvT+aASzfO9Um9qcFWYyrmOdVevzOoz8E 1ZRarrus+UP1BUjchKNsnDpXa9j+P7DD3OrGtC79PGmoTWVW0ecGBiijKsIpXo7USxB+ tr/HLFSKbzouHPEqUEf1Fdse7tGG/ttm0By6VtaR4N4FyJv2WCzXl2XuB61cjedael2d Pk052p3HvYwF5gjN+fdOTrhBqgJ4ZKHzRCL+fnz9yb3gPMz8tFz1m3OhfvteYVzOyN5v GGCG8Bni3+63xv38Jgz7Ij7w4vrrrp50IPNR1OBeqoYwatVQ4PpxqSljslUuxYYIyoCE dxqQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1737725698; x=1738330498; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=UQWWJ4GUjFEA0dJ61qiJ0w7V+q7n1+jTTHCZ/wJ2aNI=; b=CeZEBoG6nThgJ9ZBORYmcWin5NgnQ2L2MnoyaXJJJ0IQXeKPpfe9teQjnSP3K/sbeU ODKu7GCsoo6cG/65WTUQfB/vQcYRM0xzqYtThqnYB8Ihpy3J9nhxo8kHNDoPvs+zhYwz hcBJ08CEEGU/JtmLWtXfapAwkuPNu5tppN901ZFXKrlEn3t7XnWqEj14chm6nOqjCOID G4qz29rm1KMKLOK2dPJOewDGbp0MCRRf2ZjPfbldkpq4KiYkW7d/iMXd1QZCndz3nrHo /rLAO1V2oGq6IXPZass2SG5nWvSL9X31itFW9mPAELV1MPfGsHFXK22TU8siSVDpaVk4 eBmQ== X-Forwarded-Encrypted: i=1; AJvYcCUdeb2TuOkm4NiTAoSB8fZZLa4qTKXR4lgvrqzDpCDVc55RmT8LX2x8Ak/qPnr7ahC5xlHLzQQWAA==@kvack.org X-Gm-Message-State: AOJu0YyUzgVDyk0QGOEqpQTOOO7DH/URL+l+HVh+AXsYWR0CyARNCKdj BaJOjSw9jDOQLUeFfBnQ3XHM7OR4bLHb7kfX1I5tx+Qx8f5jIw26UkEXDkR6PfM= X-Gm-Gg: ASbGncsRG+8dIxrzJMngeMey38+0bpDqzG5sxnuP5W61/xN4eEY8+rI2DYpUOcFIB+I ytQYFW2eM/TYtTESQq/nmFNWPTBkrwIcJf7Jy1aiScnzdLzWUfyRP+2m0CDp/FfQk0+BEFKAZSM S6dgS+yTM7lpGzCWMu9RsL4CeJwL/BxOKMPgZQ9FmcMgd3VKGPBf3+2+71YvJcP3b+S6ts4ubZJ h82059iebHO5nSOPgzO8FG1iYQDQmZuleZyF3dxmCmd+iWJWrxYRJQmjHhuIWQQSD/PMxn5GT0u eWORRjA= X-Google-Smtp-Source: AGHT+IF3s4fU5+t8/f+hOsEy5Nf5bcm49sgfqQgftaIHUM1gK9rPTY+VC8hE4cREhJOWCxCt9EWOug== X-Received: by 2002:a5d:64a3:0:b0:385:f909:eb2c with SMTP id ffacd0b85a97d-38bf57a77a2mr37468273f8f.38.1737725698250; Fri, 24 Jan 2025 05:34:58 -0800 (PST) Received: from pathway.suse.cz ([176.114.240.130]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-38c2a1bb0besm2727227f8f.79.2025.01.24.05.34.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 24 Jan 2025 05:34:57 -0800 (PST) Date: Fri, 24 Jan 2025 14:34:55 +0100 From: Petr Mladek To: Petr Pavlu Cc: Mike Rapoport , x86@kernel.org, Andrew Morton , Andy Lutomirski , Anton Ivanov , Borislav Petkov , Brendan Higgins , Daniel Gomez , Daniel Thompson , Dave Hansen , David Gow , Douglas Anderson , Ingo Molnar , Jason Wessel , Jiri Kosina , Joe Lawrence , Johannes Berg , Josh Poimboeuf , "Kirill A. Shutemov" , Lorenzo Stoakes , Luis Chamberlain , Mark Rutland , Masami Hiramatsu , Miroslav Benes , "H. Peter Anvin" , Peter Zijlstra , Rae Moar , Richard Weinberger , Sami Tolvanen , Shuah Khan , Song Liu , Steven Rostedt , Thomas Gleixner , kgdb-bugreport@lists.sourceforge.net, kunit-dev@googlegroups.com, linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org, linux-mm@kvack.org, linux-modules@vger.kernel.org, linux-trace-kernel@vger.kernel.org, linux-um@lists.infradead.org, live-patching@vger.kernel.org Subject: Re: [PATCH v2 06/10] module: introduce MODULE_STATE_GONE Message-ID: References: <20250121095739.986006-1-rppt@kernel.org> <20250121095739.986006-7-rppt@kernel.org> <4a9ca024-fc25-4fe0-94d5-65899b2cec6b@suse.com> <8c6972c4-c1bb-402a-a72d-f92b87ee5a89@suse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8c6972c4-c1bb-402a-a72d-f92b87ee5a89@suse.com> X-Rspamd-Queue-Id: E5E7180008 X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: xck7c6uxnokafns93fn9pkwwbkjfhktt X-HE-Tag: 1737725699-928871 X-HE-Meta: U2FsdGVkX18WOXgGRaKrn6Eq1agZyhs4WjLjNvmAoDz0010zCP1iEZZhlZriNAyLWpK7eLCijHV+fAp3BMocrnX5Nv8/r9iP6KsXC+VWFdRrCoDxLD2nEWd6lh4SHlJzjj94AuBylljyz9IWsFEl1fg/RI+cXIsmDqQz+SVUCeAo6Nl4XwzDr55cZgzhEHFxn4uZ43srwMKp8gSlPQeR3Olyhp46Ts7Jtu6unqnsykbznUirK5odyY4mmGhoOKko/Fz9nvbEZG6YFZ8nHB30fyylmI7f5HAse2YN8LeJ7IalAeYyAKrq5Pu+f5q0o+5vrHMnXn/WuyrV+64dyjoRnInSs4APuHM2/Sjxi0L8u59sIMnAxUQ91quenq3gPi5C7zXeRRGGH/lcttzfTxb5i2z6oOoDk6A8g+e1qXQ9xyLXTTYAOd7ZxdDR+Wrnv4WUhhvZtpWZgbLQrtoQZqygp7LnTs6jQeL/qZEo3Q8BUUKFs4lGvZyMQrNOKd8aV40ef7LYg9Clg9/KWfluGSp53f87xqtflHCcRnm2c8Cx8PqzJslpbCNK+EE++eeRc24+ia8SjfdQEIjm48v0bjPL8Y7LBzvomik1ZHQwyaRDpn4xDYB2RtA+j5DrgVu1NAmqVFzdo6wQiEg0by3e6+itVcHJSoaJpZhMeyJLHAeJlHr3E/z0qSHTY+TsHDR5zEj48tqx6oFLWN/Pzh5Hc9sAuWWfbClgInyKxI5N4ULReLL3+aNLmrmUChqR+PEQpKdxp5+6KM7rXVAUuT9eSPQ7SxB6TLRUq9zQzPGoloNcG2MMG65MRRoLR6uGj5JVkN5QzesKhW+sKFt0OrQidA8uXw+n4l/7GWG9wfkSr7hGDFaUo7lygE/Y3KE6WENeMvcE2rNnMzjO0EaD72OYnwBXAfdFShgvjRTfUkPIuqf5qpf6rikBOH8WM/YQFx6GILixmDFQOl9HoCPXSojoT5M pMX47Bfs XgPrN0LZKAOdtXI2LKWbNiE2taAkf+P/6b9d0jDSuagbR/NvxgPdnhvIhOAIwl4MNjDMywaTvDQDnKnwluyGGU69xr7zhNhibTJmSocogV+LTGmj+s75W9sCyocVn2AmF3Ztpk0IALvQHJD6eUAZUnaaJ/03CoWF0cWJCyUmZ4/Dsy6vYqV/tpRtghcgq1TIc2NSZ38OUQtN9bri2mFYD2rTTrViCKwVyFXFJ1CztfyIeibS6hFkYyQpjw7nxAs5vSDSIiIip3I3H4m1uYuFO1ni1QNbsb1l4nktbYmPfBhTxIwcEPOQRhfpoQaku7ftRa9x4 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Fri 2025-01-24 13:59:55, Petr Pavlu wrote: > On 1/24/25 12:06, Mike Rapoport wrote: > > On Thu, Jan 23, 2025 at 03:16:28PM +0100, Petr Pavlu wrote: > >> On 1/21/25 10:57, Mike Rapoport wrote: > >>> In order to use execmem's API for temporal remapping of the memory > >>> allocated from ROX cache as writable, there is a need to distinguish > >>> between the state when the module is being formed and the state when it is > >>> deconstructed and freed so that when module_memory_free() is called from > >>> error paths during module loading it could restore ROX mappings. > >>> > >>> Replace open coded checks for MODULE_STATE_UNFORMED with a helper > >>> function module_is_formed() and add a new MODULE_STATE_GONE that will be > >>> set when the module is deconstructed and freed. > >> > >> I don't fully follow why this case requires a new module state. My > >> understanding it that the function load_module() has the necessary > >> context that after calling layout_and_allocate(), the updated ROX > >> mappings need to be restored. I would then expect the function to be > >> appropriately able to unwind this operation in case of an error. It > >> could be done by having a helper that walks the mappings and calls > >> execmem_restore_rox(), or if you want to keep it in module_memory_free() > >> as done in the patch #7 then a flag could be passed down to > >> module_deallocate() -> free_mod_mem() -> module_memory_free()? > > > > Initially I wanted to track ROX <-> RW transitions in struct module_memory > > so that module_memory_free() could do the right thing depending on memory > > state. But that meant either ugly games with const'ness in strict_rwx.c, > > an additional helper or a new global module state. The latter seemed the > > most elegant to me. > > If a new global module state is really that intrusive, I can drop it in > > favor a helper that will be called from error handling paths. E.g. > > something like the patch below (on top of this series and with this patch > > reverted) > > > > diff --git a/kernel/module/main.c b/kernel/module/main.c > > index 7164cd353a78..4a02503836d7 100644 > > --- a/kernel/module/main.c > > +++ b/kernel/module/main.c > > @@ -1268,13 +1268,20 @@ static int module_memory_alloc(struct module *mod, enum mod_mem_type type) > > return 0; > > } > > > > +static void module_memory_restore_rox(struct module *mod) > > +{ > > + for_class_mod_mem_type(type, text) { > > + struct module_memory *mem = &mod->mem[type]; > > + > > + if (mem->is_rox) > > + execmem_restore_rox(mem->base, mem->size); > > + } > > +} > > + > > static void module_memory_free(struct module *mod, enum mod_mem_type type) > > { > > struct module_memory *mem = &mod->mem[type]; > > > > - if (mod->state == MODULE_STATE_UNFORMED && mem->is_rox) > > - execmem_restore_rox(mem->base, mem->size); > > - > > execmem_free(mem->base); > > } > > > > @@ -2617,6 +2624,7 @@ static int move_module(struct module *mod, struct load_info *info) > > > > return 0; > > out_err: > > + module_memory_restore_rox(mod); > > for (t--; t >= 0; t--) > > module_memory_free(mod, t); > > if (codetag_section_found) > > @@ -3372,6 +3380,7 @@ static int load_module(struct load_info *info, const char __user *uargs, > > mod->mem[type].size); > > } > > > > + module_memory_restore_rox(mod); > > module_deallocate(mod, info); > > free_copy: > > /* > > > > This looks better to me. > > My view is that the module_state tracks major stages of a module during > its lifecycle. It provides information to the module loader itself, > other subsystems that need to closely interact with modules, and to the > userspace via the initstate sysfs attribute. > > Adding a new state means potentially more complexity for all these > parts. In this case, the state was needed because of a logic that is > local only to the module loader, or even just to the function > load_module(). I think it is better to avoid adding a new state only for > that. I fully agree here. The added complexity is already visible in the original patch. It updates about 15 locations where mod->state is checked. Every location should be reviewed whether the change is correct. The changes are spread in various subsystems, like kallsyms, kdb, tracepoint, livepatch. Many people need to understand the meaning of the new state and decide if the change is OK. So, it affects many people and touches 15 locations where things my go wrong. The alternative solution, proposed above, looks much easier to me. Best Regards, Petr