Re: [PATCH v4 15/34] lsm: rename/rework ordered_lsm_parse() to lsm_order_parse()

[Mimi Zohar <zohar@linux.ibm.com>]

On Tue, 2025-09-16 at 18:03 -0400, Paul Moore wrote:
> Rename ordered_lsm_parse() to lsm_order_parse() for the sake of
> consistency with the other LSM initialization routines, and also
> do some minor rework of the function.  Aside from some minor style
> decisions, the majority of the rework involved shuffling the order
> of the LSM_FLAG_LEGACY and LSM_ORDER_FIRST code so that the
> LSM_FLAG_LEGACY checks are handled first; it is important to note
> that this doesn't affect the order in which the LSMs are registered.
> 
> Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> Reviewed-by: John Johansen <john.johhansen@canonical.com>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  security/lsm_init.c | 82 ++++++++++++++++++++-------------------------
>  1 file changed, 37 insertions(+), 45 deletions(-)
> 
> diff --git a/security/lsm_init.c b/security/lsm_init.c
> index a314484d7c2f..7b2491120fc8 100644
> --- a/security/lsm_init.c
> +++ b/security/lsm_init.c
> @@ -228,83 +228,75 @@ static void __init initialize_lsm(struct lsm_info *lsm)
>  	}
>  }
>  
> -/* Populate ordered LSMs list from comma-separated LSM name list. */
> -static void __init ordered_lsm_parse(const char *order, const char *origin)
> +/**
> + * lsm_order_parse - Parse the comma delimited LSM list
> + * @list: LSM list
> + * @src: source of the list
> + */
> +static void __init lsm_order_parse(const char *list, const char *src)
>  {
>  	struct lsm_info *lsm;
>  	char *sep, *name, *next;
>  
> -	/* LSM_ORDER_FIRST is always first. */
> -	lsm_for_each_raw(lsm) {
> -		if (lsm->order == LSM_ORDER_FIRST)
> -			lsm_order_append(lsm, "  first");
> -	}
> -
> -	/* Process "security=", if given. */
> +	/* Handle any Legacy LSM exclusions if one was specified. */
>  	if (lsm_order_legacy) {
> -		struct lsm_info *major;
> -
>  		/*
> -		 * To match the original "security=" behavior, this
> -		 * explicitly does NOT fallback to another Legacy Major
> -		 * if the selected one was separately disabled: disable
> -		 * all non-matching Legacy Major LSMs.
> +		 * To match the original "security=" behavior, this explicitly
> +		 * does NOT fallback to another Legacy Major if the selected
> +		 * one was separately disabled: disable all non-matching
> +		 * Legacy Major LSMs.
>  		 */
> -		lsm_for_each_raw(major) {
> -			if ((major->flags & LSM_FLAG_LEGACY_MAJOR) &&
> -			    strcmp(major->id->name, lsm_order_legacy) != 0) {
> -				lsm_enabled_set(major, false);
> +		lsm_for_each_raw(lsm) {
> +			if ((lsm->flags & LSM_FLAG_LEGACY_MAJOR) &&
> +			     strcmp(lsm->id->name, lsm_order_legacy)) {
> +				lsm_enabled_set(lsm, false);
>  				init_debug("security=%s disabled: %s (only one legacy major LSM)\n",
> -					   lsm_order_legacy, major->id->name);
> +					   lsm_order_legacy, lsm->id->name);
>  			}
>  		}
>  	}
>  
> -	sep = kstrdup(order, GFP_KERNEL);
> +	/* LSM_ORDER_FIRST */
> +	lsm_for_each_raw(lsm) {
> +		if (lsm->order == LSM_ORDER_FIRST)
> +			lsm_order_append(lsm, "first");
> +	}
> +
> +	/* Normal or "mutable" LSMs */

Paul, there's a reason for another set of eyes reviewing patches and yes, even,
comments.  What are "mutable" LSMs?!

The above comment LSM_ORDER_FIRST is self describing.  Here the word "order" is
missing from this comment.
-> Normal or "mutable" LSM ordering

Otherwise, Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>


> +	sep = kstrdup(list, GFP_KERNEL);
>  	next = sep;
>  	/* Walk the list, looking for matching LSMs. */
>  	while ((name = strsep(&next, ",")) != NULL) {
> -		bool found = false;
> -
>  		lsm_for_each_raw(lsm) {
> -			if (strcmp(lsm->id->name, name) == 0) {
> -				if (lsm->order == LSM_ORDER_MUTABLE)
> -					lsm_order_append(lsm, origin);
> -				found = true;
> -			}
> +			if (!strcmp(lsm->id->name, name) &&
> +			    lsm->order == LSM_ORDER_MUTABLE)
> +				lsm_order_append(lsm, src);
>  		}
> -
> -		if (!found)
> -			init_debug("%s ignored: %s (not built into kernel)\n",
> -				   origin, name);
>  	}
> +	kfree(sep);
>  
> -	/* Process "security=", if given. */
> +	/* Legacy LSM if specified. */
>  	if (lsm_order_legacy) {
>  		lsm_for_each_raw(lsm) {
> -			if (lsm_order_exists(lsm))
> -				continue;
> -			if (strcmp(lsm->id->name, lsm_order_legacy) == 0)
> -				lsm_order_append(lsm, "security=");
> +			if (!strcmp(lsm->id->name, lsm_order_legacy))
> +				lsm_order_append(lsm, src);
>  		}
>  	}
>  
> -	/* LSM_ORDER_LAST is always last. */
> +	/* LSM_ORDER_LAST */
>  	lsm_for_each_raw(lsm) {
>  		if (lsm->order == LSM_ORDER_LAST)
> -			lsm_order_append(lsm, "   last");
> +			lsm_order_append(lsm, "last");
>  	}
>  
> -	/* Disable all LSMs not in the ordered list. */
> +	/* Disable all LSMs not previously enabled. */
>  	lsm_for_each_raw(lsm) {
>  		if (lsm_order_exists(lsm))
>  			continue;
>  		lsm_enabled_set(lsm, false);
>  		init_debug("%s skipped: %s (not in requested order)\n",
> -			   origin, lsm->id->name);
> +			   src, lsm->id->name);
>  	}
> -
> -	kfree(sep);
>  }
>  
>  /**
> @@ -322,9 +314,9 @@ static void __init lsm_init_ordered(void)
>  				lsm_order_legacy, lsm_order_cmdline);
>  			lsm_order_legacy = NULL;
>  		}
> -		ordered_lsm_parse(lsm_order_cmdline, "cmdline");
> +		lsm_order_parse(lsm_order_cmdline, "cmdline");
>  	} else
> -		ordered_lsm_parse(lsm_order_builtin, "builtin");
> +		lsm_order_parse(lsm_order_builtin, "builtin");
>  
>  	lsm_order_for_each(lsm) {
>  		lsm_prepare(*lsm);