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

[Paul Moore <paul@paul-moore.com>]

On Tue, Sep 2, 2025 at 1:20 PM John Johansen
<john.johansen@canonical.com> 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

Fair point, I'll rename it to lsm_read_str_lock, it still has "read"
in the name, but it should be a bit more clear that it references the
lsm_read_str variable.

> > +static char *lsm_read_str = NULL;
> > +static ssize_t lsm_read_len = 0;

Similarly, I'm renaming lsm_read_len to lsm_read_str_len.

> >   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();
> > +     if (!lsm_read_str) {
>
> should probably be
> if (!rcu_access_pointer(lsm_read_str)) {

The description for rcu_access_pointer() contains the following sentence:

  "Within an RCU read-side critical section, there is little reason to
   use rcu_access_pointer()."
  https://elixir.bootlin.com/linux/v6.17-rc4/source/include/linux/rcupdate.h#L628

Perhaps I'm reading it wrong, but it looks like the RCU folks would
prefer we not use rcu_access_pointer() here?

-- 
paul-moore.com