On Thu, Sep 4, 2025 at 4:48 AM John Johansen <[email protected]> wrote: > 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 <[email protected]> > >>> --- > >>> 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.
D'oh indeed! Thanks for catching this. > 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. It's funny you bring this up, my first draft of this function (unposted) did just that, although I figured I'd add the RCU read side protection ... "just in case". I'll rework this function, but I'll hold off on posting another revision until I hear back on some of the reviews that are still pending in case additional edits are needed. -- paul-moore.com
