On Thu, Jan 11, 2024 at 04:53:19PM -0500, Steven Rostedt wrote:
On Thu, 11 Jan 2024 22:01:32 +0100
Christian Brauner [off-list ref] wrote:
quoted
What I'm pointing out in the current logic is that the caller is
taxed twice:
(1) Once when the VFS has done inode_permission(MAY_EXEC, "xfs")
(2) And again when you call lookup_one_len() in eventfs_start_creating()
_because_ the permission check in lookup_one_len() is the exact
same permission check again that the vfs has done
inode_permission(MAY_EXEC, "xfs").
As I described in: https://lore.kernel.org/all/20240110133154.6e18feb9@gandalf.local.home/ (local)
The eventfs files below "events" doesn't need the .permissions callback at
all. It's only there because the "events" inode uses it.
The .permissions call for eventfs has:
It doesn't matter whether there's a ->permission handler. If you don't
add one explicitly the VFS will simply call generic_permission():
inode_permission()
-> do_inode_permission()
{
if (unlikely(!(inode->i_opflags & IOP_FASTPERM))) {
if (likely(inode->i_op->permission))
return inode->i_op->permission(idmap, inode, mask);
/* This gets set once for the inode lifetime */
spin_lock(&inode->i_lock);
inode->i_opflags |= IOP_FASTPERM;
spin_unlock(&inode->i_lock);
}
return generic_permission(idmap, inode, mask);
}
Anyway, the issue is with "events" directory and remounting, because like
the tracefs system, the inode and dentry for "evnets" is created at boot
up, before the mount happens. The VFS layer is going to check the
permissions of its inode and dentry, which will be incorrect if the mount
was mounted with a "gid" option.
The gid option has nothing to do with this and it is just handled fine
if you remove the second permission checking in (2).
You need to remove the inode_permission() code from
eventfs_start_creating(). It is just an internal lookup and the fact
that you have it in there allows userspace to break readdir on the
eventfs portions of tracefs as I've shown in the parts of the mail that
you cut off.