Re: [PATCH v2] vfs: Fix anon_inode triggering VFS_BUG_ON_INODE in may_open()

[Mateusz Guzik]

On Fri, Apr 4, 2025 at 5:30 AM Penglei Jiang <superman.xpt@xxxxxxxxx> wrote:
>
> Some anon_inodes do not set the S_IFLNK, S_IFDIR, S_IFBLK, S_IFCHR,
> S_IFIFO, S_IFSOCK, or S_IFREG flags after initialization. As a result,
> opening these files triggers VFS_BUG_ON_INODE in the may_open() function.
>
> Here is the relevant code snippet:
>
>     static int may_open(struct mnt_idmap *idmap, const struct path *path,
>                 int acc_mode, int flag)
>     {
>             ...
>             switch (inode->i_mode & S_IFMT) {
>             case S_IFLNK:
>             case S_IFDIR:
>             case S_IFBLK:
>             case S_IFCHR:
>             case S_IFIFO:
>             case S_IFSOCK:
>             case S_IFREG:
>             default:
>                     VFS_BUG_ON_INODE(1, inode);
>                     ~~~~~~~~~~~~~~~~~~~~~~~~~~
>             }
>             ...
>     }
>
> To address this, we modify the code so that only non-anon_inodes trigger
> VFS_BUG_ON_INODE, and we also check MAY_EXEC.
>
> To determine if an inode is an anon_inode, we consider two cases:
>
>   1. If the inode is the same as anon_inode_inode, it is the default
>      anon_inode.
>   2. Anonymous inodes created with alloc_anon_inode() set the S_PRIVATE
>      flag. If S_PRIVATE is set, we consider it an anonymous inode.
>
> The bug can be reproduced with the following code:
>
>     #include <stdio.h>
>     #include <unistd.h>
>     #include <fcntl.h>
>     #include <sys/timerfd.h>
>     int main(int argc, char **argv) {
>             int fd = timerfd_create(CLOCK_MONOTONIC, 0);
>             if (fd != -1) {
>                     char path[256];
>                     sprintf(path, "/proc/self/fd/%d", fd);
>                     open(path, O_RDONLY);
>             }
>             return 0;
>     }
>
> Finally, after testing, anon_inodes no longer trigger VFS_BUG_ON_INODE.
>
> Fixes: af153bb63a336 ("vfs: catch invalid modes in may_open()")
> Reported-by: syzbot+5d8e79d323a13aa0b248@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://lore.kernel.org/all/67ed3fb3.050a0220.14623d.0009.GAE@xxxxxxxxxx";
> Signed-off-by: Penglei Jiang <superman.xpt@xxxxxxxxx>
> ---
> V1 -> V2: added MAY_EXEC check, added some comments
>
>  fs/anon_inodes.c            | 12 ++++++++++++
>  fs/namei.c                  |  8 +++++++-
>  include/linux/anon_inodes.h |  1 +
>  3 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
> index 583ac81669c2..bf124d53973e 100644
> --- a/fs/anon_inodes.c
> +++ b/fs/anon_inodes.c
> @@ -303,6 +303,18 @@ int anon_inode_create_getfd(const char *name, const struct file_operations *fops
>         return __anon_inode_getfd(name, fops, priv, flags, context_inode, true);
>  }
>
> +/**
> + * is_default_anon_inode - Checks if the given inode is the default
> + * anonymous inode (anon_inode_inode)
> + *
> + * @inode: [in] the inode to be checked
> + *
> + * Returns true if the given inode is anon_inode_inode, otherwise returns false.
> + */
> +inline bool is_default_anon_inode(const struct inode *inode)
> +{
> +       return anon_inode_inode == inode;
> +}
>

I would drop the inline.

>  static int __init anon_inode_init(void)
>  {
> diff --git a/fs/namei.c b/fs/namei.c
> index 360a86ca1f02..e8cc00a7c31a 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -40,6 +40,7 @@
>  #include <linux/bitops.h>
>  #include <linux/init_task.h>
>  #include <linux/uaccess.h>
> +#include <linux/anon_inodes.h>
>
>  #include "internal.h"
>  #include "mount.h"
> @@ -3429,7 +3430,12 @@ static int may_open(struct mnt_idmap *idmap, const struct path *path,
>                         return -EACCES;
>                 break;
>         default:
> -               VFS_BUG_ON_INODE(1, inode);
> +               if (!is_default_anon_inode(inode)
> +                       && !(inode->i_flags & S_PRIVATE))
> +                       VFS_BUG_ON_INODE(1, inode);
> +               if (acc_mode & MAY_EXEC)
> +                       return -EACCES;
> +               break;
>         }

Semantically this looks ok to me.

It may be this happens to still be too restrictive, but then we are
erroring on the side of some test suite blowing up, as opposed to
something in production, so I think it's fine.

I would fold these conditions into the debug macro though, as in:
VFS_BUG_ON_INODE(!is_default_anon_inode(inode) && !(inode->i_flags &
S_PRIVATE)), inode);

>
>         error = inode_permission(idmap, inode, MAY_OPEN | acc_mode);
> diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
> index edef565c2a1a..eca4a3913ba7 100644
> --- a/include/linux/anon_inodes.h
> +++ b/include/linux/anon_inodes.h
> @@ -30,6 +30,7 @@ int anon_inode_create_getfd(const char *name,
>                             const struct file_operations *fops,
>                             void *priv, int flags,
>                             const struct inode *context_inode);
> +bool is_default_anon_inode(const struct inode *inode);
>
>  #endif /* _LINUX_ANON_INODES_H */
>
> --
> 2.17.1
>


-- 
Mateusz Guzik <mjguzik gmail.com>