Re: [PATCH v4 1/1] exec: seal system mappings

From: Matthew Wilcox
Date: Mon Nov 25 2024 - 15:40:23 EST


On Mon, Nov 25, 2024 at 08:20:21PM +0000, jeffxu@xxxxxxxxxxxx wrote:
> +/*
> + * Kernel cmdline override for CONFIG_SEAL_SYSTEM_MAPPINGS
> + */
> +enum seal_system_mappings_type {
> + SEAL_SYSTEM_MAPPINGS_DISABLED,
> + SEAL_SYSTEM_MAPPINGS_ENABLED
> +};
> +
> +static enum seal_system_mappings_type seal_system_mappings_v __ro_after_init =
> + IS_ENABLED(CONFIG_SEAL_SYSTEM_MAPPINGS) ? SEAL_SYSTEM_MAPPINGS_ENABLED :
> + SEAL_SYSTEM_MAPPINGS_DISABLED;
> +
> +static const struct constant_table value_table_sys_mapping[] __initconst = {
> + { "no", SEAL_SYSTEM_MAPPINGS_DISABLED},
> + { "yes", SEAL_SYSTEM_MAPPINGS_ENABLED},
> + { }
> +};
> +
> +static int __init early_seal_system_mappings_override(char *buf)
> +{
> + if (!buf)
> + return -EINVAL;
> +
> + seal_system_mappings_v = lookup_constant(value_table_sys_mapping,
> + buf, seal_system_mappings_v);
> + return 0;
> +}
> +
> +early_param("exec.seal_system_mappings", early_seal_system_mappings_override);

Are you paid by the line? This all seems ridiculously overcomplicated.
Look at (first example I found) kgdbwait:

static int __init opt_kgdb_wait(char *str)
{
kgdb_break_asap = 1;

kdb_init(KDB_INIT_EARLY);
if (kgdb_io_module_registered &&
IS_ENABLED(CONFIG_ARCH_HAS_EARLY_DEBUG))
kgdb_initial_breakpoint();

return 0;
}
early_param("kgdbwait", opt_kgdb_wait);

I don't understand why you've created a new 'exec' namespace, and why
this feature fits in 'exec'. That seems like an implementation detail.
I'd lose the "exec." prefix.