* Re: [PATCH 0/2] Restrict module namespace to in-tree modules and rename macro
From: Christian Brauner @ 2025-07-08 7:40 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Matthias Maennich, Jonathan Corbet, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Masahiro Yamada, Nathan Chancellor,
Nicolas Schier, Alexander Viro, Jan Kara, Christoph Hellwig,
Peter Zijlstra, David Hildenbrand, Shivank Garg,
Jiri Slaby (SUSE), Stephen Rothwell, linux-doc, linux-kernel,
linux-modules, linux-kbuild, linux-fsdevel
In-Reply-To: <20250708-export_modules-v1-0-fbf7a282d23f@suse.cz>
On Tue, Jul 08, 2025 at 09:28:56AM +0200, Vlastimil Babka wrote:
> Christian asked [1] for EXPORT_SYMBOL_FOR_MODULES() without the _GPL_
> part to avoid controversy converting selected existing EXPORT_SYMBOL().
> Christoph argued [2] that the _FOR_MODULES() export is intended for
> in-tree modules and thus GPL is implied anyway and can be simply dropped
> from the export macro name. Peter agreed [3] about the intention for
> in-tree modules only, although nothing currently enforces it.
>
> It seems straightforward to add this enforcement, so patch 1 does that.
> Patch 2 then drops the _GPL_ from the name and so we're left with
> EXPORT_SYMBOL_FOR_MODULES() restricted to in-tree modules only.
>
> Current -next has some new instances of EXPORT_SYMBOL_GPL_FOR_MODULES()
> in drivers/tty/serial/8250/8250_rsa.c by commit b20d6576cdb3 ("serial:
> 8250: export RSA functions"). Hopefully it's resolvable by a merge
> commit fixup and we don't need to provide a temporary alias.
>
> [1] https://lore.kernel.org/all/20250623-warmwasser-giftig-ff656fce89ad@brauner/
> [2] https://lore.kernel.org/all/aFleJN_fE-RbSoFD@infradead.org/
> [3] https://lore.kernel.org/all/20250623142836.GT1613200@noisy.programming.kicks-ass.net/
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
Love this. It'd be great to get this in as a bugfix,
Acked-by: Christian Brauner <brauner@kernel.org>
^ permalink raw reply
* Re: [PATCH 0/2] Restrict module namespace to in-tree modules and rename macro
From: David Hildenbrand @ 2025-07-08 7:43 UTC (permalink / raw)
To: Christian Brauner, Vlastimil Babka
Cc: Matthias Maennich, Jonathan Corbet, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Masahiro Yamada, Nathan Chancellor,
Nicolas Schier, Alexander Viro, Jan Kara, Christoph Hellwig,
Peter Zijlstra, Shivank Garg, Jiri Slaby (SUSE), Stephen Rothwell,
linux-doc, linux-kernel, linux-modules, linux-kbuild,
linux-fsdevel
In-Reply-To: <20250708-merkmal-erhitzen-23e7e9daa150@brauner>
On 08.07.25 09:40, Christian Brauner wrote:
> On Tue, Jul 08, 2025 at 09:28:56AM +0200, Vlastimil Babka wrote:
>> Christian asked [1] for EXPORT_SYMBOL_FOR_MODULES() without the _GPL_
>> part to avoid controversy converting selected existing EXPORT_SYMBOL().
>> Christoph argued [2] that the _FOR_MODULES() export is intended for
>> in-tree modules and thus GPL is implied anyway and can be simply dropped
>> from the export macro name. Peter agreed [3] about the intention for
>> in-tree modules only, although nothing currently enforces it.
>>
>> It seems straightforward to add this enforcement, so patch 1 does that.
>> Patch 2 then drops the _GPL_ from the name and so we're left with
>> EXPORT_SYMBOL_FOR_MODULES() restricted to in-tree modules only.
>>
>> Current -next has some new instances of EXPORT_SYMBOL_GPL_FOR_MODULES()
>> in drivers/tty/serial/8250/8250_rsa.c by commit b20d6576cdb3 ("serial:
>> 8250: export RSA functions"). Hopefully it's resolvable by a merge
>> commit fixup and we don't need to provide a temporary alias.
>>
>> [1] https://lore.kernel.org/all/20250623-warmwasser-giftig-ff656fce89ad@brauner/
>> [2] https://lore.kernel.org/all/aFleJN_fE-RbSoFD@infradead.org/
>> [3] https://lore.kernel.org/all/20250623142836.GT1613200@noisy.programming.kicks-ass.net/
>>
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>> ---
>
> Love this. It'd be great to get this in as a bugfix,
> Acked-by: Christian Brauner <brauner@kernel.org>
Yes, looks like the right thing to do
Acked-by: David Hildenbrand <david@redhat.com>
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH 0/2] Restrict module namespace to in-tree modules and rename macro
From: Stephen Rothwell @ 2025-07-08 7:49 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Matthias Maennich, Jonathan Corbet, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Masahiro Yamada, Nathan Chancellor,
Nicolas Schier, Alexander Viro, Christian Brauner, Jan Kara,
Christoph Hellwig, Peter Zijlstra, David Hildenbrand,
Shivank Garg, Jiri Slaby (SUSE), linux-doc, linux-kernel,
linux-modules, linux-kbuild, linux-fsdevel, Greg KH
In-Reply-To: <20250708-export_modules-v1-0-fbf7a282d23f@suse.cz>
[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]
Hi Vlastimil,
On Tue, 08 Jul 2025 09:28:56 +0200 Vlastimil Babka <vbabka@suse.cz> wrote:
>
> Christian asked [1] for EXPORT_SYMBOL_FOR_MODULES() without the _GPL_
> part to avoid controversy converting selected existing EXPORT_SYMBOL().
> Christoph argued [2] that the _FOR_MODULES() export is intended for
> in-tree modules and thus GPL is implied anyway and can be simply dropped
> from the export macro name. Peter agreed [3] about the intention for
> in-tree modules only, although nothing currently enforces it.
>
> It seems straightforward to add this enforcement, so patch 1 does that.
> Patch 2 then drops the _GPL_ from the name and so we're left with
> EXPORT_SYMBOL_FOR_MODULES() restricted to in-tree modules only.
>
> Current -next has some new instances of EXPORT_SYMBOL_GPL_FOR_MODULES()
> in drivers/tty/serial/8250/8250_rsa.c by commit b20d6576cdb3 ("serial:
> 8250: export RSA functions"). Hopefully it's resolvable by a merge
> commit fixup and we don't need to provide a temporary alias.
Thanks for the heads up and it seems easy enough. You probably should
have cc'd Greg KH (the maintainer for the tty tree), though (done now).
>
> [1] https://lore.kernel.org/all/20250623-warmwasser-giftig-ff656fce89ad@brauner/
> [2] https://lore.kernel.org/all/aFleJN_fE-RbSoFD@infradead.org/
> [3] https://lore.kernel.org/all/20250623142836.GT1613200@noisy.programming.kicks-ass.net/
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
> Vlastimil Babka (2):
> module: Restrict module namespace access to in-tree modules
> module: Rename EXPORT_SYMBOL_GPL_FOR_MODULES to EXPORT_SYMBOL_FOR_MODULES
>
> Documentation/core-api/symbol-namespaces.rst | 11 ++++++-----
> fs/anon_inodes.c | 2 +-
> include/linux/export.h | 2 +-
> kernel/module/main.c | 3 ++-
> scripts/mod/modpost.c | 6 +++++-
> 5 files changed, 15 insertions(+), 9 deletions(-)
> ---
> base-commit: d7b8f8e20813f0179d8ef519541a3527e7661d3a
> change-id: 20250708-export_modules-12908fa41006
>
> Best regards,
> --
> Vlastimil Babka <vbabka@suse.cz>
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] module: Restrict module namespace access to in-tree modules
From: Shivank Garg @ 2025-07-08 7:54 UTC (permalink / raw)
To: Vlastimil Babka, Matthias Maennich, Jonathan Corbet,
Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier,
Alexander Viro, Christian Brauner, Jan Kara
Cc: Christoph Hellwig, Peter Zijlstra, David Hildenbrand,
Jiri Slaby (SUSE), Stephen Rothwell, linux-doc, linux-kernel,
linux-modules, linux-kbuild, linux-fsdevel
In-Reply-To: <20250708-export_modules-v1-1-fbf7a282d23f@suse.cz>
On 7/8/2025 12:58 PM, Vlastimil Babka wrote:
> The module namespace support has been introduced to allow restricting
> exports to specific modules only, and intended for in-tree modules such
> as kvm. Make this intention explicit by disallowing out of tree modules
> both for the module loader and modpost.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
> Documentation/core-api/symbol-namespaces.rst | 5 +++--
> kernel/module/main.c | 3 ++-
> scripts/mod/modpost.c | 6 +++++-
> 3 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/core-api/symbol-namespaces.rst b/Documentation/core-api/symbol-namespaces.rst
> index 32fc73dc5529e8844c2ce2580987155bcd13cd09..dc228ac738a5cdc49cc736c29170ca96df6a28dc 100644
> --- a/Documentation/core-api/symbol-namespaces.rst
> +++ b/Documentation/core-api/symbol-namespaces.rst
> @@ -83,13 +83,14 @@ Symbols exported using this macro are put into a module namespace. This
> namespace cannot be imported.
>
> The macro takes a comma separated list of module names, allowing only those
> -modules to access this symbol. Simple tail-globs are supported.
> +modules to access this symbol. The access is restricted to in-tree modules.
> +Simple tail-globs are supported.
>
> For example::
>
> EXPORT_SYMBOL_GPL_FOR_MODULES(preempt_notifier_inc, "kvm,kvm-*")
>
> -will limit usage of this symbol to modules whoes name matches the given
> +will limit usage of this symbol to in-tree modules whoes name matches the given
> patterns.
>
> How to use Symbols exported in Namespaces
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 413ac6ea37021bc8ae260f624ca2745ed85333fc..ec7d8daa0347e3b65713396d6b6d14c2cb0270d3 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -1157,7 +1157,8 @@ static int verify_namespace_is_imported(const struct load_info *info,
> namespace = kernel_symbol_namespace(sym);
> if (namespace && namespace[0]) {
>
> - if (verify_module_namespace(namespace, mod->name))
> + if (get_modinfo(info, "intree") &&
> + verify_module_namespace(namespace, mod->name))
> return 0;
>
> for_each_modinfo_entry(imported_namespace, info, "import_ns") {
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 5ca7c268294ebb65acb0ba52a671eddca9279c61..d78be9834ed75f4b6ddb9af02a300a9bcc9234cc 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1695,7 +1695,8 @@ void buf_write(struct buffer *buf, const char *s, int len)
> * @modname: module name
> *
> * If @namespace is prefixed with "module:" to indicate it is a module namespace
> - * then test if @modname matches any of the comma separated patterns.
> + * then test if @modname matches any of the comma separated patterns. Access to
> + * module namespaces is restricted to in-tree modules only.
> *
> * The patterns only support tail-glob.
> */
> @@ -1706,6 +1707,9 @@ static bool verify_module_namespace(const char *namespace, const char *modname)
> const char *sep;
> bool glob;
>
> + if (external_module)
> + return false;
> +
> if (!strstarts(namespace, prefix))
> return false;
>
>
Reviewed-by: Shivank Garg <shivankg@amd.com>
Thanks,
Shivank
^ permalink raw reply
* Re: [PATCH 2/2] module: Rename EXPORT_SYMBOL_GPL_FOR_MODULES to EXPORT_SYMBOL_FOR_MODULES
From: Shivank Garg @ 2025-07-08 7:55 UTC (permalink / raw)
To: Vlastimil Babka, Matthias Maennich, Jonathan Corbet,
Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Masahiro Yamada, Nathan Chancellor, Nicolas Schier,
Alexander Viro, Christian Brauner, Jan Kara
Cc: Christoph Hellwig, Peter Zijlstra, David Hildenbrand,
Jiri Slaby (SUSE), Stephen Rothwell, linux-doc, linux-kernel,
linux-modules, linux-kbuild, linux-fsdevel
In-Reply-To: <20250708-export_modules-v1-2-fbf7a282d23f@suse.cz>
On 7/8/2025 12:58 PM, Vlastimil Babka wrote:
> With module namespace access restricted to in-tree modules, the GPL
> requirement becomes implied. Drop it from the name of the export helper.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
> Documentation/core-api/symbol-namespaces.rst | 6 +++---
> fs/anon_inodes.c | 2 +-
> include/linux/export.h | 2 +-
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/core-api/symbol-namespaces.rst b/Documentation/core-api/symbol-namespaces.rst
> index dc228ac738a5cdc49cc736c29170ca96df6a28dc..aafbc0469cd6a4b76225e0e96a86025de512008e 100644
> --- a/Documentation/core-api/symbol-namespaces.rst
> +++ b/Documentation/core-api/symbol-namespaces.rst
> @@ -76,8 +76,8 @@ A second option to define the default namespace is directly in the compilation
> within the corresponding compilation unit before the #include for
> <linux/export.h>. Typically it's placed before the first #include statement.
>
> -Using the EXPORT_SYMBOL_GPL_FOR_MODULES() macro
> ------------------------------------------------
> +Using the EXPORT_SYMBOL_FOR_MODULES() macro
> +-------------------------------------------
>
> Symbols exported using this macro are put into a module namespace. This
> namespace cannot be imported.
> @@ -88,7 +88,7 @@ Simple tail-globs are supported.
>
> For example::
>
> - EXPORT_SYMBOL_GPL_FOR_MODULES(preempt_notifier_inc, "kvm,kvm-*")
> + EXPORT_SYMBOL_FOR_MODULES(preempt_notifier_inc, "kvm,kvm-*")
>
> will limit usage of this symbol to in-tree modules whoes name matches the given
> patterns.
> diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
> index 1d847a939f29a41356af3f12e5f61372ec2fb550..180a458fc4f74249d674ec3c6e01277df1d9e743 100644
> --- a/fs/anon_inodes.c
> +++ b/fs/anon_inodes.c
> @@ -129,7 +129,7 @@ struct inode *anon_inode_make_secure_inode(struct super_block *sb, const char *n
> }
> return inode;
> }
> -EXPORT_SYMBOL_GPL_FOR_MODULES(anon_inode_make_secure_inode, "kvm");
> +EXPORT_SYMBOL_FOR_MODULES(anon_inode_make_secure_inode, "kvm");
>
> static struct file *__anon_inode_getfile(const char *name,
> const struct file_operations *fops,
> diff --git a/include/linux/export.h b/include/linux/export.h
> index f35d03b4113b19798036d2993d67eb932ad8ce6f..a686fd0ba406509da5f397e3a415d05c5a051c0d 100644
> --- a/include/linux/export.h
> +++ b/include/linux/export.h
> @@ -91,6 +91,6 @@
> #define EXPORT_SYMBOL_NS(sym, ns) __EXPORT_SYMBOL(sym, "", ns)
> #define EXPORT_SYMBOL_NS_GPL(sym, ns) __EXPORT_SYMBOL(sym, "GPL", ns)
>
> -#define EXPORT_SYMBOL_GPL_FOR_MODULES(sym, mods) __EXPORT_SYMBOL(sym, "GPL", "module:" mods)
> +#define EXPORT_SYMBOL_FOR_MODULES(sym, mods) __EXPORT_SYMBOL(sym, "GPL", "module:" mods)
>
> #endif /* _LINUX_EXPORT_H */
>
LGTM!
Reviewed-by: Shivank Garg <shivankg@amd.com>
Thanks,
Shivank
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: update Daniel Gomez's role and email address
From: Petr Pavlu @ 2025-07-08 8:00 UTC (permalink / raw)
To: Daniel Gomez
Cc: Luis Chamberlain, Daniel Gomez, Sami Tolvanen, linux-modules,
linux-kernel, Daniel Gomez
In-Reply-To: <20250704-add-dagomez-maintainer-v1-1-5fc32033c51c@samsung.com>
On 7/4/25 9:39 PM, Daniel Gomez wrote:
> From: Daniel Gomez <da.gomez@samsung.com>
>
> Update Daniel Gomez's modules reviewer role to maintainer. This is
> according to the plan [1][2] of scaling with more reviewers for modules
> (for the incoming Rust support [3]) and rotate [4] every 6 months.
>
> [1] Link:
> https://lore.kernel.org/linux-modules/
> ZsPANzx4-5DrOl5m@bombadil.infradead.org
>
> https://lore.kernel.org/linux-modules/
> a3701a9a-5b42-4581-a150-67d84601061c@suse.com
>
> [2] Link:
> https://lore.kernel.org/linux-modules/
> 458901be-1da8-4987-9c72-5aa3da6db15e@suse.com
>
> [3] Link:
> https://lore.kernel.org/linux-modules/
> 20250702-module-params-v3-v14-0-5b1cc32311af@kernel.org
>
> [4] Link:
> https://lore.kernel.org/linux-modules/
> Z3gDAnPlA3SZEbgl@bombadil.infradead.org
>
> Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
Nits: Link tags should be single lines and their indices at the end of
the tag, see Documentation/process/5.Posting.rst, section "Patch
formatting and changelogs". The second link would also be better if it
pointed to the initial email in the thread.
"""
Update Daniel Gomez's modules reviewer role to maintainer. This is
according to the plan [1][2][3] of scaling with more reviewers for modules
(for the incoming Rust support [4]) and rotate [5] every 6 months.
Link: https://lore.kernel.org/linux-modules/ZsPANzx4-5DrOl5m@bombadil.infradead.org [1]
Link: https://lore.kernel.org/linux-modules/20240821174021.2371547-1-mcgrof@kernel.org [2]
Link: https://lore.kernel.org/linux-modules/458901be-1da8-4987-9c72-5aa3da6db15e@suse.com [3]
Link: https://lore.kernel.org/linux-modules/20250702-module-params-v3-v14-0-5b1cc32311af@kernel.org [4]
Link: https://lore.kernel.org/linux-modules/Z3gDAnPlA3SZEbgl@bombadil.infradead.org [5]
Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
"""
You can update the patch on modules-next, no need to resend.
Acked-by: Petr Pavlu <petr.pavlu@suse.com>
--
Thanks
Petr
^ permalink raw reply
* Re: [PATCH 3/8] execmem: rework execmem_cache_free()
From: Mike Rapoport @ 2025-07-08 8:13 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Liam R. Howlett, Andrew Morton, Andy Lutomirski, Borislav Petkov,
Daniel Gomez, Dave Hansen, Ingo Molnar, Luis Chamberlain,
Mark Rutland, Masami Hiramatsu, H. Peter Anvin, Petr Pavlu,
Sami Tolvanen, Steven Rostedt, Thomas Gleixner, linux-kernel,
linux-mm, linux-modules, linux-trace-kernel, x86
In-Reply-To: <20250708072649.GC1613376@noisy.programming.kicks-ass.net>
On Tue, Jul 08, 2025 at 09:26:49AM +0200, Peter Zijlstra wrote:
> On Mon, Jul 07, 2025 at 06:12:26PM +0300, Mike Rapoport wrote:
> > On Mon, Jul 07, 2025 at 11:06:25AM -0400, Liam R. Howlett wrote:
> > > * Mike Rapoport <rppt@kernel.org> [250707 07:32]:
> > > > On Mon, Jul 07, 2025 at 01:11:02PM +0200, Peter Zijlstra wrote:
> > > > >
> > > > > err = __execmem_cache_free(&mas, ptr, GFP_KERNEL | __GFP_NORETRY);
> > > > > if (err) {
> > > > > mas_store_gfp(&mas, pending_free_set(ptr), GFP_KERNEL);
> > > > > execmem_cache.pending_free_cnt++;
> > > > > schedule_delayed_work(&execmem_cache_free_work, FREE_DELAY);
> > > > > return true;
> > > > > }
> > > > >
> > > > > schedule_work(&execmem_cache_clean_work);
> > > > > return true;
> > > > > }
> > > > >
> > > > > And now I have to ask what happens if mas_store_gfp() returns an error?
> > > >
> > > > AFAIU it won't. mas points to exact slot we've got the area from, nothing else
> > > > can modify the tree because of the mutex, so that mas_store_gfp()
> > > > essentially updates the value at an existing entry.
> > > >
> > > > I'll add a comment about it.
> > > >
> > > > Added @Liam to make sure I'm not saying nonsense :)
> > > >
> > >
> > > Yes, if there is already a node with a value with the same range, there
> > > will be no allocations that will happen, so it'll just change the
> > > pointer for you. This is a slot store operation.
> > >
> > > But, if it's possible to have no entries (an empty tree, or a single
> > > value at 0), you will most likely allocate a node to store it, which is
> > > 256B.
> > >
> > > I don't think this is a concern in this particular case though as you
> > > are searching for an entry and storing, so it needs to exist. So
> > > really, the only scenario here is if you store 1 - ULONG_MAX (without
> > > having expanded a root node) or 0 - ULONG_MAX, and that seems invalid.
> >
> > Thanks for clarification, Liam!
> > The tree cannot be empty at that point and if it has a single value, it
> > won't be at 0, I'm quite sure no architecture has execmem areas at 0.
>
> Would it make sense to have something like GFP_NO_ALLOC to pass to
> functions like this where we know it won't actually allocate -- and
> which when it does reach the allocator generates a WARN and returns NULL
> ?
We can add a WARN at the caller as well, that won't require a new gfp flag.
The question is how to recover if such thing happen, I don't really see
what execmem can do here if mas_store_gfp() returns an error :/
--
Sincerely yours,
Mike.
^ permalink raw reply
* Re: [PATCH 1/8] execmem: drop unused execmem_update_copy()
From: Mike Rapoport @ 2025-07-08 8:22 UTC (permalink / raw)
To: Christophe Leroy
Cc: Andrew Morton, Andy Lutomirski, Borislav Petkov, Daniel Gomez,
Dave Hansen, Ingo Molnar, Luis Chamberlain, Mark Rutland,
Masami Hiramatsu, H. Peter Anvin, Peter Zijlstra, Petr Pavlu,
Sami Tolvanen, Steven Rostedt, Thomas Gleixner, linux-kernel,
linux-mm, linux-modules, linux-trace-kernel, x86
In-Reply-To: <2ea9c28f-c3d1-4837-b000-10eccaa2775b@csgroup.eu>
On Mon, Jul 07, 2025 at 03:02:15PM +0200, Christophe Leroy wrote:
>
>
> Le 07/07/2025 à 13:49, Mike Rapoport a écrit :
> > On Mon, Jul 07, 2025 at 12:10:43PM +0200, Christophe Leroy wrote:
> > >
> > > Le 04/07/2025 à 15:49, Mike Rapoport a écrit :
> > > > From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
> > > >
> > > > The execmem_update_copy() that used text poking was required when memory
> > > > allocated from ROX cache was always read-only. Since now its permissions
> > > > can be switched to read-write there is no need in a function that updates
> > > > memory with text poking.
> > >
> > > Erm. Looks like I missed the patch that introduced this change.
> > >
> > > On some variant of powerpc, namely book3s/32, this is not feasible.
> >
> > The only user of EXECMEM_ROX_CACHE for now is x86-64, we can always revisit
> > when powerpc book3s/32 would want to opt in to cache usage.
> >
> > And it seems that [MODULES_VADDR, MODULES_END] is already mapped with
> > "large pages", isn't it?
>
> I don't think so. It uses execmem_alloc() which sets VM_ALLOW_HUGE_VMAP only
> when using EXECMEM_ROX_CACHE. And book3s/32 doesn't have large pages.
>
> Only 8xx has large pages but they are not PMD aligned (PMD_SIZE is 4M while
> large pages are 512k and 8M) so it wouldn't work well with existing
> execmem_vmalloc().
The PMD_SIZE can be replaced with one of arch_vmap size helpers if needed.
Or even parametrized in execmem_info.
> > > The granularity for setting the NX (non exec) bit is 256 Mbytes sections.
> > > So the area dedicated to execmem [MODULES_VADDR; MODULES_END[ always have
> > > the NX bit unset.
> > >
> > > You can change any page within this area from ROX to RWX but you can't make
> > > it RW without X. If you want RW without X you must map it in the VMALLOC
> > > area, as VMALLOC area have NX bit always set.
> >
> > So what will happen when one callse
> >
> > set_memory_nx()
> > set_memory_rw()
> >
> > in such areas?
>
> Nothing will happen. It will successfully unset the X bit on the PTE but
> that will be ignored by the HW which only relies on the segment's NX bit
> which is set for the entire VMALLOC area and unset for the entire MODULE
> area.
And set_memory_rw() will essentially make the mapping RWX if it's in MODULE
area?
> Christophe
>
--
Sincerely yours,
Mike.
^ permalink raw reply
* Re: [PATCH v15 1/7] rust: sync: add `SetOnce`
From: Andreas Hindborg @ 2025-07-08 8:47 UTC (permalink / raw)
To: Alice Ryhl
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Masahiro Yamada, Nathan Chancellor,
Luis Chamberlain, Danilo Krummrich, Benno Lossin, Nicolas Schier,
Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
In-Reply-To: <CAH5fLgiKo=jN_V5cAe_AJqxxp7mQWqhKx7knkEj6js3yiU9sqA@mail.gmail.com>
"Alice Ryhl" <aliceryhl@google.com> writes:
> On Mon, Jul 7, 2025 at 3:32 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Introduce the `SetOnce` type, a container that can only be written once.
>> The container uses an internal atomic to synchronize writes to the internal
>> value.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> LGTM:
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>
>> +impl<T> Drop for SetOnce<T> {
>> + fn drop(&mut self) {
>> + if self.init.load(Acquire) == 2 {
>> + // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
>> + // contains a valid value. We have exclusive access, as we hold a `mut` reference to
>> + // `self`.
>> + unsafe { drop_in_place(self.value.get()) };
>
> This load does not need to be Acquire. It can be a Relaxed load or
> even an unsynchronized one since the access is exclusive.
Right, that is actually very cool. My rationale was that if a reference
has been shared to another thread of execution, we would need to
synchronize here to see a possible initialization from that other
thread. But I guess it is impossible to end the lifetime of a reference
without doing a synchronization somewhere else.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v15 1/7] rust: sync: add `SetOnce`
From: Andreas Hindborg @ 2025-07-08 8:54 UTC (permalink / raw)
To: Boqun Feng
Cc: Alice Ryhl, Miguel Ojeda, Alex Gaynor, Gary Guo,
Björn Roy Baron, Masahiro Yamada, Nathan Chancellor,
Luis Chamberlain, Danilo Krummrich, Benno Lossin, Nicolas Schier,
Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
In-Reply-To: <aGvkFbs5caxLSQxa@Mac.home>
"Boqun Feng" <boqun.feng@gmail.com> writes:
> On Mon, Jul 07, 2025 at 03:38:58PM +0200, Alice Ryhl wrote:
>> On Mon, Jul 7, 2025 at 3:32 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> >
>> > Introduce the `SetOnce` type, a container that can only be written once.
>> > The container uses an internal atomic to synchronize writes to the internal
>> > value.
>> >
>> > Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>
>> LGTM:
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>>
>> > +impl<T> Drop for SetOnce<T> {
>> > + fn drop(&mut self) {
>> > + if self.init.load(Acquire) == 2 {
>> > + // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
>> > + // contains a valid value. We have exclusive access, as we hold a `mut` reference to
>> > + // `self`.
>> > + unsafe { drop_in_place(self.value.get()) };
>>
>> This load does not need to be Acquire. It can be a Relaxed load or
>> even an unsynchronized one since the access is exclusive.
>
> Right, I think we can do the similar as Revocable here:
>
> if *self.init.get_mut() == 2 { }
>
> Further, with my following Benno's suggestion and making `Atomic<T>` an
> `UnsafeCell<T>:
>
> https://lore.kernel.org/rust-for-linux/aGhh-TvNOWhkt0JG@Mac.home/
>
> compiler can generate a noalias reference here, which allows further
> optimization.
>
You would like to remove `PhantomPinned` to enable noalias? I guess that
makes sense in this case. I'll fix that for next spin.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v15 2/7] rust: str: add radix prefixed integer parsing functions
From: Benno Lossin @ 2025-07-08 8:55 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
In-Reply-To: <20250707-module-params-v3-v15-2-c1f4269a57b9@kernel.org>
On Mon Jul 7, 2025 at 3:29 PM CEST, Andreas Hindborg wrote:
> Add the trait `ParseInt` for parsing string representations of integers
> where the string representations are optionally prefixed by a radix
> specifier. Implement the trait for the primitive integer types.
>
> Tested-by: Daniel Gomez <da.gomez@samsung.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Suggested-by: Benno Lossin <benno.lossin@proton.me>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
Reviewed-by: Benno Lossin <lossin@kernel.org>
---
Cheers,
Benno
> ---
> rust/kernel/str.rs | 2 +
> rust/kernel/str/parse_int.rs | 148 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 150 insertions(+)
^ permalink raw reply
* Re: [PATCH v15 1/7] rust: sync: add `SetOnce`
From: Alice Ryhl @ 2025-07-08 9:00 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Masahiro Yamada, Nathan Chancellor,
Luis Chamberlain, Danilo Krummrich, Benno Lossin, Nicolas Schier,
Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
In-Reply-To: <87cyaboypx.fsf@kernel.org>
On Tue, Jul 8, 2025 at 10:48 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Alice Ryhl" <aliceryhl@google.com> writes:
>
> > On Mon, Jul 7, 2025 at 3:32 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >> Introduce the `SetOnce` type, a container that can only be written once.
> >> The container uses an internal atomic to synchronize writes to the internal
> >> value.
> >>
> >> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> >
> > LGTM:
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> >
> >> +impl<T> Drop for SetOnce<T> {
> >> + fn drop(&mut self) {
> >> + if self.init.load(Acquire) == 2 {
> >> + // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
> >> + // contains a valid value. We have exclusive access, as we hold a `mut` reference to
> >> + // `self`.
> >> + unsafe { drop_in_place(self.value.get()) };
> >
> > This load does not need to be Acquire. It can be a Relaxed load or
> > even an unsynchronized one since the access is exclusive.
>
> Right, that is actually very cool. My rationale was that if a reference
> has been shared to another thread of execution, we would need to
> synchronize here to see a possible initialization from that other
> thread. But I guess it is impossible to end the lifetime of a reference
> without doing a synchronization somewhere else.
Yup, a mutable reference generally implies synchronization.
Alice
^ permalink raw reply
* Re: [PATCH v15 1/7] rust: sync: add `SetOnce`
From: Benno Lossin @ 2025-07-08 9:02 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier
Cc: Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
In-Reply-To: <20250707-module-params-v3-v15-1-c1f4269a57b9@kernel.org>
On Mon Jul 7, 2025 at 3:29 PM CEST, Andreas Hindborg wrote:
> Introduce the `SetOnce` type, a container that can only be written once.
> The container uses an internal atomic to synchronize writes to the internal
> value.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
One nit and a safety comment fix below. (feel free to ignore the nit)
With the safety comment fixed:
Reviewed-by: Benno Lossin <lossin@kernel.org>
> ---
> rust/kernel/sync.rs | 2 +
> rust/kernel/sync/set_once.rs | 125 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 127 insertions(+)
>
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index 81e3a806e57e2..13e6bc7fa87ac 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -18,6 +18,7 @@
> mod locked_by;
> pub mod poll;
> pub mod rcu;
> +mod set_once;
I would have named this `once`.
> pub use arc::{Arc, ArcBorrow, UniqueArc};
> pub use completion::Completion;
> + /// Get a reference to the contained object.
> + ///
> + /// Returns [`None`] if this [`SetOnce`] is empty.
> + pub fn as_ref(&self) -> Option<&T> {
> + if self.init.load(Acquire) == 2 {
> + // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
> + // contains a valid value.
And the type invariants also ensure that the value of `self.init`
doesn't change.
So probably
// SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
// contains a valid value. They also guarantee that `self.init` doesn't change.
If you come up with something better, feel free to use it.
---
Cheers,
Benno
> + Some(unsafe { &*self.value.get() })
> + } else {
> + None
> + }
> + }
^ permalink raw reply
* Re: [PATCH v15 1/7] rust: sync: add `SetOnce`
From: Benno Lossin @ 2025-07-08 9:07 UTC (permalink / raw)
To: Andreas Hindborg, Boqun Feng
Cc: Alice Ryhl, Miguel Ojeda, Alex Gaynor, Gary Guo,
Björn Roy Baron, Masahiro Yamada, Nathan Chancellor,
Luis Chamberlain, Danilo Krummrich, Nicolas Schier, Trevor Gross,
Adam Bratschi-Kaye, rust-for-linux, linux-kernel, linux-kbuild,
Petr Pavlu, Sami Tolvanen, Daniel Gomez, Simona Vetter, Greg KH,
Fiona Behrens, Daniel Almeida, linux-modules
In-Reply-To: <877c0joyfo.fsf@kernel.org>
On Tue Jul 8, 2025 at 10:54 AM CEST, Andreas Hindborg wrote:
> "Boqun Feng" <boqun.feng@gmail.com> writes:
>
>> On Mon, Jul 07, 2025 at 03:38:58PM +0200, Alice Ryhl wrote:
>>> On Mon, Jul 7, 2025 at 3:32 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>> >
>>> > Introduce the `SetOnce` type, a container that can only be written once.
>>> > The container uses an internal atomic to synchronize writes to the internal
>>> > value.
>>> >
>>> > Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>>
>>> LGTM:
>>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>>>
>>> > +impl<T> Drop for SetOnce<T> {
>>> > + fn drop(&mut self) {
>>> > + if self.init.load(Acquire) == 2 {
>>> > + // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
>>> > + // contains a valid value. We have exclusive access, as we hold a `mut` reference to
>>> > + // `self`.
>>> > + unsafe { drop_in_place(self.value.get()) };
>>>
>>> This load does not need to be Acquire. It can be a Relaxed load or
>>> even an unsynchronized one since the access is exclusive.
>>
>> Right, I think we can do the similar as Revocable here:
>>
>> if *self.init.get_mut() == 2 { }
>>
>> Further, with my following Benno's suggestion and making `Atomic<T>` an
>> `UnsafeCell<T>:
>>
>> https://lore.kernel.org/rust-for-linux/aGhh-TvNOWhkt0JG@Mac.home/
>>
>> compiler can generate a noalias reference here, which allows further
>> optimization.
>>
>
> You would like to remove `PhantomPinned` to enable noalias? I guess that
> makes sense in this case. I'll fix that for next spin.
I think you two are talking about different things. Boqun is saying that
the `Atomic<T>` will use `UnsafeCell` rather than `Opaque`, which will
potentially allow more optimizations.
But you are talking about `SetOnce`, right? I think it makes more sense
for `SetOnce` to use `UnsafeCell<MaybeUninit<T>>` rather than `Opaque`
too. So feel free to change it in the next version.
---
Cheers,
Benno
^ permalink raw reply
* Re: [PATCH 2/3] module: make structure definitions always visible
From: Petr Pavlu @ 2025-07-08 9:32 UTC (permalink / raw)
To: Daniel Gomez
Cc: Thomas Weißschuh, Luis Chamberlain, Sami Tolvanen,
Daniel Gomez, Brendan Higgins, David Gow, Rae Moar, linux-modules,
linux-kernel, linux-kselftest, kunit-dev
In-Reply-To: <a9eade27-9b77-431f-b7c8-24c3fb891673@kernel.org>
On 7/7/25 9:11 PM, Daniel Gomez wrote:
> On 12/06/2025 16.53, Thomas WeiÃschuh wrote:
>> To write code that works with both CONFIG_MODULES=y and CONFIG_MODULES=n
>> it is convenient to use "if (IS_ENABLED(CONFIG_MODULES))" over raw #ifdef.
>> The code will still fully typechecked but the unreachable parts are
>> discarded by the compiler. This prevents accidental breakage when a certain
>> kconfig combination was not specifically tested by the developer.
>> This pattern is already supported to some extend by module.h defining
>> empty stub functions if CONFIG_MODULES=n.
>> However some users of module.h work on the structured defined by module.h.
>>
>> Therefore these structure definitions need to be visible, too.
>
> We are missing here which structures are needed. + we are making more things
> visible than what we actually need.
>
>>
>> Many structure members are still gated by specific configuration settings.
>> The assumption for those is that the code using them will be gated behind
>> the same configuration setting anyways.
>
> I think code and kconfig need to reflect the actual dependencies. For example,
> if CONFIG_LIVEPATCH depends on CONFIG_MODULES, we need to specify that in
> Kconfig with depends on, as well as keep the code gated by these 2 configs with
> ifdef/IS_ENABLED.
>
>>
>> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>> ---
>> include/linux/module.h | 23 ++++++++++++-----------
>> 1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index 52f7b0487a2733c56e2531a434887e56e1bf45b2..7f783e71636542b99db3dd869a9387d14992df45 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -302,17 +302,6 @@ static typeof(name) __mod_device_table__##type##__##name \
>>
>> struct notifier_block;
>>
>> -#ifdef CONFIG_MODULES
>> -
>> -extern int modules_disabled; /* for sysctl */
>> -/* Get/put a kernel symbol (calls must be symmetric) */
>> -void *__symbol_get(const char *symbol);
>> -void *__symbol_get_gpl(const char *symbol);
>> -#define symbol_get(x) ({ \
>> - static const char __notrim[] \
>> - __used __section(".no_trim_symbol") = __stringify(x); \
>> - (typeof(&x))(__symbol_get(__stringify(x))); })
>> -
>> enum module_state {
>> MODULE_STATE_LIVE, /* Normal state. */
>> MODULE_STATE_COMING, /* Full formed, running module_init. */
>> @@ -598,6 +587,18 @@ struct module {
>> struct _ddebug_info dyndbg_info;
>> #endif
>> } ____cacheline_aligned __randomize_layout;
>> +
>> +#ifdef CONFIG_MODULES
>> +
>> +extern int modules_disabled; /* for sysctl */
>> +/* Get/put a kernel symbol (calls must be symmetric) */
>> +void *__symbol_get(const char *symbol);
>> +void *__symbol_get_gpl(const char *symbol);
>> +#define symbol_get(x) ({ \
>> + static const char __notrim[] \
>> + __used __section(".no_trim_symbol") = __stringify(x); \
>> + (typeof(&x))(__symbol_get(__stringify(x))); })
>> +
>
> The patch exposes data structures that are not needed. + breaks the
> config dependencies.
>
> For example, before this patch:
>
> #ifdef CONFIG_MODULES
>
> {...}
>
> struct mod_tree_node {
>
> {...}
>
> struct module_memory {
> void *base;
> bool is_rox;
> unsigned int size;
>
> #ifdef CONFIG_MODULES_TREE_LOOKUP
> struct mod_tree_node mtn;
> #endif
> };
>
> {...}
> #endif /* CONFIG_MODULES */
>
> After the patch, mod_tree_node is not needed externally. And the mtn field
> in module_memory is exposed only under MODULES_TREE_LOOKUP and not MODULES
> + MODULES_TREE_LOOKUP.
>
> I general, I see the issues I mentioned with LIVEPATCH, mod_tree_node, macros,
> and LOOKUP.
I think the idea is that having unnecessary structures in header files
isn't particularly harmful, as they won't affect the resulting binary.
On the other hand, they can help with type checking of conditional code
as shown by patch #3.
This is different compared to "extern int modules_disabled;" and
"void *__symbol_get(const char *symbol);" which the patch correctly
still protects by '#ifdef CONFIG_MODULES'. Not hiding them could result
in successful compilation but an error only at link time.
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH 2/3] module: make structure definitions always visible
From: Petr Pavlu @ 2025-07-08 9:39 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Luis Chamberlain, Sami Tolvanen, Daniel Gomez, Brendan Higgins,
David Gow, Rae Moar, linux-modules, linux-kernel, linux-kselftest,
kunit-dev
In-Reply-To: <20250612-kunit-ifdef-modules-v1-2-fdccd42dcff8@linutronix.de>
On 6/12/25 4:53 PM, Thomas Weißschuh wrote:
> To write code that works with both CONFIG_MODULES=y and CONFIG_MODULES=n
> it is convenient to use "if (IS_ENABLED(CONFIG_MODULES))" over raw #ifdef.
> The code will still fully typechecked but the unreachable parts are
> discarded by the compiler. This prevents accidental breakage when a certain
> kconfig combination was not specifically tested by the developer.
> This pattern is already supported to some extend by module.h defining
> empty stub functions if CONFIG_MODULES=n.
> However some users of module.h work on the structured defined by module.h.
>
> Therefore these structure definitions need to be visible, too.
>
> Many structure members are still gated by specific configuration settings.
> The assumption for those is that the code using them will be gated behind
> the same configuration setting anyways.
>
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
> include/linux/module.h | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 52f7b0487a2733c56e2531a434887e56e1bf45b2..7f783e71636542b99db3dd869a9387d14992df45 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -302,17 +302,6 @@ static typeof(name) __mod_device_table__##type##__##name \
>
> struct notifier_block;
>
> -#ifdef CONFIG_MODULES
> -
> -extern int modules_disabled; /* for sysctl */
> -/* Get/put a kernel symbol (calls must be symmetric) */
> -void *__symbol_get(const char *symbol);
> -void *__symbol_get_gpl(const char *symbol);
> -#define symbol_get(x) ({ \
> - static const char __notrim[] \
> - __used __section(".no_trim_symbol") = __stringify(x); \
> - (typeof(&x))(__symbol_get(__stringify(x))); })
> -
> enum module_state {
> MODULE_STATE_LIVE, /* Normal state. */
> MODULE_STATE_COMING, /* Full formed, running module_init. */
> @@ -598,6 +587,18 @@ struct module {
> struct _ddebug_info dyndbg_info;
> #endif
> } ____cacheline_aligned __randomize_layout;
> +
> +#ifdef CONFIG_MODULES
> +
> +extern int modules_disabled; /* for sysctl */
> +/* Get/put a kernel symbol (calls must be symmetric) */
> +void *__symbol_get(const char *symbol);
> +void *__symbol_get_gpl(const char *symbol);
> +#define symbol_get(x) ({ \
> + static const char __notrim[] \
> + __used __section(".no_trim_symbol") = __stringify(x); \
> + (typeof(&x))(__symbol_get(__stringify(x))); })
> +
> #ifndef MODULE_ARCH_INIT
> #define MODULE_ARCH_INIT {}
> #endif
>
Nit: I suggest keeping MODULE_ARCH_INIT in its current position,
immediately after the 'struct module' declaration, because the macro is
directly tied to that structure.
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH 1/2] module: Restrict module namespace access to in-tree modules
From: Masahiro Yamada @ 2025-07-08 12:41 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Matthias Maennich, Jonathan Corbet, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Nathan Chancellor, Nicolas Schier,
Alexander Viro, Christian Brauner, Jan Kara, Christoph Hellwig,
Peter Zijlstra, David Hildenbrand, Shivank Garg,
Jiri Slaby (SUSE), Stephen Rothwell, linux-doc, linux-kernel,
linux-modules, linux-kbuild, linux-fsdevel
In-Reply-To: <20250708-export_modules-v1-1-fbf7a282d23f@suse.cz>
On Tue, Jul 8, 2025 at 4:29 PM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> The module namespace support has been introduced to allow restricting
> exports to specific modules only, and intended for in-tree modules such
> as kvm. Make this intention explicit by disallowing out of tree modules
> both for the module loader and modpost.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
In my understanding, an external module with the same name
can override the internal one.
This change disallows such a use-case.
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH 1/2] module: Restrict module namespace access to in-tree modules
From: Petr Pavlu @ 2025-07-08 13:03 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Matthias Maennich, Jonathan Corbet, Luis Chamberlain,
Sami Tolvanen, Daniel Gomez, Masahiro Yamada, Nathan Chancellor,
Nicolas Schier, Alexander Viro, Christian Brauner, Jan Kara,
Christoph Hellwig, Peter Zijlstra, David Hildenbrand,
Shivank Garg, Jiri Slaby (SUSE), Stephen Rothwell, linux-doc,
linux-kernel, linux-modules, linux-kbuild, linux-fsdevel
In-Reply-To: <20250708-export_modules-v1-1-fbf7a282d23f@suse.cz>
On 7/8/25 9:28 AM, Vlastimil Babka wrote:
> The module namespace support has been introduced to allow restricting
> exports to specific modules only, and intended for in-tree modules such
> as kvm. Make this intention explicit by disallowing out of tree modules
> both for the module loader and modpost.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
> [...]
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 413ac6ea37021bc8ae260f624ca2745ed85333fc..ec7d8daa0347e3b65713396d6b6d14c2cb0270d3 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -1157,7 +1157,8 @@ static int verify_namespace_is_imported(const struct load_info *info,
> namespace = kernel_symbol_namespace(sym);
> if (namespace && namespace[0]) {
>
> - if (verify_module_namespace(namespace, mod->name))
> + if (get_modinfo(info, "intree") &&
> + verify_module_namespace(namespace, mod->name))
> return 0;
>
> for_each_modinfo_entry(imported_namespace, info, "import_ns") {
I'd rather avoid another walk of the modinfo data in
verify_namespace_is_imported(). I suggest checking whether mod->taints
has TAINT_OOT_MODULE set instead, which should provide the same
information. The symbol resolution already relies on the taint flags, so
this is consistent with the rest of the code.
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH v15 1/7] rust: sync: add `SetOnce`
From: Andreas Hindborg @ 2025-07-08 13:06 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <DB6JVOZLCMBL.3EZQJP50UUB86@kernel.org>
"Benno Lossin" <lossin@kernel.org> writes:
> On Mon Jul 7, 2025 at 3:29 PM CEST, Andreas Hindborg wrote:
>> Introduce the `SetOnce` type, a container that can only be written once.
>> The container uses an internal atomic to synchronize writes to the internal
>> value.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> One nit and a safety comment fix below. (feel free to ignore the nit)
> With the safety comment fixed:
>
> Reviewed-by: Benno Lossin <lossin@kernel.org>
>
>> ---
>> rust/kernel/sync.rs | 2 +
>> rust/kernel/sync/set_once.rs | 125 +++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 127 insertions(+)
>>
>> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
>> index 81e3a806e57e2..13e6bc7fa87ac 100644
>> --- a/rust/kernel/sync.rs
>> +++ b/rust/kernel/sync.rs
>> @@ -18,6 +18,7 @@
>> mod locked_by;
>> pub mod poll;
>> pub mod rcu;
>> +mod set_once;
>
> I would have named this `once`.
So module `once` and struct `SetOnce`? Struct name `Once` would lead
thoughts to `std::sync::Once`, which is a different thing.
>
>> pub use arc::{Arc, ArcBorrow, UniqueArc};
>> pub use completion::Completion;
>
>> + /// Get a reference to the contained object.
>> + ///
>> + /// Returns [`None`] if this [`SetOnce`] is empty.
>> + pub fn as_ref(&self) -> Option<&T> {
>> + if self.init.load(Acquire) == 2 {
>> + // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
>> + // contains a valid value.
>
> And the type invariants also ensure that the value of `self.init`
> doesn't change.
>
> So probably
>
> // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
> // contains a valid value. They also guarantee that `self.init` doesn't change.
>
Sure 👍
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v15 0/7] rust: extend `module!` macro with integer parameter support
From: Andreas Hindborg @ 2025-07-08 13:09 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Benno Lossin, Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye,
rust-for-linux, linux-kernel, linux-kbuild, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Simona Vetter, Greg KH,
Fiona Behrens, Daniel Almeida, linux-modules
In-Reply-To: <CANiq72k5mU7cACFh8TZC1+NfUQpdrZBToZ-jd75DfCNTfMU1fA@mail.gmail.com>
"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:
> On Mon, Jul 7, 2025 at 3:31 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Based on code by Adam Bratschi-Kaye lifted from the original `rust` branch [1].
>
> I would suggest adding this sort of note to the commit messages,
> especially since the commits have no Co-developed-by/Link tags
> otherwise.
>
> From our discussion in Zulip: the code itself is older than that the
> merge above. I think you wanted an example sentence for this --
> something simple could be e.g.
>
> Based on the original module parameter support by Miguel [1],
> later extended and generalized by Adam for more types [2][3].
> Originally tracked at [4].
>
> Link: https://github.com/Rust-for-Linux/linux/pull/7 [1]
> Link: https://github.com/Rust-for-Linux/linux/pull/82 [2]
> Link: https://github.com/Rust-for-Linux/linux/pull/87 [3]
> Link: https://github.com/Rust-for-Linux/linux/issues/11 [4]
>
> By the way, I guess you should inherit that issue in the last link :)
> It had some details about things we may or may not want to support
> etc. that I looked up back then. If you prefer that we close it or
> that we create sub-issues, that is fine -- up to you!
>
> Finally, if you end up adding strings, please link to Adam's
> https://github.com/Rust-for-Linux/linux/pull/110.
Thanks, I'll add this!
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v15 1/7] rust: sync: add `SetOnce`
From: Benno Lossin @ 2025-07-08 14:19 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <87v7o2omqf.fsf@kernel.org>
On Tue Jul 8, 2025 at 3:06 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Mon Jul 7, 2025 at 3:29 PM CEST, Andreas Hindborg wrote:
>>> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
>>> index 81e3a806e57e2..13e6bc7fa87ac 100644
>>> --- a/rust/kernel/sync.rs
>>> +++ b/rust/kernel/sync.rs
>>> @@ -18,6 +18,7 @@
>>> mod locked_by;
>>> pub mod poll;
>>> pub mod rcu;
>>> +mod set_once;
>>
>> I would have named this `once`.
>
> So module `once` and struct `SetOnce`? Struct name `Once` would lead
> thoughts to `std::sync::Once`, which is a different thing.
Hmm I thought that `Once` and `SetOnce` would live in the same module,
but if they don't then I think it's better to keep the `set_once`
module as-is.
---
Cheers,
Benno
^ permalink raw reply
* Re: [PATCH 1/2] module: Restrict module namespace access to in-tree modules
From: Vlastimil Babka @ 2025-07-08 15:08 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Matthias Maennich, Jonathan Corbet, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Nathan Chancellor, Nicolas Schier,
Alexander Viro, Christian Brauner, Jan Kara, Christoph Hellwig,
Peter Zijlstra, David Hildenbrand, Shivank Garg,
Jiri Slaby (SUSE), Stephen Rothwell, linux-doc, linux-kernel,
linux-modules, linux-kbuild, linux-fsdevel
In-Reply-To: <CAK7LNATpQrHX_8x4WvhDN7cODCCLr8kihydtfM-6wxhY17xtQw@mail.gmail.com>
On 7/8/25 14:41, Masahiro Yamada wrote:
> On Tue, Jul 8, 2025 at 4:29 PM Vlastimil Babka <vbabka@suse.cz> wrote:
>>
>> The module namespace support has been introduced to allow restricting
>> exports to specific modules only, and intended for in-tree modules such
>> as kvm. Make this intention explicit by disallowing out of tree modules
>> both for the module loader and modpost.
>>
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>
>
>
> In my understanding, an external module with the same name
> can override the internal one.
>
> This change disallows such a use-case.
Hmm I'm not familiar with this, but for such cases to be legitimate we can
assume the external module has to be derived from the internal one and not
something completely unrelated impersonating the internal one? So in that
case just patch 2 alone would be sufficient and not break any legitimate use
cases?
^ permalink raw reply
* Re: [PATCH 1/2] module: Restrict module namespace access to in-tree modules
From: Masahiro Yamada @ 2025-07-08 15:35 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Matthias Maennich, Jonathan Corbet, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Nathan Chancellor, Nicolas Schier,
Alexander Viro, Christian Brauner, Jan Kara, Christoph Hellwig,
Peter Zijlstra, David Hildenbrand, Shivank Garg,
Jiri Slaby (SUSE), Stephen Rothwell, linux-doc, linux-kernel,
linux-modules, linux-kbuild, linux-fsdevel
In-Reply-To: <39bed180-e21c-4801-8ac4-ba40b57f6df2@suse.cz>
On Wed, Jul 9, 2025 at 12:08 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 7/8/25 14:41, Masahiro Yamada wrote:
> > On Tue, Jul 8, 2025 at 4:29 PM Vlastimil Babka <vbabka@suse.cz> wrote:
> >>
> >> The module namespace support has been introduced to allow restricting
> >> exports to specific modules only, and intended for in-tree modules such
> >> as kvm. Make this intention explicit by disallowing out of tree modules
> >> both for the module loader and modpost.
> >>
> >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> >
> >
> >
> > In my understanding, an external module with the same name
> > can override the internal one.
> >
> > This change disallows such a use-case.
>
> Hmm I'm not familiar with this, but for such cases to be legitimate we can
> assume the external module has to be derived from the internal one and not
> something completely unrelated impersonating the internal one? So in that
> case just patch 2 alone would be sufficient and not break any legitimate use
> cases?
>
IIRC, nvdimm uses this feature for testing.
In-tree drivers:
drivers/nvdimm/Makefile
Out-of-tree drivers:
tools/testing/nvdimm/Makefile
tools/testing/nvdimm/Kbuild
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH 1/2] module: Restrict module namespace access to in-tree modules
From: Nicolas Schier @ 2025-07-08 19:22 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Matthias Maennich, Jonathan Corbet, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Masahiro Yamada, Nathan Chancellor,
Nicolas Schier, Alexander Viro, Christian Brauner, Jan Kara,
Christoph Hellwig, Peter Zijlstra, David Hildenbrand,
Shivank Garg, Jiri Slaby (SUSE), Stephen Rothwell, linux-doc,
linux-kernel, linux-modules, linux-kbuild, linux-fsdevel
In-Reply-To: <20250708-export_modules-v1-1-fbf7a282d23f@suse.cz>
On Tue, Jul 08, 2025 at 09:28:57AM +0200 Vlastimil Babka wrote:
> The module namespace support has been introduced to allow restricting
> exports to specific modules only, and intended for in-tree modules such
> as kvm. Make this intention explicit by disallowing out of tree modules
> both for the module loader and modpost.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
> Documentation/core-api/symbol-namespaces.rst | 5 +++--
> kernel/module/main.c | 3 ++-
> scripts/mod/modpost.c | 6 +++++-
> 3 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/core-api/symbol-namespaces.rst b/Documentation/core-api/symbol-namespaces.rst
> index 32fc73dc5529e8844c2ce2580987155bcd13cd09..dc228ac738a5cdc49cc736c29170ca96df6a28dc 100644
> --- a/Documentation/core-api/symbol-namespaces.rst
> +++ b/Documentation/core-api/symbol-namespaces.rst
> @@ -83,13 +83,14 @@ Symbols exported using this macro are put into a module namespace. This
> namespace cannot be imported.
>
> The macro takes a comma separated list of module names, allowing only those
> -modules to access this symbol. Simple tail-globs are supported.
> +modules to access this symbol. The access is restricted to in-tree modules.
> +Simple tail-globs are supported.
>
> For example::
>
> EXPORT_SYMBOL_GPL_FOR_MODULES(preempt_notifier_inc, "kvm,kvm-*")
>
> -will limit usage of this symbol to modules whoes name matches the given
> +will limit usage of this symbol to in-tree modules whoes name matches the given
If you keep touching this line, might you fix the typo?
s/whoes/whose/
Kind regards,
Nicolas
^ permalink raw reply
* [GIT PULL] Modules fixes for v6.16-rc6
From: Daniel Gomez @ 2025-07-08 19:38 UTC (permalink / raw)
To: Linus Torvalds, Luis Chamberlain, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, linux-modules
Cc: Sebastian Andrzej Siewior, linux-kernel
The following changes since commit d7b8f8e20813f0179d8ef519541a3527e7661d3a:
Linux 6.16-rc5 (2025-07-06 14:10:26 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/modules/linux.git tags/modules-6.16-rc6.fixes
for you to fetch changes up to af1ccf546e5f2915fbbde26841db43a971d81cf3:
MAINTAINERS: update Daniel Gomez's role and email address (2025-07-08 20:52:30 +0200)
----------------------------------------------------------------
Modules fixes for 6.16-rc6
This includes 2 fixes: one introduced in the current release cycle (v6.16-rc1)
and another introduced back in v6.4-rc1. Both have been in linux-next since
next-250701 and have passed the modules ci tests using kdevops. Finally, as Petr
and Luis mentioned in previous pull requests, add myself (Daniel Gomez) to the
list of modules maintainers.
The first was reported by Intel's kernel test robot, and it addresses a crash
exposed by Sebastian's commit c50d295c37f2 ("rds: Use nested-BH locking for
rds_page_remainder") by allowing relocations for the per-CPU section even if it
lacks the SHF_ALLOC flag. Petr and Sebastian went down to the archive history
(before Git) and found the commit that broke it b3b91325f3c7 ("Don't relocate
non-allocated regions in modules.").
The second fix, reported and fixed by Petr (with additional cleanup), resolves a
memory leak by ensuring proper deallocation if module loading fails. We couldn't
find a reproducer other than forcing it manually or leveraging eBPF. So, I
tested it by enabling error injection in the codetag functions through the error
path that produces the leak and made it fail until execmem is unable to allocate
more memory.
Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
-----BEGIN PGP SIGNATURE-----
iQIzBAABCAAdFiEE73Ua4R8Pc+G5xjxTQJ6jxB8ZUfsFAmhta7AACgkQQJ6jxB8Z
UftulhAAiK78CFOdIcKbGKMD8tBGCHUT6IVCskY0E3aqDmhZiNEXhpmwKnOq56zo
ztbwg81Acc41t0Y283vgmZuEpJbzmS47qRYIZEhk5BuLdKl3Hn91Iyi3sdZLB9gD
VaYkpY0y4V3pWL5AM/9rtnLvY7PSLZG0bnAeS2fsOzxN5DIn2IIdD3WsU++96O+k
LHIik7dXcBQQKOukRClAI+cZiU2IGGG/FcDuMmANcsw+E4GKTBQunZiixTZOjdgi
e//NMYw1+Y5DJQ/i7C3Fl6+W+r/8/HPkRxyZ3Wt79WBZxwV/Hp5iTG9652vHKE1i
ybBDwlJkQC/EQsup4NKzDby38bPW1tdYwwK0dNbMRGxNICqNe7rq5fEG4WjTjXAA
H2O5VZ84Ys1SRx+uXeu83aog6fdiZ9DezGuaxFgrTRHUken2QvxJ/HTXe1BmMNY5
9BbHPrG22dp+dEDptSVaDWWMQZ6OP73gXrPYowQC8/FOm9NdBXVIH9hvvI7KRSf5
OC4A7nyZCDNW/Dwzc6yi3qAWMkKCR6IGMqN9vFiR3iIkaHJN1KABFzaVmh3JpR+B
6TsuO8/EIu9Ue25VgMehSj9LuGFgbhcfg40N2Fj/7+vgAM4Rd5sOw/NyiaD8xUPC
j0Ql4ehZiOQof8wAhdxpHFQQjO30UdpQgOmUO1yNUdmuPIpdSN0=
=dQvQ
-----END PGP SIGNATURE-----
----------------------------------------------------------------
Daniel Gomez (1):
MAINTAINERS: update Daniel Gomez's role and email address
Petr Pavlu (2):
module: Fix memory deallocation on error path in move_module()
module: Avoid unnecessary return value initialization in move_module()
Sebastian Andrzej Siewior (1):
module: Make sure relocations are applied to the per-CPU section
MAINTAINERS | 2 +-
kernel/module/main.c | 17 +++++++++++------
2 files changed, 12 insertions(+), 7 deletions(-)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox