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
