Re: [PATCH v11 2/4] fs: add LSM-supporting anon-inode interface

[Eric Biggers]

On Thu, Nov 05, 2020 at 01:33:22PM -0800, Lokesh Gidra wrote:
> +/**
> + * Like anon_inode_getfd(), but creates a new !S_PRIVATE anon inode rather than
> + * reuse the singleton anon inode, and call the init_security_anon() LSM hook.
> + * This allows the inode to have its own security context and for a LSM to
> + * reject creation of the inode.  An optional @context_inode argument is also
> + * added to provide the logical relationship with the new inode.  The LSM may use
> + * @context_inode in init_security_anon(), but a reference to it is not held.
> + */
> +int anon_inode_getfd_secure(const char *name, const struct file_operations *fops,
> +			    void *priv, int flags,
> +			    const struct inode *context_inode)
> +{
> +	return __anon_inode_getfd(name, fops, priv, flags, context_inode, true);
> +}
> +EXPORT_SYMBOL_GPL(anon_inode_getfd_secure);

inode_init_security_anon(), not init_security_anon().  Also please use a
consistent line width (preferably 80 characters).

> diff --git a/fs/libfs.c b/fs/libfs.c
> index fc34361c1489..5b12228ecc81 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -1213,9 +1213,9 @@ static int anon_set_page_dirty(struct page *page)
>  };
>  
>  /*
> - * A single inode exists for all anon_inode files. Contrary to pipes,
> - * anon_inode inodes have no associated per-instance data, so we need
> - * only allocate one of them.
> + * A single inode exists for all anon_inode files, except for the secure ones.
> + * Contrary to pipes and secure anon_inode inodes, ordinary anon_inode inodes
> + * have no associated per-instance data, so we need only allocate one of them.
>   */
>  struct inode *alloc_anon_inode(struct super_block *s)
>  {

This comment is still wrong, and the first sentence contradicts the second one.
There are a lot of callers of alloc_anon_inode() and none of them use the
singleton inode, since alloc_anon_inode() doesn't actually use it.  The
singleton inode is only used by anon_inode_getfile() and anon_inode_getfd(),
which already have comments describing how they use a singleton inode.

IMO, just deleting this comment would be much better than either the original
version or your proposed version.

> diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
> index d0d7d96261ad..6cf447cfceed 100644
> --- a/include/linux/anon_inodes.h
> +++ b/include/linux/anon_inodes.h
> @@ -10,10 +10,15 @@
>  #define _LINUX_ANON_INODES_H
>  
>  struct file_operations;
> +struct inode;
>  
>  struct file *anon_inode_getfile(const char *name,
>  				const struct file_operations *fops,
>  				void *priv, int flags);
> +int anon_inode_getfd_secure(const char *name,
> +			    const struct file_operations *fops,
> +			    void *priv, int flags,
> +			    const struct inode *context_inode);
>  int anon_inode_getfd(const char *name, const struct file_operations *fops,
>  		     void *priv, int flags);
>  

Keeping declarations in the same order as the definitions can be helpful.

- Eric