Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-12 Thread Steven Rostedt
On Fri, 12 Jan 2024 08:53:44 -0500 Steven Rostedt wrote: > > // We managed to open the directory so we have permission to list > > // directory entries in "xfs". > > fd = open("/sys/kernel/tracing/events/xfs"); > > > > // Remove ownership so we can't open the directory anymore > >

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-12 Thread Steven Rostedt
On Fri, 12 Jan 2024 09:27:24 +0100 Christian Brauner wrote: > On Thu, Jan 11, 2024 at 04:53:19PM -0500, Steven Rostedt wrote: > > On Thu, 11 Jan 2024 22:01:32 +0100 > > Christian Brauner wrote: > > > > > What I'm pointing out in the current logic is that the caller is > > > taxed twice: > >

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-12 Thread Christian Brauner
On Thu, Jan 11, 2024 at 04:53:19PM -0500, Steven Rostedt wrote: > On Thu, 11 Jan 2024 22:01:32 +0100 > Christian Brauner wrote: > > > 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") > >

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-11 Thread Steven Rostedt
On Thu, 11 Jan 2024 22:01:32 +0100 Christian Brauner wrote: > 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() >

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-11 Thread Christian Brauner
On Wed, Jan 10, 2024 at 08:07:46AM -0500, Steven Rostedt wrote: > On Wed, 10 Jan 2024 12:45:36 +0100 > Christian Brauner wrote: > > > So say you do: > > > > mkdir /sys/kernel/tracing/instances/foo > > > > After this has returned we know everything we need to know about the new > > tracefs

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-10 Thread Steven Rostedt
On Wed, 10 Jan 2024 10:52:51 -0500 Steven Rostedt wrote: > On Wed, 10 Jan 2024 08:07:46 -0500 > Steven Rostedt 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

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-10 Thread Steven Rostedt
On Wed, 10 Jan 2024 10:52:51 -0500 Steven Rostedt wrote: > I'll apply this patch too, as it appears to work with this code. I meant "appears to work without this code". -- Steve

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-10 Thread Steven Rostedt
On Wed, 10 Jan 2024 08:07:46 -0500 Steven Rostedt 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

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-10 Thread Steven Rostedt
On Wed, 10 Jan 2024 12:45:36 +0100 Christian Brauner wrote: > So say you do: > > mkdir /sys/kernel/tracing/instances/foo > > After this has returned we know everything we need to know about the new > tracefs instance including the ownership and the mode of all inodes in >

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-10 Thread Christian Brauner
On Mon, Jan 08, 2024 at 10:23:31AM -0500, Steven Rostedt wrote: > On Mon, 8 Jan 2024 12:04:54 +0100 > Christian Brauner wrote: > > > > > IOW, the inode_permission() in lookup_one_len() that eventfs does is > > > > redundant and just wrong. > > > > > > I don't think so. > > > > I'm very

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-08 Thread Steven Rostedt
On Mon, 8 Jan 2024 12:32:46 +0100 Christian Brauner wrote: > On Sun, Jan 07, 2024 at 01:32:28PM -0500, Steven Rostedt wrote: > > On Sun, 7 Jan 2024 13:29:12 -0500 > > Steven Rostedt wrote: > > > > > > > > > > IOW, the inode_permission() in lookup_one_len() that eventfs does is > > > >

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-08 Thread Steven Rostedt
On Mon, 8 Jan 2024 12:04:54 +0100 Christian Brauner wrote: > > > IOW, the inode_permission() in lookup_one_len() that eventfs does is > > > redundant and just wrong. > > > > I don't think so. > > I'm very well aware that the dentries and inode aren't created during > mkdir but the

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-08 Thread Christian Brauner
On Sun, Jan 07, 2024 at 01:32:28PM -0500, Steven Rostedt wrote: > On Sun, 7 Jan 2024 13:29:12 -0500 > Steven Rostedt wrote: > > > > > > > IOW, the inode_permission() in lookup_one_len() that eventfs does is > > > redundant and just wrong. > > > > I don't think so. > > Just to make it clear.

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-08 Thread Christian Brauner
> > * Tracefs supports the creation of instances from userspace via mkdir. > > For example, > > > > mkdir /sys/kernel/tracing/instances/foo > > > > And here the idmapping is relevant so we need to make the helpers > > aware of the idmapping. > > > > I just went and plumbed this

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-07 Thread Steven Rostedt
On Sun, 7 Jan 2024 13:29:12 -0500 Steven Rostedt wrote: > > > > IOW, the inode_permission() in lookup_one_len() that eventfs does is > > redundant and just wrong. > > I don't think so. Just to make it clear. eventfs has nothing to do with mkdir instance/foo. It exists without that. Although

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-07 Thread Steven Rostedt
On Sun, 7 Jan 2024 13:42:39 +0100 Christian Brauner wrote: > > So, I tried to do an exploratory patch even though I promised myself not > to do it. But hey... > > Some notes: > > * Permission handling for idmapped mounts is done completely in the > VFS. That's the case for all filesytems

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-07 Thread Christian Brauner
On Sun, Jan 07, 2024 at 06:42:33PM +0100, Christian Brauner wrote: > On Sun, Jan 07, 2024 at 01:42:39PM +0100, Christian Brauner wrote: > > > > So tracefs supports remounting with different uid/gid mount options and > > > > then actually wades through _all_ of the inodes and changes their > > > >

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-07 Thread Christian Brauner
On Sun, Jan 07, 2024 at 01:42:39PM +0100, Christian Brauner wrote: > > > So tracefs supports remounting with different uid/gid mount options and > > > then actually wades through _all_ of the inodes and changes their > > > ownership internally? What's the use-case for this? Containers? > > > >

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-07 Thread Christian Brauner
> > So tracefs supports remounting with different uid/gid mount options and > > then actually wades through _all_ of the inodes and changes their > > ownership internally? What's the use-case for this? Containers? > > No, in fact tracing doesn't work well with containers as tracing is global > to

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-05 Thread Steven Rostedt
On Fri, 5 Jan 2024 15:26:28 +0100 Christian Brauner wrote: > On Wed, Jan 03, 2024 at 08:32:46PM -0500, Steven Rostedt wrote: > > From: "Steven Rostedt (Google)" > > > > Instead of walking the dentries on mount/remount to update the gid values of > > all the dentries if a gid option is

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-05 Thread Christian Brauner
On Wed, Jan 03, 2024 at 08:32:46PM -0500, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" > > Instead of walking the dentries on mount/remount to update the gid values of > all the dentries if a gid option is specified on mount, just update the root > inode. Add .getattr, .setattr, and

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-03 Thread Al Viro
On Wed, Jan 03, 2024 at 09:25:06PM -0500, Steven Rostedt wrote: > On Thu, 4 Jan 2024 01:48:37 + > Al Viro wrote: > > > On Wed, Jan 03, 2024 at 08:32:46PM -0500, Steven Rostedt wrote: > > > > > + /* Get the tracefs root from the parent */ > > > + inode = d_inode(dentry->d_parent); > > > +

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-03 Thread Steven Rostedt
On Thu, 4 Jan 2024 01:48:37 + Al Viro wrote: > On Wed, Jan 03, 2024 at 08:32:46PM -0500, Steven Rostedt wrote: > > > + /* Get the tracefs root from the parent */ > > + inode = d_inode(dentry->d_parent); > > + inode = d_inode(inode->i_sb->s_root); > > That makes no sense. First of

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-03 Thread Steven Rostedt
On Thu, 4 Jan 2024 01:59:10 + Al Viro wrote: > On Wed, Jan 03, 2024 at 08:32:46PM -0500, Steven Rostedt wrote: > > > +static struct inode *instance_inode(struct dentry *parent, struct inode > > *inode) > > +{ > > + struct tracefs_inode *ti; > > + struct inode *root_inode; > > + > > +

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-03 Thread Al Viro
On Wed, Jan 03, 2024 at 08:32:46PM -0500, Steven Rostedt wrote: > +static struct inode *instance_inode(struct dentry *parent, struct inode > *inode) > +{ > + struct tracefs_inode *ti; > + struct inode *root_inode; > + > + root_inode = d_inode(inode->i_sb->s_root); > + > + /* If

Re: [PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-03 Thread Al Viro
On Wed, Jan 03, 2024 at 08:32:46PM -0500, Steven Rostedt wrote: > + /* Get the tracefs root from the parent */ > + inode = d_inode(dentry->d_parent); > + inode = d_inode(inode->i_sb->s_root); That makes no sense. First of all, for any positive dentry we have dentry->d_sb ==

[PATCH] tracefs/eventfs: Use root and instance inodes as default ownership

2024-01-03 Thread Steven Rostedt
From: "Steven Rostedt (Google)" Instead of walking the dentries on mount/remount to update the gid values of all the dentries if a gid option is specified on mount, just update the root inode. Add .getattr, .setattr, and .permissions on the tracefs inode operations to update the permissions of