Re: [PATCH v3 11/34] lsm: get rid of the lsm_names list and do some cleanup

[John Johansen <john.johansen@canonical.com>]

On 9/4/25 01:12, Roberto Sassu wrote:
> On Tue, 2025-09-02 at 10:20 -0700, John Johansen wrote:
>> On 8/14/25 15:50, Paul Moore wrote:
>>> The LSM currently has a lot of code to maintain a list of the currently
>>> active LSMs in a human readable string, with the only user being the
>>> "/sys/kernel/security/lsm" code.  Let's drop all of that code and
>>> generate the string on first use and then cache it for subsequent use.
>>>
>>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>>> ---
>>>    include/linux/lsm_hooks.h |  1 -
>>>    security/inode.c          | 59 +++++++++++++++++++++++++++++++++++++--
>>>    security/lsm_init.c       | 49 --------------------------------
>>>    3 files changed, 57 insertions(+), 52 deletions(-)
>>>
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index 7343dd60b1d5..65a8227bece7 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -172,7 +172,6 @@ struct lsm_info {
>>>    
>>>    
>>>    /* DO NOT tamper with these variables outside of the LSM framework */
>>> -extern char *lsm_names;
>>>    extern struct lsm_static_calls_table static_calls_table __ro_after_init;
>>>    
>>>    /**
>>> diff --git a/security/inode.c b/security/inode.c
>>> index 43382ef8896e..a5e7a073e672 100644
>>> --- a/security/inode.c
>>> +++ b/security/inode.c
>>> @@ -22,6 +22,8 @@
>>>    #include <linux/lsm_hooks.h>
>>>    #include <linux/magic.h>
>>>    
>>> +#include "lsm.h"
>>> +
>>>    static struct vfsmount *mount;
>>>    static int mount_count;
>>>    
>>> @@ -315,12 +317,65 @@ void securityfs_remove(struct dentry *dentry)
>>>    EXPORT_SYMBOL_GPL(securityfs_remove);
>>>    
>>>    #ifdef CONFIG_SECURITY
>>> +#include <linux/spinlock.h>
>>> +
>>>    static struct dentry *lsm_dentry;
>>> +
>>> +/* NOTE: we never free the string below once it is set. */
>>> +static DEFINE_SPINLOCK(lsm_read_lock);
>>
>> nit, this is only used on the write side, so not the best name
>>
>>> +static char *lsm_read_str = NULL;
>>> +static ssize_t lsm_read_len = 0;
>>> +
>>>    static ssize_t lsm_read(struct file *filp, char __user *buf, size_t count,
>>>    			loff_t *ppos)
>>>    {
>>> -	return simple_read_from_buffer(buf, count, ppos, lsm_names,
>>> -		strlen(lsm_names));
>>> +	int i;
>>> +	char *str;
>>> +	ssize_t len;
>>> +
>>> +restart:
>>> +
>>> +	rcu_read_lock();
> 
> Uhm, it seems we cannot use plain RCU here, simple_read_from_buffer()
> can sleep.
> 

doh, yes.

But we shouldn't need RCU here either. This is a write once, never update
again situation. Instead we can get away with just a lock on the write
side to ensure exclusion on setting the value.

We don't even need the read side memory barrier, if the assumption that
the pointer is read as a single value holds, and the the null read will
go to the lock, and end up rereading.

> Roberto
> 
>>> +	if (!lsm_read_str) {
>> should probably be
>> if (!rcu_access_pointer(lsm_read_str)) {
>>
>>> +		/* we need to generate the string and try again */
>>> +		rcu_read_unlock();
>>> +		goto generate_string;
>>> +	}
>>> +	len = simple_read_from_buffer(buf, count, ppos,
>>> +				      rcu_dereference(lsm_read_str),
>>> +				      lsm_read_len);
>>> +	rcu_read_unlock();
>>> +	return len;
>>> +
>>> +generate_string:
>>> +
>>> +	for (i = 0; i < lsm_active_cnt; i++)
>>> +		/* the '+ 1' accounts for either a comma or a NUL */
>>> +		len += strlen(lsm_idlist[i]->name) + 1;
>>> +
>>> +	str = kmalloc(len, GFP_KERNEL);
>>> +	if (!str)
>>> +		return -ENOMEM;
>>> +	str[0] = '\0';
>>> +
>>> +	for (i = 0; i < lsm_active_cnt; i++) {
>>> +		if (i > 0)
>>> +			strcat(str, ",");
>>> +		strcat(str, lsm_idlist[i]->name);
>>> +	}
>>> +
>>> +	spin_lock(&lsm_read_lock);
>>> +	if (lsm_read_str) {
>>> +		/* we raced and lost */
>>> +		spin_unlock(&lsm_read_lock);
>>> +		kfree(str);
>>> +		goto restart;
>>> +	}
>>> +	lsm_read_str = str;
>>> +	lsm_read_len = len - 1;
>>> +	spin_unlock(&lsm_read_lock);
>>> +
>>> +	goto restart;
>>>    }
>>>    
>>>    static const struct file_operations lsm_ops = {
>>> diff --git a/security/lsm_init.c b/security/lsm_init.c
>>> index 9e495a36a332..87e2147016b3 100644
>>> --- a/security/lsm_init.c
>>> +++ b/security/lsm_init.c
>>> @@ -10,8 +10,6 @@
>>>    
>>>    #include "lsm.h"
>>>    
>>> -char *lsm_names;
>>> -
>>>    /* Pointers to LSM sections defined in include/asm-generic/vmlinux.lds.h */
>>>    extern struct lsm_info __start_lsm_info[], __end_lsm_info[];
>>>    extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[];
>>> @@ -371,42 +369,6 @@ static void __init lsm_init_ordered(void)
>>>    	}
>>>    }
>>>    
>>> -static bool match_last_lsm(const char *list, const char *lsm)
>>> -{
>>> -	const char *last;
>>> -
>>> -	if (WARN_ON(!list || !lsm))
>>> -		return false;
>>> -	last = strrchr(list, ',');
>>> -	if (last)
>>> -		/* Pass the comma, strcmp() will check for '\0' */
>>> -		last++;
>>> -	else
>>> -		last = list;
>>> -	return !strcmp(last, lsm);
>>> -}
>>> -
>>> -static int lsm_append(const char *new, char **result)
>>> -{
>>> -	char *cp;
>>> -
>>> -	if (*result == NULL) {
>>> -		*result = kstrdup(new, GFP_KERNEL);
>>> -		if (*result == NULL)
>>> -			return -ENOMEM;
>>> -	} else {
>>> -		/* Check if it is the last registered name */
>>> -		if (match_last_lsm(*result, new))
>>> -			return 0;
>>> -		cp = kasprintf(GFP_KERNEL, "%s,%s", *result, new);
>>> -		if (cp == NULL)
>>> -			return -ENOMEM;
>>> -		kfree(*result);
>>> -		*result = cp;
>>> -	}
>>> -	return 0;
>>> -}
>>> -
>>>    static void __init lsm_static_call_init(struct security_hook_list *hl)
>>>    {
>>>    	struct lsm_static_call *scall = hl->scalls;
>>> @@ -443,15 +405,6 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>>>    		hooks[i].lsmid = lsmid;
>>>    		lsm_static_call_init(&hooks[i]);
>>>    	}
>>> -
>>> -	/*
>>> -	 * Don't try to append during early_security_init(), we'll come back
>>> -	 * and fix this up afterwards.
>>> -	 */
>>> -	if (slab_is_available()) {
>>> -		if (lsm_append(lsmid->name, &lsm_names) < 0)
>>> -			panic("%s - Cannot get early memory.\n", __func__);
>>> -	}
>>>    }
>>>    
>>>    int __init early_security_init(void)
>>> @@ -488,8 +441,6 @@ int __init security_init(void)
>>>    	lsm_early_for_each_raw(lsm) {
>>>    		init_debug("  early started: %s (%s)\n", lsm->id->name,
>>>    			   is_enabled(lsm) ? "enabled" : "disabled");
>>> -		if (lsm->enabled)
>>> -			lsm_append(lsm->id->name, &lsm_names);
>>>    	}
>>>    
>>>    	/* Load LSMs in specified order. */
>>
>