* Jeff Xu <jeffxu@xxxxxxxxxxxx> [250213 12:17]:
> On Wed, Feb 12, 2025 at 7:05 AM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
> >
> ...
> > >
> > > In this version, we've improved the handling of system mapping sealing from
> > > previous versions, instead of modifying the _install_special_mapping
> > > function itself, which would affect all architectures, we now call
> > > _install_special_mapping with a sealing flag only within the specific
> > > architecture that requires it. This targeted approach offers two key
> > > advantages: 1) It limits the code change's impact to the necessary
> > > architectures, and 2) It aligns with the software architecture by keeping
> > > the core memory management within the mm layer, while delegating the
> > > decision of sealing system mappings to the individual architecture, which
> > > is particularly relevant since 32-bit architectures never require sealing.
> > >
> > > Additionally, this patch introduces a new header,
> > > include/linux/usrprocess.h, which contains the mseal_system_mappings()
> > > function. This function helps the architecture determine if system
> > > mapping is enabled within the current kernel configuration. It can be
> > > extended in the future to handle opt-in/out prctl for enabling system
> > > mapping sealing at the process level or a kernel cmdline feature.
> > >
> > > A new header file was introduced because it was difficult to find the
> > > best location for this function. Although include/linux/mm.h was
> > > considered, this feature is more closely related to user processes
> > > than core memory management. Additionally, future prctl or kernel
> > > cmd-line implementations for this feature would not fit within the
> > > scope of core memory management or mseal.c. This is relevant because
> > > if we add unit-tests for mseal.c in the future, we would want to limit
> > > mseal.c's dependencies to core memory management.
> > >
> ...
> > >
> > > diff --git a/include/linux/userprocess.h b/include/linux/userprocess.h
> > > new file mode 100644
> > > index 000000000000..bd11e2e972c5
> > > --- /dev/null
> > > +++ b/include/linux/userprocess.h
> > > @@ -0,0 +1,18 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef _LINUX_USER_PROCESS_H
> > > +#define _LINUX_USER_PROCESS_H
> > > +#include <linux/mm.h>
> >
> > Why is this in a new file and not mm.h directly? Anyone who needs to
> > use this will pull in mm.h anyways, and probably already has mm.h
> > included.
> >
> The commit message includes the reason. I've snipped and left the
> relevant portion for easy reference, please see the beginning of this
> email.
This is a _really_ good reason NOT to stack the entire patch set
description into a single patch description. Within the above 111 lines
of text, I missed that statement.
When Andrew takes the first patch, he'll put the patch description in
there, and then we can actually concentrate on the patch with the
limited context of what is going on.
If it doesn't go through Andrew's branch, then we can fall back to each
patch standing on its own with the change stating why things are done.
Also, I don't agree with your stated reason for a new header.
Please remove this header until it is needed. It can be added later if
it is needed. If we all had tiny headers because we might need them in
the future, we'd have serious issues finding anything and the list of
included headers would be huge.
You have added unnecessary changes and complications to this patch set
on v5.
>
> > > +
> > > +/*
> > > + * mseal of userspace process's system mappings.
> > > + */
> > > +static inline unsigned long mseal_system_mappings(void)
> > > +{
> > > +#ifdef CONFIG_MSEAL_SYSTEM_MAPPINGS
> > > + return VM_SEALED;
> > > +#else
> > > + return 0;
> > > +#endif
> > > +}
> > > +
> > > +#endif
> >
> > Looking in mm.h, there are examples of config checks which either set
> > the bit or set it to 0. This means you wouldn't need the function at
> > all and the precompiler would do all the work (although with a static
> > inline, I'm not sure it would make a big difference in instructions).
> >
> > For instance, you could do:
> > #ifdef CONFIG_64BIT
> > /* VM is sealed, in vm_flags */
> > #define VM_SEALED _BITUL(63)
> >
> > #ifdef CONFIG_MSEAL_SYSTEM_MAPPINGS
> > #define VM_SYSTEM_SEAL VM_SEALED
> > else
> > #define VM_SYSTEM_SEAL VM_NONE
> > #endif
> >
> > #else /* CONFIG_64BIT */
> > #define VM_SYSTEM_SEAL VM_NONE
> > #endif
> >
> > Then you can use VM_SYSTEM_SEAL in the system mappings function in the
> > list of flags instead of having a variable + calling the static
> > function.
> >
> > I didn't look close enough to see if the 32bit version is necessary.
> >
> I'm aware of the other pattern.
>
> VM_SEALED isn't defined in 32-bit systems, and mseal.c isn't part of
> the build. This is intentional. Any 32-bit code trying to use the
> sealing function or the VM_SEALED flag will immediately fail
> compilation. This makes it easier to identify incorrect usage.
So you are against using the #define because the VM_SYSTEM_SEAL will be
defined, even though it will be VM_NONE? This is no worse than a
function that returns 0, and it aligns better with what we have today in
that it can be put in the list of other flags.
Also, my vote for where you should put this code is clear: it should
live with the mseal #define in mm.h. You're going to need mm.h anyways,
and that's a big hint as to where it should live.
>
> > > diff --git a/init/Kconfig b/init/Kconfig
> > > index d0d021b3fa3b..892d2bcdf397 100644
> > > --- a/init/Kconfig
> > > +++ b/init/Kconfig
> > > @@ -1882,6 +1882,24 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS
> > > config ARCH_HAS_MEMBARRIER_SYNC_CORE
> > > bool
> > >
> > > +config ARCH_HAS_MSEAL_SYSTEM_MAPPINGS
> >
> > ARCH_SUPPORTS_MSEAL_SYSTEM_MAPPINGS is more clear. HAS may mean it's
> > supported or it could mean it's enabled. SUPPORTS is more clear that
> > this is set if an arch supports the feature. After all, I think HAS
> > here means it "has support for" mseal system mappings.
> >
> > I see that both are used (HAS and SUPPORTS), but I'm still not sure what
> > HAS means in other contexts without digging.
> >
> The existing pattern is to indicate arch support with
> CONFIG_ARCH_HAS_N and security/KConfig to have CONFIG_N as the main
> switch. For example, CONFIG_ARCH_HAS_FORTIFY_SOURCE and
> CONFIG_FORTIFY_SOURCE. Since the MSEAL_SYSTEM_MAPPINGS is placed in
> security/Kconfig, hence I'm following the existing pattern in
> security/Kconfig.
Okay, thanks. I really think SUPPORTS is more clear, but sticking with
whatever your area uses is also fine.
...
Thanks,
Liam
