On Wed, 10 Jan 2024 10:52:51 -0500
Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> On Wed, 10 Jan 2024 08:07:46 -0500
> Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> > Or are you saying that I don't need the ".permission" callback, because
> > eventfs does it when it creates the inodes? But for eventfs to know what
> > the permissions changes are, it uses .getattr and .setattr.
>
> OK, if your main argument is that we do not need .permission, I agree with
> you. But that's a trivial change and doesn't affect the complexity that
> eventfs is doing. In fact, removing the "permission" check is simply this
> patch:
>
> --
> diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
> index fdff53d5a1f8..f2af07a857e2 100644
> --- a/fs/tracefs/event_inode.c
> +++ b/fs/tracefs/event_inode.c
> @@ -192,18 +192,10 @@ static int eventfs_get_attr(struct mnt_idmap *idmap,
> return 0;
> }
>
> -static int eventfs_permission(struct mnt_idmap *idmap,
> - struct inode *inode, int mask)
> -{
> - set_top_events_ownership(inode);
> - return generic_permission(idmap, inode, mask);
> -}
> -
> static const struct inode_operations eventfs_root_dir_inode_operations = {
> .lookup = eventfs_root_lookup,
> .setattr = eventfs_set_attr,
> .getattr = eventfs_get_attr,
> - .permission = eventfs_permission,
> };
>
> static const struct inode_operations eventfs_file_inode_operations = {
> --
>
> I only did that because Linus mentioned it, and I thought it was needed.
> I'll apply this patch too, as it appears to work with this code.
Oh, eventfs files and directories don't need the .permissions because its
inodes and dentries are not created until accessed. But the "events"
directory itself has its dentry and inode created at boot up, but still
uses the eventfs_root_dir_inode_operations. So the .permissions is still
needed!
If you look at the "set_top_events_ownership()" function, it has:
/* The top events directory doesn't get automatically updated */
if (!ei || !ei->is_events || !(ei->attr.mode & EVENTFS_TOPLEVEL))
return;
That is, it does nothing if the entry is not the "events" directory. It
falls back to he default "->permissions()" function for everything but the
top level "events" directory.
But this and .getattr are still needed for the events directory, because it
suffers the same issue as the other tracefs entries. That is, it's inodes
and dentries are created at boot up before it is mounted. So if the mount
has gid=1000, it will be ignored.
The .getattr is called by "stat" which ls does. So after boot up if you
just do:
# chmod 0750 /sys/kernel/events
# chmod 0770 /sys/kernel/tracing
# mount -o remount,gid=1000 /sys/kernel/tracing
# su - rostedt
$ id
uid=1000(rostedt) gid=1000(rostedt) groups=1000(rostedt)
$ ls /sys/kernel/tracing/events/
9p ext4 iomap module raw_syscalls thermal
alarmtimer fib iommu msr rcu thp
avc fib6 io_uring napi regmap timer
block filelock ipi neigh regulator tlb
bpf_test_run filemap irq net resctrl udp
bpf_trace ftrace irq_matrix netfs rpm virtio_gpu[
...]
The above works because "ls" does a stat() on the directory first, which
does a .getattr() call that updates the permissions of the existing "events"
directory inode.
BUT!
If I had used my own getents() program that has:
fd = openat(AT_FDCWD, argv[1], O_RDONLY);
if (fd < 0)
perror("openat");
n = getdents64(fd, buf, BUF_SIZE);
if (n < 0)
perror("getdents64");
Where it calls the openat() without doing a stat fist, and after boot, had done:
# chmod 0750 /sys/kernel/events
# chmod 0770 /sys/kernel/tracing
# mount -o remount,gid=1000 /sys/kernel/tracing
# su - rostedt
$ id
uid=1000(rostedt) gid=1000(rostedt) groups=1000(rostedt)
$ ./getdents /sys/kernel/tracing/events
openat: Permission denied
getdents64: Bad file descriptor
It errors because he "events" inode permission hasn't been updated yet.
Now after getting the above error, if I do the "ls" and then run it again:
$ ls /sys/kernel/tracing/events > /dev/null
$ ./getdents /sys/kernel/tracing/events
enable
header_page
header_event
initcall
vsyscall
syscalls
it works!
so no, I can't remove that .permissions callback from eventfs.
-- Steve
