From: "Steven Rostedt (Google)" <[email protected]>
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 the files and directories.
For all files and directories in the top level instance:
/sys/kernel/tracing/*
It will use the root inode as the default permissions. The inode that
represents: /sys/kernel/tracing (or wherever it is mounted).
When an instance is created:
mkdir /sys/kernel/tracing/instance/foo
The directory "foo" and all its files and directories underneath will use
the default of what foo is when it was created. A remount of tracefs will
not affect it.
If a user were to modify the permissions of any file or directory in
tracefs, it will also no longer be modified by a change in ownership of a
remount.
The events directory, if it is in the top level instance, will use the
tracefs root inode as the default ownership for itself and all the files and
directories below it.
For the events directory in an instance ("foo"), it will keep the ownership
of what it was when it was created, and that will be used as the default
ownership for the files and directories beneath it.
Link: https://lore.kernel.org/linux-trace-kernel/CAHk-=wjVdGkjDXBbvLn2wbZnqP4UsH46E3gqJ9m7UG6DpX2+WA@mail.gmail.com/
Signed-off-by: Steven Rostedt (Google) <[email protected]>
---
fs/tracefs/event_inode.c | 80 ++++++++++++++-
fs/tracefs/inode.c | 205 ++++++++++++++++++++++-----------------
fs/tracefs/internal.h | 3 +
3 files changed, 198 insertions(+), 90 deletions(-)
diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 53d34a4b5a2b..641bffa0f139 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -45,6 +45,7 @@ enum {
EVENTFS_SAVE_MODE = BIT(16),
EVENTFS_SAVE_UID = BIT(17),
EVENTFS_SAVE_GID = BIT(18),
+ EVENTFS_TOPLEVEL = BIT(19),
};
#define EVENTFS_MODE_MASK (EVENTFS_SAVE_MODE - 1)
@@ -115,10 +116,17 @@ static int eventfs_set_attr(struct mnt_idmap *idmap, struct dentry *dentry,
* The events directory dentry is never freed, unless its
* part of an instance that is deleted. It's attr is the
* default for its child files and directories.
- * Do not update it. It's not used for its own mode or ownership
+ * Do not update it. It's not used for its own mode or ownership.
*/
- if (!ei->is_events)
+ if (ei->is_events) {
+ /* But it still needs to know if it was modified */
+ if (iattr->ia_valid & ATTR_UID)
+ ei->attr.mode |= EVENTFS_SAVE_UID;
+ if (iattr->ia_valid & ATTR_GID)
+ ei->attr.mode |= EVENTFS_SAVE_GID;
+ } else {
update_attr(&ei->attr, iattr);
+ }
} else {
name = dentry->d_name.name;
@@ -136,9 +144,67 @@ static int eventfs_set_attr(struct mnt_idmap *idmap, struct dentry *dentry,
return ret;
}
+static void update_top_events_attr(struct eventfs_inode *ei, struct dentry *dentry)
+{
+ struct inode *inode;
+
+ /* Only update if the "events" was on the top level */
+ if (!ei || !(ei->attr.mode & EVENTFS_TOPLEVEL))
+ return;
+
+ /* Get the tracefs root from the parent */
+ inode = d_inode(dentry->d_parent);
+ inode = d_inode(inode->i_sb->s_root);
+ ei->attr.uid = inode->i_uid;
+ ei->attr.gid = inode->i_gid;
+}
+
+static void set_top_events_ownership(struct inode *inode)
+{
+ struct tracefs_inode *ti = get_tracefs(inode);
+ struct eventfs_inode *ei = ti->private;
+ struct dentry *dentry;
+
+ /* The top events directory doesn't get automatically updated */
+ if (!ei || !ei->is_events || !(ei->attr.mode & EVENTFS_TOPLEVEL))
+ return;
+
+ dentry = ei->dentry;
+
+ update_top_events_attr(ei, dentry);
+
+ if (!(ei->attr.mode & EVENTFS_SAVE_UID))
+ inode->i_uid = ei->attr.uid;
+
+ if (!(ei->attr.mode & EVENTFS_SAVE_GID))
+ inode->i_gid = ei->attr.gid;
+}
+
+static int eventfs_get_attr(struct mnt_idmap *idmap,
+ const struct path *path, struct kstat *stat,
+ u32 request_mask, unsigned int flags)
+{
+ struct dentry *dentry = path->dentry;
+ struct inode *inode = d_backing_inode(dentry);
+
+ set_top_events_ownership(inode);
+
+ generic_fillattr(idmap, request_mask, inode, stat);
+ 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 = {
@@ -174,6 +240,8 @@ static struct eventfs_inode *eventfs_find_events(struct dentry *dentry)
} while (!ei->is_events);
mutex_unlock(&eventfs_mutex);
+ update_top_events_attr(ei, dentry);
+
return ei;
}
@@ -887,6 +955,14 @@ struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry
uid = d_inode(dentry->d_parent)->i_uid;
gid = d_inode(dentry->d_parent)->i_gid;
+ /*
+ * If the events directory is of the top instance, then parent
+ * is NULL. Set the attr.mode to reflect this and its permissions will
+ * default to the tracefs root dentry.
+ */
+ if (!parent)
+ ei->attr.mode = EVENTFS_TOPLEVEL;
+
/* This is used as the default ownership of the files and directories */
ei->attr.uid = uid;
ei->attr.gid = gid;
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index bc86ffdb103b..63284f18741f 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -91,6 +91,7 @@ static int tracefs_syscall_mkdir(struct mnt_idmap *idmap,
struct inode *inode, struct dentry *dentry,
umode_t mode)
{
+ struct tracefs_inode *ti;
char *name;
int ret;
@@ -98,6 +99,15 @@ static int tracefs_syscall_mkdir(struct mnt_idmap *idmap,
if (!name)
return -ENOMEM;
+ /*
+ * This is a new directory that does not take the default of
+ * the rootfs. It becomes the default permissions for all the
+ * files and directories underneath it.
+ */
+ ti = get_tracefs(inode);
+ ti->flags |= TRACEFS_INSTANCE_INODE;
+ ti->private = inode;
+
/*
* The mkdir call can call the generic functions that create
* the files within the tracefs system. It is up to the individual
@@ -141,10 +151,76 @@ static int tracefs_syscall_rmdir(struct inode *inode, struct dentry *dentry)
return ret;
}
-static const struct inode_operations tracefs_dir_inode_operations = {
+static void set_tracefs_inode_owner(struct inode *inode)
+{
+ struct tracefs_inode *ti = get_tracefs(inode);
+ struct inode *root_inode = ti->private;
+
+ /*
+ * If this inode has never been referenced, then update
+ * the permissions to the superblock.
+ */
+ if (!(ti->flags & TRACEFS_UID_PERM_SET))
+ inode->i_uid = root_inode->i_uid;
+
+ if (!(ti->flags & TRACEFS_GID_PERM_SET))
+ inode->i_gid = root_inode->i_gid;
+}
+
+static int tracefs_permission(struct mnt_idmap *idmap,
+ struct inode *inode, int mask)
+{
+ set_tracefs_inode_owner(inode);
+ return generic_permission(idmap, inode, mask);
+}
+
+static int tracefs_getattr(struct mnt_idmap *idmap,
+ const struct path *path, struct kstat *stat,
+ u32 request_mask, unsigned int flags)
+{
+ struct inode *inode = d_backing_inode(path->dentry);
+
+ set_tracefs_inode_owner(inode);
+ generic_fillattr(idmap, request_mask, inode, stat);
+ return 0;
+}
+
+static int tracefs_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
+ struct iattr *attr)
+{
+ unsigned int ia_valid = attr->ia_valid;
+ struct inode *inode = d_inode(dentry);
+ struct tracefs_inode *ti = get_tracefs(inode);
+
+ if (ia_valid & ATTR_UID)
+ ti->flags |= TRACEFS_UID_PERM_SET;
+
+ if (ia_valid & ATTR_GID)
+ ti->flags |= TRACEFS_GID_PERM_SET;
+
+ return simple_setattr(idmap, dentry, attr);
+}
+
+static const struct inode_operations tracefs_instance_dir_inode_operations = {
.lookup = simple_lookup,
.mkdir = tracefs_syscall_mkdir,
.rmdir = tracefs_syscall_rmdir,
+ .permission = tracefs_permission,
+ .getattr = tracefs_getattr,
+ .setattr = tracefs_setattr,
+};
+
+static const struct inode_operations tracefs_dir_inode_operations = {
+ .lookup = simple_lookup,
+ .permission = tracefs_permission,
+ .getattr = tracefs_getattr,
+ .setattr = tracefs_setattr,
+};
+
+static const struct inode_operations tracefs_file_inode_operations = {
+ .permission = tracefs_permission,
+ .getattr = tracefs_getattr,
+ .setattr = tracefs_setattr,
};
struct inode *tracefs_get_inode(struct super_block *sb)
@@ -183,87 +259,6 @@ struct tracefs_fs_info {
struct tracefs_mount_opts mount_opts;
};
-static void change_gid(struct dentry *dentry, kgid_t gid)
-{
- if (!dentry->d_inode)
- return;
- dentry->d_inode->i_gid = gid;
-}
-
-/*
- * Taken from d_walk, but without he need for handling renames.
- * Nothing can be renamed while walking the list, as tracefs
- * does not support renames. This is only called when mounting
- * or remounting the file system, to set all the files to
- * the given gid.
- */
-static void set_gid(struct dentry *parent, kgid_t gid)
-{
- struct dentry *this_parent;
- struct list_head *next;
-
- this_parent = parent;
- spin_lock(&this_parent->d_lock);
-
- change_gid(this_parent, gid);
-repeat:
- next = this_parent->d_subdirs.next;
-resume:
- while (next != &this_parent->d_subdirs) {
- struct tracefs_inode *ti;
- struct list_head *tmp = next;
- struct dentry *dentry = list_entry(tmp, struct dentry, d_child);
- next = tmp->next;
-
- /* Note, getdents() can add a cursor dentry with no inode */
- if (!dentry->d_inode)
- continue;
-
- spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
-
- change_gid(dentry, gid);
-
- /* If this is the events directory, update that too */
- ti = get_tracefs(dentry->d_inode);
- if (ti && (ti->flags & TRACEFS_EVENT_INODE))
- eventfs_update_gid(dentry, gid);
-
- if (!list_empty(&dentry->d_subdirs)) {
- spin_unlock(&this_parent->d_lock);
- spin_release(&dentry->d_lock.dep_map, _RET_IP_);
- this_parent = dentry;
- spin_acquire(&this_parent->d_lock.dep_map, 0, 1, _RET_IP_);
- goto repeat;
- }
- spin_unlock(&dentry->d_lock);
- }
- /*
- * All done at this level ... ascend and resume the search.
- */
- rcu_read_lock();
-ascend:
- if (this_parent != parent) {
- struct dentry *child = this_parent;
- this_parent = child->d_parent;
-
- spin_unlock(&child->d_lock);
- spin_lock(&this_parent->d_lock);
-
- /* go into the first sibling still alive */
- do {
- next = child->d_child.next;
- if (next == &this_parent->d_subdirs)
- goto ascend;
- child = list_entry(next, struct dentry, d_child);
- } while (unlikely(child->d_flags & DCACHE_DENTRY_KILLED));
- rcu_read_unlock();
- goto resume;
- }
- rcu_read_unlock();
- spin_unlock(&this_parent->d_lock);
- return;
-}
-
static int tracefs_parse_options(char *data, struct tracefs_mount_opts *opts)
{
substring_t args[MAX_OPT_ARGS];
@@ -336,10 +331,8 @@ static int tracefs_apply_options(struct super_block *sb, bool remount)
if (!remount || opts->opts & BIT(Opt_uid))
inode->i_uid = opts->uid;
- if (!remount || opts->opts & BIT(Opt_gid)) {
- /* Set all the group ids to the mount option */
- set_gid(sb->s_root, opts->gid);
- }
+ if (!remount || opts->opts & BIT(Opt_gid))
+ inode->i_gid = opts->gid;
return 0;
}
@@ -573,6 +566,33 @@ struct dentry *eventfs_end_creating(struct dentry *dentry)
return dentry;
}
+/* Find the inode that this will use for default */
+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 parent is NULL then use root inode */
+ if (!parent)
+ return root_inode;
+
+ /* Find the inode that is flagged as an instance or the root inode */
+ do {
+ inode = d_inode(parent);
+ if (inode == root_inode)
+ return root_inode;
+
+ ti = get_tracefs(inode);
+
+ if (ti->flags & TRACEFS_INSTANCE_INODE)
+ return inode;
+ } while ((parent = parent->d_parent));
+
+ return NULL;
+}
+
/**
* tracefs_create_file - create a file in the tracefs filesystem
* @name: a pointer to a string containing the name of the file to create.
@@ -603,6 +623,7 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
struct dentry *parent, void *data,
const struct file_operations *fops)
{
+ struct tracefs_inode *ti;
struct dentry *dentry;
struct inode *inode;
@@ -621,7 +642,11 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
if (unlikely(!inode))
return tracefs_failed_creating(dentry);
+ ti = get_tracefs(inode);
+ ti->private = instance_inode(parent, inode);
+
inode->i_mode = mode;
+ inode->i_op = &tracefs_file_inode_operations;
inode->i_fop = fops ? fops : &tracefs_file_operations;
inode->i_private = data;
inode->i_uid = d_inode(dentry->d_parent)->i_uid;
@@ -634,6 +659,7 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
static struct dentry *__create_dir(const char *name, struct dentry *parent,
const struct inode_operations *ops)
{
+ struct tracefs_inode *ti;
struct dentry *dentry = tracefs_start_creating(name, parent);
struct inode *inode;
@@ -651,6 +677,9 @@ static struct dentry *__create_dir(const char *name, struct dentry *parent,
inode->i_uid = d_inode(dentry->d_parent)->i_uid;
inode->i_gid = d_inode(dentry->d_parent)->i_gid;
+ ti = get_tracefs(inode);
+ ti->private = instance_inode(parent, inode);
+
/* directory inodes start off with i_nlink == 2 (for "." entry) */
inc_nlink(inode);
d_instantiate(dentry, inode);
@@ -681,7 +710,7 @@ struct dentry *tracefs_create_dir(const char *name, struct dentry *parent)
if (security_locked_down(LOCKDOWN_TRACEFS))
return NULL;
- return __create_dir(name, parent, &simple_dir_inode_operations);
+ return __create_dir(name, parent, &tracefs_dir_inode_operations);
}
/**
@@ -712,7 +741,7 @@ __init struct dentry *tracefs_create_instance_dir(const char *name,
if (WARN_ON(tracefs_ops.mkdir || tracefs_ops.rmdir))
return NULL;
- dentry = __create_dir(name, parent, &tracefs_dir_inode_operations);
+ dentry = __create_dir(name, parent, &tracefs_instance_dir_inode_operations);
if (!dentry)
return NULL;
diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
index 42bdeb471a07..12b7d0150ae9 100644
--- a/fs/tracefs/internal.h
+++ b/fs/tracefs/internal.h
@@ -5,6 +5,9 @@
enum {
TRACEFS_EVENT_INODE = BIT(1),
TRACEFS_EVENT_TOP_INODE = BIT(2),
+ TRACEFS_GID_PERM_SET = BIT(3),
+ TRACEFS_UID_PERM_SET = BIT(4),
+ TRACEFS_INSTANCE_INODE = BIT(5),
};
struct tracefs_inode {
--
2.42.0
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 == dentry->d_inode->i_sb. And it's the same for all
dentries on given superblock. So what's the point of that dance?
If you want the root inode, just go for d_inode(dentry->d_sb->s_root)
and be done with that...
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 parent is NULL then use root inode */
> + if (!parent)
> + return root_inode;
> +
> + /* Find the inode that is flagged as an instance or the root inode */
> + do {
> + inode = d_inode(parent);
> + if (inode == root_inode)
> + return root_inode;
> +
> + ti = get_tracefs(inode);
> +
> + if (ti->flags & TRACEFS_INSTANCE_INODE)
> + return inode;
> + } while ((parent = parent->d_parent));
*blink*
This is equivalent to
...
parent = parent->d_parent;
} while (true);
->d_parent is *never* NULL. And what the hell is that loop supposed to do,
anyway? Find the nearest ancestor tagged with TRACEFS_INSTANCE_INODE?
If root is not marked that way, I would suggest
if (!parent)
parent = inode->i_sb->s_root;
while (!IS_ROOT(parent)) {
struct tracefs_inode *ti = get_tracefs(parent->d_inode);
if (ti->flags & TRACEFS_INSTANCE_INODE)
break;
parent = parent->d_parent;
}
return parent->d_inode;
On Thu, 4 Jan 2024 01:59:10 +0000
Al Viro <[email protected]> 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;
> > +
> > + root_inode = d_inode(inode->i_sb->s_root);
> > +
> > + /* If parent is NULL then use root inode */
> > + if (!parent)
> > + return root_inode;
> > +
> > + /* Find the inode that is flagged as an instance or the root inode */
> > + do {
> > + inode = d_inode(parent);
> > + if (inode == root_inode)
> > + return root_inode;
> > +
> > + ti = get_tracefs(inode);
> > +
> > + if (ti->flags & TRACEFS_INSTANCE_INODE)
> > + return inode;
> > + } while ((parent = parent->d_parent));
>
> *blink*
>
> This is equivalent to
> ...
> parent = parent->d_parent;
> } while (true);
Yeah, that loop went through a few iterations as I first thought that root
was a tracefs_inode and the get_tracefs() would work on it. No, it was not,
and it caused a cash. But I didn't rewrite the loop well after fixing that.
I was also not sure if parent could be NULL, and wanted to protect against it.
>
> ->d_parent is *never* NULL. And what the hell is that loop supposed to do,
> anyway? Find the nearest ancestor tagged with TRACEFS_INSTANCE_INODE?
>
> If root is not marked that way, I would suggest
> if (!parent)
> parent = inode->i_sb->s_root;
> while (!IS_ROOT(parent)) {
> struct tracefs_inode *ti = get_tracefs(parent->d_inode);
> if (ti->flags & TRACEFS_INSTANCE_INODE)
> break;
> parent = parent->d_parent;
> }
> return parent->d_inode;
Sure, I could rewrite it that way too.
Thanks,
-- Steve
On Thu, 4 Jan 2024 01:48:37 +0000
Al Viro <[email protected]> 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 all, for any positive dentry we have
> dentry->d_sb == dentry->d_inode->i_sb. And it's the same for all
> dentries on given superblock. So what's the point of that dance?
> If you want the root inode, just go for d_inode(dentry->d_sb->s_root)
> and be done with that...
That was more of thinking that the dentry and dentry->d_parent are
different. As dentry is part of eventfs and dentry->d_parent is part of
tracefs. Currently they both have the same superblock so yeah, I could just
write it that way too and it would work. But in my head, I was thinking
that they behave differently and maybe one day eventfs would get its own
superblock which would not work.
To explain this better:
/sys/kernel/tracing/ is the parent of /sys/kernel/tracing/events
But everything but "events" in /sys/kernel/tracing/* is part of tracefs.
Everything in /sys/kernel/tracing/events is part of eventfs.
That was my thought process. But as both tracefs and eventfs still use
tracefs_get_inode(), it would work as you state.
I'll update that, as I don't foresee that eventfs will become its own file
system.
Thanks,
-- Steve
On Wed, Jan 03, 2024 at 09:25:06PM -0500, Steven Rostedt wrote:
> On Thu, 4 Jan 2024 01:48:37 +0000
> Al Viro <[email protected]> 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 all, for any positive dentry we have
> > dentry->d_sb == dentry->d_inode->i_sb. And it's the same for all
> > dentries on given superblock. So what's the point of that dance?
> > If you want the root inode, just go for d_inode(dentry->d_sb->s_root)
> > and be done with that...
>
> That was more of thinking that the dentry and dentry->d_parent are
> different. As dentry is part of eventfs and dentry->d_parent is part of
> tracefs.
???
>Currently they both have the same superblock so yeah, I could just
> write it that way too and it would work. But in my head, I was thinking
> that they behave differently and maybe one day eventfs would get its own
> superblock which would not work.
->d_parent *always* points to the same filesystem; if you get an (automounted?)
mountpoint there, ->d_parent simply won't work - it will point to dentry itself.
> To explain this better:
>
> /sys/kernel/tracing/ is the parent of /sys/kernel/tracing/events
>
> But everything but "events" in /sys/kernel/tracing/* is part of tracefs.
> Everything in /sys/kernel/tracing/events is part of eventfs.
>
> That was my thought process. But as both tracefs and eventfs still use
> tracefs_get_inode(), it would work as you state.
>
> I'll update that, as I don't foresee that eventfs will become its own file
> system.
There is no way to get to underlying mountpoint by dentry - simply because
the same fs instance can be mounted in any number of places.
A crude overview of taxonomy:
file_system_type: what filesystem instances belong to. Not quite the same
thing as fs driver (one driver can provide several of those). Usually
it's 1-to-1, but that's not required (e.g. NFS vs NFSv4, or ext[234], or...).
super_block: individual filesystem instance. Hosts dentry tree (connected or
several disconnected parts - think NFSv4 or the state while trying to get
a dentry by fhandle, etc.).
dentry: object in a filesystem's directory tree(s). Always belongs to
specific filesystem instance - that relationship never changes. Tree
structure (and names) _within_ _filesystem_ belong on that level.
->d_parent is part of that tree structure; never NULL, root of a (sub)tree
has it pointing to itself. Might be negative, might refer to a filesystem object
(file, directory, symlink, etc.).
inode: filesystem object (file, directory, etc.). Always belongs to
specific filesystem instance. Non-directory inodes might have any
number of dentry instances (aliases) refering to it; a directory one - no
more than one. Filesystem object contents belongs here; multiple hardlinks
have different dentries and the same inode. Of course, filesystem type in
question might have no such thing as multiple hardlinks - that's up to
filesystem. In general there is no way to find (or enumerate) such links;
e.g. a local filesystem might have an extra hardlink somewhere we had
never looked at and there won't be any dentries for such hardlinks and
no way to get them short of doing the full search of the entire tree.
The situation with e.g. NFS client is even worse, obviously.
mount: in a sense, mount to super_block is what dentry is to inode. It
provides a view of (sub)tree hosted in given filesystem instance. The
same filesystem may have any number of mounts, refering to its subtrees
(possibly the same subtree for each, possibly all different - up to
the callers of mount(2)). They form mount tree(s) - that's where the
notions related to "this mounted on top of that" belong. Note that
they can be moved around - with no telling the filesystem about that
happening. Again, there's no such thing as "the mountpoint of given
filesystem instance" - it might be mounted in any number of places
at the same time. Specific mount - sure, no problem, except that it
can move around.
namespace: mount tree. Unlike everything prior, this one is a part of
process state - same as descriptor table, mappings, etc.
file: opened IO channel. It does refer to specific mount and specific
dentry (and thus filesystem instance and an inode on it). Current
IO position lives here, so does any per-open(2) state.
descriptor table: mapping from numbers to IO channels (opened files).
Again, a part of process state. dup() creates a new entry, with
reference to the same file as the old one; multiple open() of the
same pathname will each yield a separate opened file. _Some_ state
belongs here (close-on-exec, mostly). Note that there's no such
thing as "the descriptor of this file" - not even "the user-supplied
number that had been used to get the file we are currently reading
from", since that number might be refering to something entirely
different right after we'd resolved it to opened file and that
happens *without* disrupting the operation.
On Thu, 4 Jan 2024 04:39:45 +0000
Al Viro <[email protected]> wrote:
> On Wed, Jan 03, 2024 at 09:25:06PM -0500, Steven Rostedt wrote:
> > On Thu, 4 Jan 2024 01:48:37 +0000
> > Al Viro <[email protected]> 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 all, for any positive dentry we have
> > > dentry->d_sb == dentry->d_inode->i_sb. And it's the same for all
> > > dentries on given superblock. So what's the point of that dance?
> > > If you want the root inode, just go for d_inode(dentry->d_sb->s_root)
> > > and be done with that...
> >
> > That was more of thinking that the dentry and dentry->d_parent are
> > different. As dentry is part of eventfs and dentry->d_parent is part of
> > tracefs.
>
> ???
>
> >Currently they both have the same superblock so yeah, I could just
> > write it that way too and it would work. But in my head, I was thinking
> > that they behave differently and maybe one day eventfs would get its own
> > superblock which would not work.
>
> ->d_parent *always* points to the same filesystem; if you get an (automounted?)
> mountpoint there, ->d_parent simply won't work - it will point to dentry itself.
>
This is the "tribal knowledge" I'm talking about. I really didn't know how
the root dentry parent worked. I guess that makes sense, as it matches the
'..' of a directory, and the '/' directory '..' points to itself. Although
mounted file systems do not behave that way. My /proc/.. is '/'. I just
figured that the dentry->d_parent would be similar. Learn something everyday.
> > To explain this better:
> >
> > /sys/kernel/tracing/ is the parent of /sys/kernel/tracing/events
> >
> > But everything but "events" in /sys/kernel/tracing/* is part of tracefs.
> > Everything in /sys/kernel/tracing/events is part of eventfs.
> >
> > That was my thought process. But as both tracefs and eventfs still use
> > tracefs_get_inode(), it would work as you state.
> >
> > I'll update that, as I don't foresee that eventfs will become its own file
> > system.
>
> There is no way to get to underlying mountpoint by dentry - simply because
> the same fs instance can be mounted in any number of places.
OK, so the dentry is still separate from the path and tied closer to the
inode.
>
> A crude overview of taxonomy:
>
> file_system_type: what filesystem instances belong to. Not quite the same
> thing as fs driver (one driver can provide several of those). Usually
> it's 1-to-1, but that's not required (e.g. NFS vs NFSv4, or ext[234], or...).
I don't know the difference between NFS and NFSv4 as I just used whatever
was the latest. But I understand the ext[234] part.
>
> super_block: individual filesystem instance. Hosts dentry tree (connected or
> several disconnected parts - think NFSv4 or the state while trying to get
> a dentry by fhandle, etc.).
I don't know how NFSv4 works, I'm only a user of it, I never actually
looked at the code. So that's not the best example, at least for me.
>
> dentry: object in a filesystem's directory tree(s). Always belongs to
> specific filesystem instance - that relationship never changes. Tree
> structure (and names) _within_ _filesystem_ belong on that level.
> ->d_parent is part of that tree structure; never NULL, root of a (sub)tree
> has it pointing to itself. Might be negative, might refer to a filesystem object
> (file, directory, symlink, etc.).
This is useful.
>
> inode: filesystem object (file, directory, etc.). Always belongs to
> specific filesystem instance. Non-directory inodes might have any
> number of dentry instances (aliases) refering to it; a directory one - no
> more than one.
This above is very useful knowledge that I did not know. That directory
inodes can only have a single dentry.
> Filesystem object contents belongs here; multiple hardlinks
> have different dentries and the same inode.
So, can I assume that an inode could only have as many dentries as hard
links? I know directories are only allowed to have a single hard link. Is
that why they can only have a single dentry?
> Of course, filesystem type in
> question might have no such thing as multiple hardlinks - that's up to
> filesystem. In general there is no way to find (or enumerate) such links;
> e.g. a local filesystem might have an extra hardlink somewhere we had
> never looked at and there won't be any dentries for such hardlinks and
> no way to get them short of doing the full search of the entire tree.
> The situation with e.g. NFS client is even worse, obviously.
>
> mount: in a sense, mount to super_block is what dentry is to inode. It
> provides a view of (sub)tree hosted in given filesystem instance. The
> same filesystem may have any number of mounts, refering to its subtrees
> (possibly the same subtree for each, possibly all different - up to
> the callers of mount(2)). They form mount tree(s) - that's where the
> notions related to "this mounted on top of that" belong. Note that
> they can be moved around - with no telling the filesystem about that
> happening. Again, there's no such thing as "the mountpoint of given
> filesystem instance" - it might be mounted in any number of places
> at the same time. Specific mount - sure, no problem, except that it
> can move around.
>
> namespace: mount tree. Unlike everything prior, this one is a part of
> process state - same as descriptor table, mappings, etc.
And I'm guessing namespace is for containers. At least that's what I've
been assuming they are for.
>
> file: opened IO channel. It does refer to specific mount and specific
> dentry (and thus filesystem instance and an inode on it). Current
> IO position lives here, so does any per-open(2) state.
And IIUC, this is what maps to a processes fd table. That is, the process's
file descriptor number it passes to the kernel will be mapped to this
"file".
>
> descriptor table: mapping from numbers to IO channels (opened files).
This is that "process fd table" I mentioned above (I wrote that before
reading this).
> Again, a part of process state. dup() creates a new entry, with
> reference to the same file as the old one; multiple open() of the
Hmm, wouldn't "dup()" create another "file" that just points to the same
dentry? It wouldn't be the "same file", or did you mean "file" from the
user space point of view?
> same pathname will each yield a separate opened file. _Some_ state
> belongs here (close-on-exec, mostly). Note that there's no such
> thing as "the descriptor of this file" - not even "the user-supplied
> number that had been used to get the file we are currently reading
> from", since that number might be refering to something entirely
> different right after we'd resolved it to opened file and that
> happens *without* disrupting the operation.
This last paragraph confused me. What do you mean by ""referring to
something entirely different"?
Thanks for this overview. It was very useful, and something I think we
should add to kernel doc. I did read Documentation/filesystems/vfs.rst but
honestly, I think your writeup here is a better overview.
-- Steve
On Thu, Jan 04, 2024 at 10:05:44AM -0500, Steven Rostedt wrote:
> This is the "tribal knowledge" I'm talking about. I really didn't know how
> the root dentry parent worked. I guess that makes sense, as it matches the
> '..' of a directory, and the '/' directory '..' points to itself. Although
> mounted file systems do not behave that way. My /proc/.. is '/'. I just
> figured that the dentry->d_parent would be similar. Learn something everyday.
What would you expect to happen if you have the same filesystem mounted in
several places? Having separate dentry trees would be a nightmare - you'd
get cache coherency problems from hell. It's survivable for procfs, but
for something like a normal local filesystem it'd become very painful.
And if we want them to share dentry tree, how do you choose where the ..
would lead from the root dentry?
The way it's done is that linkage between the trees is done separately -
there's a tree of struct mount (well, forest, really - different processes
can easily have separate trees, which is how namespaces are done) and
each node in the mount tree refers to a dentry (sub)tree in some filesystem
instance. Location is represented by (mount, dentry) pair and handling of
.. is basically (modulo refcounting, locking, error handling, etc.)
while dentry == subtree_root(mount) && mount != mountpoint_mount(mount)
// cross into the mountpoint under it
dentry = mountpoint_dentry(mount)
mount = mountpoint_mount(mount)
go_into(mount, dentry->d_parent)
Note that you can have e.g. /usr/lib/gcc/x86_64-linux-gnu/12 mounted on /mnt/blah:
; mount --bind /usr/lib/gcc/x86_64-linux-gnu/12 /mnt/blah
will do it. Then e.g. /mnt/blah/include will resolve to the same dentry as
/usr/lib/gcc/x86_64-linux-gnu/12/include, etc.
; chdir /mnt/blah
; ls
32 crtprec80.o libgomp.so libsanitizer.spec
cc1 g++-mapper-server libgomp.spec libssp_nonshared.a
cc1plus include libitm.a libstdc++.a
collect2 libasan.a libitm.so libstdc++fs.a
crtbegin.o libasan_preinit.o libitm.spec libstdc++.so
crtbeginS.o libasan.so liblsan.a libsupc++.a
crtbeginT.o libatomic.a liblsan_preinit.o libtsan.a
crtend.o libatomic.so liblsan.so libtsan_preinit.o
crtendS.o libbacktrace.a liblto_plugin.so libtsan.so
crtfastmath.o libcc1.so libobjc.a libubsan.a
crtoffloadbegin.o libgcc.a libobjc_gc.a libubsan.so
crtoffloadend.o libgcc_eh.a libobjc_gc.so lto1
crtoffloadtable.o libgcc_s.so libobjc.so lto-wrapper
crtprec32.o libgcov.a libquadmath.a plugin
crtprec64.o libgomp.a libquadmath.so x32
We obviously want .. to resolve to /mnt, though.
; ls ..
; ls /usr/lib/gcc/x86_64-linux-gnu/
12
So the trigger for "cross into underlying mountpoint" has to be "dentry is
the root of subtree mount refers to" - it depends upon the mount we are
in.
> > Filesystem object contents belongs here; multiple hardlinks
> > have different dentries and the same inode.
>
> So, can I assume that an inode could only have as many dentries as hard
> links? I know directories are only allowed to have a single hard link. Is
> that why they can only have a single dentry?
Not quite. Single alias for directories is more about cache coherency
fun; we really can't afford multiple aliases for those. For non-directories
it's possible to have an entirely disconnected dentry refering to that
sucker; if somebody hands you an fhandle with no indication of the parent
directory, you might end up having to do one of those, no matter how many
times you find the same inode later. Not an issue for tracefs, though.
> > namespace: mount tree. Unlike everything prior, this one is a part of
> > process state - same as descriptor table, mappings, etc.
>
> And I'm guessing namespace is for containers. At least that's what I've
> been assuming they are for.
It predates containers by quite a few years, but yes, that's one of the
users. It is related to virtual machines, in the same sense the set
of memory mappings is - each thread can be thought of as a VM, with
a bunch of components. Just as mmap() manipulates the virtual address
translation for the threads that share memory space with the caller,
mount() manipulates the pathname resolution for the threads that share
the namespace with the caller.
> > descriptor table: mapping from numbers to IO channels (opened files).
>
> This is that "process fd table" I mentioned above (I wrote that before
> reading this).
>
> > Again, a part of process state. dup() creates a new entry, with
> > reference to the same file as the old one; multiple open() of the
>
> Hmm, wouldn't "dup()" create another "file" that just points to the same
> dentry? It wouldn't be the "same file", or did you mean "file" from the
> user space point of view?
No. The difference between open() and dup() is that the latter will
result in a descriptor that really refers to the same file. Current
IO position belongs to IO channel; it doesn't matter for e.g. terminals,
but for regular file it immediately becomes an issue.
fd1 = open("foo", 0);
fd2 = open("foo", 0);
read(fd1, &c1, 1);
read(fd2, &c2, 1);
will result in the first byte of foo read into c1 and c2, but
fd1 = open("foo", 0);
fd2 = dup(fd1);
read(fd1, &c1, 1);
read(fd2, &c2, 1);
will have the first byte of foo in c1 and the second one - in c2.
open() yields a new IO channel attached to new descriptor; dup()
(and dup2()) attaches the existing IO channel to new descriptor.
fork() acts like dup() in that respect - child gets its descriptor
table populated with references to the same IO channels as the
parent does.
Any Unix since about '71 has it done that way and the same goes
for NT, DOS, etc. - you can't implement redirects to/from regular
files without that distinction.
Unfortunately, the terms are clumsy as hell - POSIX ends up with
"file descriptor" (for numbers) vs. "file description" (for IO
channels), which is hard to distinguish when reading and just
as hard to distinguish when listening. "Opened file" (as IO
channel) vs. "file on disc" (as collection of data that might
be accessed via said channels) distinction on top of that also
doesn't help, to put it mildly. It's many decades too late to
do anything about, unfortunately. Pity the UNIX 101 students... ;-/
The bottom line:
* struct file represents an IO channel; it might be operating
on various objects, including regular files, pipes, sockets, etc.
* current IO position is a property of IO channel.
* struct files_struct represents a descriptor table; each of
those maps numbers to IO channels.
* each thread uses a descriptor table to turn numbers ("file
descriptors") into struct file references. Different threads might
share the same descriptor table or have separate descriptor tables.
current->files points to the descriptor table of the current thread.
* open() creates a new IO channel and attaches it to an
unused position in descriptor table.
* dup(n) takes the IO channel from position 'n' in descriptor
table and attaches it to an unused position.
* dup2(old, new) takes the IO channel from position 'old' and
attaches it to position 'new'; if there used to be something in position
'new', it gets detached.
* close(n) takes the IO channel from position 'n', flushes and
detaches it. Note that it IO channel itself is *NOT* closed until
all references to it are gone. E.g. open() + fork() + (in parent) close()
will end up with the child's descriptor table keeping a reference to
IO channel established by open(); close() in parent will not shut the
channel down. The same goes for implicit close() done by dup2() or
by exit(), etc.
* things like mmap() retain struct file references;
open() + mmap() + close() ends up with struct file left (in vma->vm_file)
alive and well for as long as the mapping exists, nevermind the reference
that used to be in descriptor table. In other words, IO channels can
exist with no references in any descriptor tables. There are other
ways for such situation to occur (e.g. SCM_RIGHTS stuff); it's entirely
normal.
> > same pathname will each yield a separate opened file. _Some_ state
> > belongs here (close-on-exec, mostly). Note that there's no such
> > thing as "the descriptor of this file" - not even "the user-supplied
> > number that had been used to get the file we are currently reading
> > from", since that number might be refering to something entirely
> > different right after we'd resolved it to opened file and that
> > happens *without* disrupting the operation.
>
> This last paragraph confused me. What do you mean by ""referring to
> something entirely different"?
Two threads share descriptor table; one of them is in
read(fd, ...), another does dup2(fd2, fd). If read() gets past the
point where it gets struct file reference, it will keep accessing that
IO channel. dup2() will replace the reference in descriptor table,
but that won't disrupt the read()...
>
> Thanks for this overview. It was very useful, and something I think we
> should add to kernel doc. I did read Documentation/filesystems/vfs.rst but
> honestly, I think your writeup here is a better overview.
At the very least it would need serious reordering ;-/
On Thu, Jan 04, 2024 at 10:05:44AM -0500, Steven Rostedt wrote:
> > file_system_type: what filesystem instances belong to. Not quite the same
> > thing as fs driver (one driver can provide several of those). Usually
> > it's 1-to-1, but that's not required (e.g. NFS vs NFSv4, or ext[234], or...).
>
> I don't know the difference between NFS and NFSv4 as I just used whatever
> was the latest. But I understand the ext[234] part.
What Al's sying is that nfs.ko provides both nfs_fs_type and
nfs4_fs_type. ext4.ko provides ext2_fs_type, ext3_fs_type and
ext4_fs_type. This is allowed but anomalous. Most filesystems provide
only one, eg ocfs2_fs_type.
> >
> > super_block: individual filesystem instance. Hosts dentry tree (connected or
> > several disconnected parts - think NFSv4 or the state while trying to get
> > a dentry by fhandle, etc.).
>
> I don't know how NFSv4 works, I'm only a user of it, I never actually
> looked at the code. So that's not the best example, at least for me.
Right, so NFS (v4 or otherwise) is Special. In the protocol, files
are identified by a thing called an fhandle. This is (iirc) a 32-byte
identifier which must persist across server reboot. Originally it was
probably supposed to encode dev_t plus ino_t plus generation number.
But you can do all kinds of things in the NFS protocol with an fhandle
that you need a dentry for in Linux (like path walks). Unfortunately,
clients can't be told "Hey, we've lost context, please rewalk" (which
would have other problems anyway), so we need a way to find the dentry
for an fhandle. I understand this very badly, but essentially we end
up looking for canonical ones, and then creating isolated trees of
dentries if we can't find them. Sometimes we then graft these isolated
trees into the canonical spots if we end up connecting them through
various filesystem activity.
At least that's my understanding which probably contains several
misunderstandings.
> > Filesystem object contents belongs here; multiple hardlinks
> > have different dentries and the same inode.
>
> So, can I assume that an inode could only have as many dentries as hard
> links? I know directories are only allowed to have a single hard link. Is
> that why they can only have a single dentry?
There could be more. For example, I could open("A"); ln("A", "B");
open("B"); rm("A"); ln("B", "C"); open("C"); rm("B").
Now there are three dentries for this inode, its link count is currently
one and never exceeded two.
> Thanks for this overview. It was very useful, and something I think we
> should add to kernel doc. I did read Documentation/filesystems/vfs.rst but
> honestly, I think your writeup here is a better overview.
Documentation/filesystems/locking.rst is often a better source, although
the two should really be merged. Not for the faint-hearted.
On Thu, 4 Jan 2024 18:25:02 +0000
Al Viro <[email protected]> wrote:
> On Thu, Jan 04, 2024 at 10:05:44AM -0500, Steven Rostedt wrote:
>
> > This is the "tribal knowledge" I'm talking about. I really didn't know how
> > the root dentry parent worked. I guess that makes sense, as it matches the
> > '..' of a directory, and the '/' directory '..' points to itself. Although
> > mounted file systems do not behave that way. My /proc/.. is '/'. I just
> > figured that the dentry->d_parent would be similar. Learn something everyday.
>
> What would you expect to happen if you have the same filesystem mounted in
> several places? Having separate dentry trees would be a nightmare - you'd
> get cache coherency problems from hell. It's survivable for procfs, but
> for something like a normal local filesystem it'd become very painful.
> And if we want them to share dentry tree, how do you choose where the ..
> would lead from the root dentry?
My mistake was thinking that the dentry was attached more to the path than
the inode. But that doesn't seem to be the case. I wasn't sure if there was
a way to get to a dentry from the inode. I see the i_dentry list, which is
a list, where I got some of my idea that dentry was closer to path than inode.
>
> The way it's done is that linkage between the trees is done separately -
> there's a tree of struct mount (well, forest, really - different processes
> can easily have separate trees, which is how namespaces are done) and
> each node in the mount tree refers to a dentry (sub)tree in some filesystem
> instance. Location is represented by (mount, dentry) pair and handling of
> .. is basically (modulo refcounting, locking, error handling, etc.)
> while dentry == subtree_root(mount) && mount != mountpoint_mount(mount)
> // cross into the mountpoint under it
> dentry = mountpoint_dentry(mount)
> mount = mountpoint_mount(mount)
> go_into(mount, dentry->d_parent)
>
> Note that you can have e.g. /usr/lib/gcc/x86_64-linux-gnu/12 mounted on /mnt/blah:
> ; mount --bind /usr/lib/gcc/x86_64-linux-gnu/12 /mnt/blah
> will do it. Then e.g. /mnt/blah/include will resolve to the same dentry as
> /usr/lib/gcc/x86_64-linux-gnu/12/include, etc.
> ; chdir /mnt/blah
> ; ls
> 32 crtprec80.o libgomp.so libsanitizer.spec
> cc1 g++-mapper-server libgomp.spec libssp_nonshared.a
> cc1plus include libitm.a libstdc++.a
> collect2 libasan.a libitm.so libstdc++fs.a
> crtbegin.o libasan_preinit.o libitm.spec libstdc++.so
> crtbeginS.o libasan.so liblsan.a libsupc++.a
> crtbeginT.o libatomic.a liblsan_preinit.o libtsan.a
> crtend.o libatomic.so liblsan.so libtsan_preinit.o
> crtendS.o libbacktrace.a liblto_plugin.so libtsan.so
> crtfastmath.o libcc1.so libobjc.a libubsan.a
> crtoffloadbegin.o libgcc.a libobjc_gc.a libubsan.so
> crtoffloadend.o libgcc_eh.a libobjc_gc.so lto1
> crtoffloadtable.o libgcc_s.so libobjc.so lto-wrapper
> crtprec32.o libgcov.a libquadmath.a plugin
> crtprec64.o libgomp.a libquadmath.so x32
>
> We obviously want .. to resolve to /mnt, though.
> ; ls ..
> ; ls /usr/lib/gcc/x86_64-linux-gnu/
> 12
>
> So the trigger for "cross into underlying mountpoint" has to be "dentry is
> the root of subtree mount refers to" - it depends upon the mount we are
> in.
>
> > > Filesystem object contents belongs here; multiple hardlinks
> > > have different dentries and the same inode.
> >
> > So, can I assume that an inode could only have as many dentries as hard
> > links? I know directories are only allowed to have a single hard link. Is
> > that why they can only have a single dentry?
>
> Not quite. Single alias for directories is more about cache coherency
> fun; we really can't afford multiple aliases for those. For non-directories
> it's possible to have an entirely disconnected dentry refering to that
> sucker; if somebody hands you an fhandle with no indication of the parent
> directory, you might end up having to do one of those, no matter how many
> times you find the same inode later. Not an issue for tracefs, though.
>
> > > namespace: mount tree. Unlike everything prior, this one is a part of
> > > process state - same as descriptor table, mappings, etc.
> >
> > And I'm guessing namespace is for containers. At least that's what I've
> > been assuming they are for.
>
> It predates containers by quite a few years, but yes, that's one of the
> users. It is related to virtual machines, in the same sense the set
> of memory mappings is - each thread can be thought of as a VM, with
> a bunch of components. Just as mmap() manipulates the virtual address
> translation for the threads that share memory space with the caller,
> mount() manipulates the pathname resolution for the threads that share
> the namespace with the caller.
>
> > > descriptor table: mapping from numbers to IO channels (opened files).
> >
> > This is that "process fd table" I mentioned above (I wrote that before
> > reading this).
> >
> > > Again, a part of process state. dup() creates a new entry, with
> > > reference to the same file as the old one; multiple open() of the
> >
> > Hmm, wouldn't "dup()" create another "file" that just points to the same
> > dentry? It wouldn't be the "same file", or did you mean "file" from the
> > user space point of view?
>
> No. The difference between open() and dup() is that the latter will
> result in a descriptor that really refers to the same file. Current
> IO position belongs to IO channel; it doesn't matter for e.g. terminals,
> but for regular file it immediately becomes an issue.
> fd1 = open("foo", 0);
> fd2 = open("foo", 0);
> read(fd1, &c1, 1);
> read(fd2, &c2, 1);
> will result in the first byte of foo read into c1 and c2, but
> fd1 = open("foo", 0);
> fd2 = dup(fd1);
> read(fd1, &c1, 1);
> read(fd2, &c2, 1);
> will have the first byte of foo in c1 and the second one - in c2.
> open() yields a new IO channel attached to new descriptor; dup()
> (and dup2()) attaches the existing IO channel to new descriptor.
> fork() acts like dup() in that respect - child gets its descriptor
> table populated with references to the same IO channels as the
> parent does.
Ah, looking at the code I use dup() in, it's mostly for pipes in
and for redirecting stdout,stdin, etc. So yeah, that makes sense.
>
> Any Unix since about '71 has it done that way and the same goes
> for NT, DOS, etc. - you can't implement redirects to/from regular
> files without that distinction.
Yep, which is what I used it for. Just forgot the details.
>
> > > same pathname will each yield a separate opened file. _Some_ state
> > > belongs here (close-on-exec, mostly). Note that there's no such
> > > thing as "the descriptor of this file" - not even "the user-supplied
> > > number that had been used to get the file we are currently reading
> > > from", since that number might be refering to something entirely
> > > different right after we'd resolved it to opened file and that
> > > happens *without* disrupting the operation.
> >
> > This last paragraph confused me. What do you mean by ""referring to
> > something entirely different"?
>
> Two threads share descriptor table; one of them is in
> read(fd, ...), another does dup2(fd2, fd). If read() gets past the
> point where it gets struct file reference, it will keep accessing that
> IO channel. dup2() will replace the reference in descriptor table,
> but that won't disrupt the read()...
Oh, OK. So basically if fd 4 is a reference to /tmp/foo and you open
/tmp/bar which gets fd2, and one thread is reading fd 4 (/tmp/foo), the
other thread doing dup2(fd2, fd) will make fd 4 a reference to /tmp/bar but
the read will finish reading /tmp/foo.
But if the first thread were to do another read(fd, ...) it would then read
/tmp/bar. In other words, it allows read() to stay atomic with respect to
what it is reading until it returns.
>
> >
> > Thanks for this overview. It was very useful, and something I think we
> > should add to kernel doc. I did read Documentation/filesystems/vfs.rst but
> > honestly, I think your writeup here is a better overview.
>
> At the very least it would need serious reordering ;-/
Yeah, but this is all great information. Thanks for explaining it.
-- Steve
On Thu, 4 Jan 2024 18:25:02 +0000
Al Viro <[email protected]> wrote:
> Unfortunately, the terms are clumsy as hell - POSIX ends up with
> "file descriptor" (for numbers) vs. "file description" (for IO
> channels), which is hard to distinguish when reading and just
> as hard to distinguish when listening. "Opened file" (as IO
> channel) vs. "file on disc" (as collection of data that might
> be accessed via said channels) distinction on top of that also
> doesn't help, to put it mildly. It's many decades too late to
> do anything about, unfortunately. Pity the UNIX 101 students... ;-/
Just so I understand this correctly.
"file descriptor" - is just what maps to a specific inode.
"file description" - is how the file is accessed (position in the file and
flags associated to how it was opened)
Did I get that correct?
-- Steve
On Thu, 4 Jan 2024 at 11:09, Steven Rostedt <[email protected]> wrote:
>
> My mistake was thinking that the dentry was attached more to the path than
> the inode. But that doesn't seem to be the case. I wasn't sure if there was
> a way to get to a dentry from the inode.
Yeah, so dentry->inode and path->dentry are one-way translations,
because the other way can have multiple different cases.
IOW, a path will specify *one* dentry, and a dentry will specily *one*
inode, but one inode can be associated with multiple dentries, and
there may be other undiscovered dentries that *would* point to it but
aren't even cached right now.
And a single dentry can be part of multiple paths, thanks to bind mounts.
The "inode->i_dentry" list is *not* a way to look up all dentries,
because - as mentioned - there may be potential other paths (and thus
other dentries) that lead to the same inode that just haven't been
looked up yet (or that have already been aged out of the cache).
Of course any *particular* filesystem may not have hard links (so one
inode has only one possible dentry), and you may not have bind mounts,
and it might be one of the virtual filesystems where everything is
always in memory, so none of the above problems are guaranteed to be
the case in any *particular* situation.
But it's all part of why the dcache is actually really subtle. It's
not just the RCU lookup rules and the specialized locking (both
reflock and the rather complicated rules about d_lock ordering), it's
also that whole "yeah, the filesystem only sees a 'dentry', but
because of bind mounts the vfs layer actually does things internally
in terms of 'struct path' in order to be able to then show that single
fiolesystem in multiple places".
Etc etc.
There's a reason Al Viro ends up owning the dcache. Nobody else can
wrap their tiny little minds around it all.
Linus
On Thu, Jan 04, 2024 at 02:15:17PM -0500, Steven Rostedt wrote:
> On Thu, 4 Jan 2024 18:25:02 +0000
> Al Viro <[email protected]> wrote:
>
> > Unfortunately, the terms are clumsy as hell - POSIX ends up with
> > "file descriptor" (for numbers) vs. "file description" (for IO
> > channels), which is hard to distinguish when reading and just
> > as hard to distinguish when listening. "Opened file" (as IO
> > channel) vs. "file on disc" (as collection of data that might
> > be accessed via said channels) distinction on top of that also
> > doesn't help, to put it mildly. It's many decades too late to
> > do anything about, unfortunately. Pity the UNIX 101 students... ;-/
>
> Just so I understand this correctly.
>
> "file descriptor" - is just what maps to a specific inode.
No -- file descriptor is a number in fdtable that maps to a struct file.
> "file description" - is how the file is accessed (position in the file and
> flags associated to how it was opened)
file description is posix's awful name for struct file.
On Thu, 4 Jan 2024 at 11:14, Steven Rostedt <[email protected]> wrote:
>
> "file descriptor" - is just what maps to a specific inode.
Nope. Technically and traditionally, file descriptor is just the
integer index that is used to look up a 'struct file *'.
Except in the kernel, we really just tend to use that term (well, I
do) for the 'struct file *' itself, since the integer 'fd' is usually
not really relevant except at the system call interface.
Which is *NOT* the inode, because the 'struct file' has other things
in it (the file position, the permissions that were used at open time
etc, close-on-exec state etc etc).
> "file description" - is how the file is accessed (position in the file and
> flags associated to how it was opened)
That's a horrible term that shouldn't be used at all. Apparently some
people use it for what is our 'struct file *", also known as a "file
table entry". Avoid it.
If anything, just use "fd" for the integer representation, and "file"
for the pointer to a 'struct file".
But most of the time the two are conceptually interchangeable, in that
an 'fd' just translates directly to a 'struct file *'.
Note that while there's that conceptual direct translation, there's
also very much a "time of use" issue, in that a "fd -> file"
translation happens at one particular time and in one particular user
context, and then it's *done* (so closing and possibly re-using the fd
after it's been looked up does not actually affect an existing 'struct
file *').
And while 'fd -> file' lookup is quick and common, the other way
doesn't exist, because multiple 'fd's can map to one 'struct file *'
thanks to dup() (and 'fork()', since a 'fd -> file' translation always
happens within the context of a particular user space, an 'fd' in one
process is obviously not the same as an 'fd' in another one).
Linus
On Thu, 4 Jan 2024 at 11:35, Linus Torvalds
<[email protected]> wrote:
>>
> Which is *NOT* the inode, because the 'struct file' has other things
> in it (the file position, the permissions that were used at open time
> etc, close-on-exec state etc etc).
That close-on-exec thing was a particularly bad example of things that
are in the 'struct file', because it's in fact the only thing that
*isn't* in 'struct file' and is associated directly with the 'int fd'.
But hopefully the intent was clear despite me picking a particularly
bad example.
Linus
On Thu, Jan 04, 2024 at 11:35:37AM -0800, Linus Torvalds wrote:
> > "file description" - is how the file is accessed (position in the file and
> > flags associated to how it was opened)
>
> That's a horrible term that shouldn't be used at all. Apparently some
> people use it for what is our 'struct file *", also known as a "file
> table entry". Avoid it.
Worse, really. As far as I can reconstruct what happened it was something
along the lines of "colloquial expression is 'opened file', but that is
confusing - sounds like a property+noun, so it might be misparsed as
a member of subset of files satisfying the property of 'being opened';
can't have that in a standard, let's come up with something else".
Except that what they did come up with had been much worse, for obvious
linguistic reasons.
The *ONLY* uses for that expression I can think of are
1. When reading POSIX texts, watch out for that one - if you
see them talking about a file descriptor in context where it really
should be about an opened file, check the wording. If it really says
"file descriptOR", it's probably a bug in standard or a codified
bullshit practice. If it says "file descriptION" instead, replace with
"opened file" and move on.
2. An outstanding example of the taste of that bunch.
IO channel would be a saner variant, but it's far too late for that.
The 3-way distinction between descriptor/opened file/file as collection of data
needs to be explained in UNIX 101; it is userland-visible and it has to be
understood. Unfortunately, it's often done in a way that leaves students
seriously confused ;-/
On Wed, Jan 03, 2024 at 08:32:46PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <[email protected]>
>
> 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 the files and directories.
>
> For all files and directories in the top level instance:
>
> /sys/kernel/tracing/*
>
> It will use the root inode as the default permissions. The inode that
> represents: /sys/kernel/tracing (or wherever it is mounted).
>
> When an instance is created:
>
> mkdir /sys/kernel/tracing/instance/foo
>
> The directory "foo" and all its files and directories underneath will use
> the default of what foo is when it was created. A remount of tracefs will
> not affect it.
That kinda sounds like eventfs should actually be a separate filesystem.
But I don't know enough about the relationship between the two concepts.
>
> If a user were to modify the permissions of any file or directory in
> tracefs, it will also no longer be modified by a change in ownership of a
> remount.
Very odd semantics and I would recommend to avoid that. It's just plain
weird imo.
>
> The events directory, if it is in the top level instance, will use the
> tracefs root inode as the default ownership for itself and all the files and
> directories below it.
>
> For the events directory in an instance ("foo"), it will keep the ownership
> of what it was when it was created, and that will be used as the default
> ownership for the files and directories beneath it.
>
> Link: https://lore.kernel.org/linux-trace-kernel/CAHk-=wjVdGkjDXBbvLn2wbZnqP4UsH46E3gqJ9m7UG6DpX2+WA@mail.gmail.com/
>
> Signed-off-by: Steven Rostedt (Google) <[email protected]>
> ---
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?
Aside from optimizing this and the special semantics for this eventfs
stuff that you really should think twice of doing, here's one idea for
an extension that might alleviate some of the pain:
If you need flexible dynamic ownership change to e.g., be able to
delegate (all, a directory, a single file of) tracefs to
unprivileged/containers/whatever then you might want to consider
supporting idmapped mounts for tracefs. Because then you can do stuff
like:
user1@localhost:~/data/scripts$ sudo mount --bind -o X-mount.idmap='g:0:1000:1 u:0:1234:1' /run/ /mnt
user1@localhost:~/data/scripts$ ls -ln /run/
total 12
drwxr-xr-x 2 0 0 40 Jan 5 12:12 credentials
drwx------ 2 0 0 40 Jan 5 11:57 cryptsetup
drwxr-xr-x 2 0 0 60 Jan 5 11:57 dbus
drwx------ 6 0 0 280 Jan 5 11:57 incus_agent
prw------- 1 0 0 0 Jan 5 11:57 initctl
drwxrwxrwt 4 0 0 80 Jan 5 11:57 lock
drwxr-xr-x 3 0 0 60 Jan 5 11:57 log
drwx------ 2 0 0 40 Jan 5 11:57 lvm
-r--r--r-- 1 0 0 33 Jan 5 11:57 machine-id
-rw-r--r-- 1 0 0 101 Jan 5 11:58 motd.dynamic
drwxr-xr-x 2 0 0 40 Jan 5 11:57 mount
drwx------ 2 0 0 40 Jan 5 11:57 multipath
drwxr-xr-x 2 0 0 40 Jan 5 11:57 sendsigs.omit.d
lrwxrwxrwx 1 0 0 8 Jan 5 11:57 shm -> /dev/shm
drwx--x--x 2 0 0 40 Jan 5 11:57 sudo
drwxr-xr-x 24 0 0 660 Jan 5 14:30 systemd
drwxr-xr-x 6 0 0 140 Jan 5 14:30 udev
drwxr-xr-x 4 0 0 80 Jan 5 11:58 user
-rw-rw-r-- 1 0 43 2304 Jan 5 15:15 utmp
user1@localhost:~/data/scripts$ ls -ln /mnt/
total 12
drwxr-xr-x 2 1234 1000 40 Jan 5 12:12 credentials
drwx------ 2 1234 1000 40 Jan 5 11:57 cryptsetup
drwxr-xr-x 2 1234 1000 60 Jan 5 11:57 dbus
drwxr-xr-x 2 1234 1000 40 Jan 5 11:57 incus_agent
prw------- 1 1234 1000 0 Jan 5 11:57 initctl
drwxr-xr-x 2 1234 1000 40 Jan 5 11:57 lock
drwxr-xr-x 3 1234 1000 60 Jan 5 11:57 log
drwx------ 2 1234 1000 40 Jan 5 11:57 lvm
-r--r--r-- 1 1234 1000 33 Jan 5 11:57 machine-id
-rw-r--r-- 1 1234 1000 101 Jan 5 11:58 motd.dynamic
drwxr-xr-x 2 1234 1000 40 Jan 5 11:57 mount
drwx------ 2 1234 1000 40 Jan 5 11:57 multipath
drwxr-xr-x 2 1234 1000 40 Jan 5 11:57 sendsigs.omit.d
lrwxrwxrwx 1 1234 1000 8 Jan 5 11:57 shm -> /dev/shm
drwx--x--x 2 1234 1000 40 Jan 5 11:57 sudo
drwxr-xr-x 24 1234 1000 660 Jan 5 14:30 systemd
drwxr-xr-x 6 1234 1000 140 Jan 5 14:30 udev
drwxr-xr-x 4 1234 1000 80 Jan 5 11:58 user
-rw-rw-r-- 1 1234 65534 2304 Jan 5 15:15 utmp
Where you can see that ownership of this tmpfs instance in this example
is changed. I'm not trying to advocate here but this will probably
ultimately be nicer for your users because it means that a container
manager or whatever can be handed a part of tracefs (or all of it) and
the ownership and access rights for that thing is correct. And you can
get rid of that gid based access completely.
You can change uids, gids, or both. You can specify up to 340 individual
mappings it's quite flexible.
Because then you can have a single tracefs superblock and have multiple
mounts with different ownership for the relevant parts of tracefs that
you want to delegate to whoever. If you need an ownership change you can
then just create another idmapped mount with the new ownership and then
use MOVE_MOUNT_BENEATH + umount to replace that mount.
Probably even know someone that would implement this for you (not me) if
that sounds like something that would cover some of the use-case for the
proposed change here. But maybe I just misunderstood things completely.
On Fri, 5 Jan 2024 15:26:28 +0100
Christian Brauner <[email protected]> wrote:
> On Wed, Jan 03, 2024 at 08:32:46PM -0500, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)" <[email protected]>
> >
> > 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 the files and directories.
> >
> > For all files and directories in the top level instance:
> >
> > /sys/kernel/tracing/*
> >
> > It will use the root inode as the default permissions. The inode that
> > represents: /sys/kernel/tracing (or wherever it is mounted).
> >
> > When an instance is created:
> >
> > mkdir /sys/kernel/tracing/instance/foo
> >
> > The directory "foo" and all its files and directories underneath will use
> > the default of what foo is when it was created. A remount of tracefs will
> > not affect it.
>
> That kinda sounds like eventfs should actually be a separate filesystem.
> But I don't know enough about the relationship between the two concepts.
It may someday become one, as eventfs is used by perf where the rest of the
tracefs system is not.
>
> >
> > If a user were to modify the permissions of any file or directory in
> > tracefs, it will also no longer be modified by a change in ownership of a
> > remount.
>
> Very odd semantics and I would recommend to avoid that. It's just plain
> weird imo.
>
> >
> > The events directory, if it is in the top level instance, will use the
> > tracefs root inode as the default ownership for itself and all the files and
> > directories below it.
> >
> > For the events directory in an instance ("foo"), it will keep the ownership
> > of what it was when it was created, and that will be used as the default
> > ownership for the files and directories beneath it.
> >
> > Link: https://lore.kernel.org/linux-trace-kernel/CAHk-=wjVdGkjDXBbvLn2wbZnqP4UsH46E3gqJ9m7UG6DpX2+WA@mail.gmail.com/
> >
> > Signed-off-by: Steven Rostedt (Google) <[email protected]>
> > ---
>
> 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 the entire machine. It can work with privileged containers though.
The reason for this is because tracefs was based off of debugfs where the
files and directores are created at boot up and mounted later. The reason
to do this was to allow users to mount with gid=GID to allow a given group
to have access to tracing. Without this update, tracefs would ignore it
like debugfs and proc does today.
I think its time I explain the purpose of tracefs and how it came to be.
The tracing system required a way to control tracing and read the traces.
It could have just used a new system like perf (although
/sys/kernel/debug/tracing predates perf), where it created a single ioctl()
like system call do do everything.
As the ftrace tracing came from PREEMPT_RT latency tracer and my own logdev
tracer, which both have an embedded background, I chose an interface that
could work with just an unmodified version of busybox. That is, I wanted it
to work with just cat and echo.
The main difference with tracefs compared to other file systems is that it
is a control interface, where writes happen as much as reads. The data read
is controlled. The closest thing I can think of is how cgroups work.
As tracing is a privileged operation, but something that could be changed
to allow a group to have access to, I wanted to make it easy for an admin
to decide who gets to do what at boot up via the /etc/fstab file.
>
> Aside from optimizing this and the special semantics for this eventfs
> stuff that you really should think twice of doing, here's one idea for
> an extension that might alleviate some of the pain:
>
> If you need flexible dynamic ownership change to e.g., be able to
> delegate (all, a directory, a single file of) tracefs to
> unprivileged/containers/whatever then you might want to consider
> supporting idmapped mounts for tracefs. Because then you can do stuff
> like:
I'm a novice here and have no idea on how id maps work ;-)
>
> user1@localhost:~/data/scripts$ sudo mount --bind -o X-mount.idmap='g:0:1000:1 u:0:1234:1' /run/ /mnt
> user1@localhost:~/data/scripts$ ls -ln /run/
> total 12
> drwxr-xr-x 2 0 0 40 Jan 5 12:12 credentials
> drwx------ 2 0 0 40 Jan 5 11:57 cryptsetup
> drwxr-xr-x 2 0 0 60 Jan 5 11:57 dbus
> drwx------ 6 0 0 280 Jan 5 11:57 incus_agent
> prw------- 1 0 0 0 Jan 5 11:57 initctl
> drwxrwxrwt 4 0 0 80 Jan 5 11:57 lock
> drwxr-xr-x 3 0 0 60 Jan 5 11:57 log
> drwx------ 2 0 0 40 Jan 5 11:57 lvm
> -r--r--r-- 1 0 0 33 Jan 5 11:57 machine-id
> -rw-r--r-- 1 0 0 101 Jan 5 11:58 motd.dynamic
> drwxr-xr-x 2 0 0 40 Jan 5 11:57 mount
> drwx------ 2 0 0 40 Jan 5 11:57 multipath
> drwxr-xr-x 2 0 0 40 Jan 5 11:57 sendsigs.omit.d
> lrwxrwxrwx 1 0 0 8 Jan 5 11:57 shm -> /dev/shm
> drwx--x--x 2 0 0 40 Jan 5 11:57 sudo
> drwxr-xr-x 24 0 0 660 Jan 5 14:30 systemd
> drwxr-xr-x 6 0 0 140 Jan 5 14:30 udev
> drwxr-xr-x 4 0 0 80 Jan 5 11:58 user
> -rw-rw-r-- 1 0 43 2304 Jan 5 15:15 utmp
>
> user1@localhost:~/data/scripts$ ls -ln /mnt/
> total 12
> drwxr-xr-x 2 1234 1000 40 Jan 5 12:12 credentials
> drwx------ 2 1234 1000 40 Jan 5 11:57 cryptsetup
> drwxr-xr-x 2 1234 1000 60 Jan 5 11:57 dbus
> drwxr-xr-x 2 1234 1000 40 Jan 5 11:57 incus_agent
> prw------- 1 1234 1000 0 Jan 5 11:57 initctl
> drwxr-xr-x 2 1234 1000 40 Jan 5 11:57 lock
> drwxr-xr-x 3 1234 1000 60 Jan 5 11:57 log
> drwx------ 2 1234 1000 40 Jan 5 11:57 lvm
> -r--r--r-- 1 1234 1000 33 Jan 5 11:57 machine-id
> -rw-r--r-- 1 1234 1000 101 Jan 5 11:58 motd.dynamic
> drwxr-xr-x 2 1234 1000 40 Jan 5 11:57 mount
> drwx------ 2 1234 1000 40 Jan 5 11:57 multipath
> drwxr-xr-x 2 1234 1000 40 Jan 5 11:57 sendsigs.omit.d
> lrwxrwxrwx 1 1234 1000 8 Jan 5 11:57 shm -> /dev/shm
> drwx--x--x 2 1234 1000 40 Jan 5 11:57 sudo
> drwxr-xr-x 24 1234 1000 660 Jan 5 14:30 systemd
> drwxr-xr-x 6 1234 1000 140 Jan 5 14:30 udev
> drwxr-xr-x 4 1234 1000 80 Jan 5 11:58 user
> -rw-rw-r-- 1 1234 65534 2304 Jan 5 15:15 utmp
>
> Where you can see that ownership of this tmpfs instance in this example
> is changed. I'm not trying to advocate here but this will probably
> ultimately be nicer for your users because it means that a container
> manager or whatever can be handed a part of tracefs (or all of it) and
> the ownership and access rights for that thing is correct. And you can
> get rid of that gid based access completely.
>
> You can change uids, gids, or both. You can specify up to 340 individual
> mappings it's quite flexible.
>
> Because then you can have a single tracefs superblock and have multiple
> mounts with different ownership for the relevant parts of tracefs that
> you want to delegate to whoever. If you need an ownership change you can
> then just create another idmapped mount with the new ownership and then
> use MOVE_MOUNT_BENEATH + umount to replace that mount.
>
> Probably even know someone that would implement this for you (not me) if
> that sounds like something that would cover some of the use-case for the
> proposed change here. But maybe I just misunderstood things completely.
That actually sounds nice. I have no idea how to implement it, but having a
way to bind mount with different permissions looks like a nifty feature to
have. Now, can we do that and still keep the dynamic creation of inodes and
dentries? Does this require having more than one dentry per inode?
-- Steve
> > 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 the entire machine. It can work with privileged containers though.
At least the tracefs interface is easily supportable within a delegation
model. IOW, you have a privileged process that delegates relevant
portions to a container via idmapped mounts _without_ doing the insane thing
and making it mountable by a container aka the fs-to-CVE pipeline.
>
> The reason for this is because tracefs was based off of debugfs where the
> files and directores are created at boot up and mounted later. The reason
> to do this was to allow users to mount with gid=GID to allow a given group
> to have access to tracing. Without this update, tracefs would ignore it
> like debugfs and proc does today.
>
> I think its time I explain the purpose of tracefs and how it came to be.
>
> The tracing system required a way to control tracing and read the traces.
> It could have just used a new system like perf (although
> /sys/kernel/debug/tracing predates perf), where it created a single ioctl()
> like system call do do everything.
>
> As the ftrace tracing came from PREEMPT_RT latency tracer and my own logdev
> tracer, which both have an embedded background, I chose an interface that
> could work with just an unmodified version of busybox. That is, I wanted it
> to work with just cat and echo.
>
> The main difference with tracefs compared to other file systems is that it
> is a control interface, where writes happen as much as reads. The data read
> is controlled. The closest thing I can think of is how cgroups work.
>
> As tracing is a privileged operation, but something that could be changed
> to allow a group to have access to, I wanted to make it easy for an admin
> to decide who gets to do what at boot up via the /etc/fstab file.
Yeah, ok. I think you could achieve the same thing via idmapped mounts. You
just need to swap out the mnt on /sys/kernel/tracing with an idmapped mount.
mount(8) should just give you the ability to specify "map the ids I explicitly
want to remap to something else and for the rest use the identity mapping". I
wanted that for other reasons anyway.
So in one of the next versions of mount(8) you can then do (where --beneath
means place the mount beneath the current one and --replace is
self-explanatory):
sudo mount --beneath -o X-mount.idmap='g:0:1234:1 u:0:0:1' /sys/kernel/tracing
sudo umount /sys/kernel/tracing
or as a shortcut provided by mount(8):
sudo mount --replace -o X-mount.idmap='g:0:1234:1 u:0:0:1' /sys/kernel/tracing
In both cases you replace the mount without unmounting tracefs.
I can illustrate this right now though:
user1@localhost:~$ sudo mount --bind -o X-mount.idmap='g:0:1000:1 u:0:1000:1' /sys/kernel/tracing/ /mnt/
# This is a tool I wrote for testing the patchset I wrote back then.
user1@localhost:~/data/move-mount-beneath$ sudo ./move-mount --beneath --detached /mnt /sys/kernel/tracing
Mounting beneath top mount
Creating anonymous mount
Attaching mount /mnt -> /sys/kernel/tracing
Creating single detached mount
user1@localhost:~/data/move-mount-beneath$
# Now there's two mounts stacked on top of each other.
user1@localhost:~/data/move-mount-beneath$ findmnt | grep tracing
| `-/sys/kernel/tracing tracefs tracefs rw,nosuid,nodev,noexec,relatime,idmapped
| `-/sys/kernel/tracing tracefs tracefs rw,nosuid,nodev,noexec,relatime
user1@localhost:~/data/move-mount-beneath$ sudo ls -al /sys/kernel/tracing/| head
total 0
drwx------ 6 root root 0 Jan 7 13:33 .
drwxr-xr-x 16 root root 0 Jan 7 13:33 ..
-r--r----- 1 root root 0 Jan 7 13:33 README
-r--r----- 1 root root 0 Jan 7 13:33 available_events
-r--r----- 1 root root 0 Jan 7 13:33 available_filter_functions
-r--r----- 1 root root 0 Jan 7 13:33 available_filter_functions_addrs
-r--r----- 1 root root 0 Jan 7 13:33 available_tracers
-rw-r----- 1 root root 0 Jan 7 13:33 buffer_percent
-rw-r----- 1 root root 0 Jan 7 13:33 buffer_size_kb
# Reveal updated mount
user1@localhost:~/data/move-mount-beneath$ sudo umount /sys/kernel/tracing
user1@localhost:~/data/move-mount-beneath$ findmnt | grep tracing
| `-/sys/kernel/tracing tracefs tracefs rw,nosuid,nodev,noexec,relatime,idmapped
user1@localhost:~/data/move-mount-beneath$ sudo ls -al /sys/kernel/tracing/| head
total 0
drwx------ 6 user1 user1 0 Jan 7 13:33 .
drwxr-xr-x 16 root root 0 Jan 7 13:33 ..
-r--r----- 1 user1 user1 0 Jan 7 13:33 README
-r--r----- 1 user1 user1 0 Jan 7 13:33 available_events
-r--r----- 1 user1 user1 0 Jan 7 13:33 available_filter_functions
-r--r----- 1 user1 user1 0 Jan 7 13:33 available_filter_functions_addrs
-r--r----- 1 user1 user1 0 Jan 7 13:33 available_tracers
-rw-r----- 1 user1 user1 0 Jan 7 13:33 buffer_percent
-rw-r----- 1 user1 user1 0 Jan 7 13:33 buffer_size_kb
sudo umount -l /sys/kernel/tracing
and reveal the new mount with updated permissions and at no point in time will
you have had to unmount tracefs itself. No chown needed, no remount needed that
has to touch all inodes.
I did perf numbers for this when I implemented this and there's no meaningful
perf impact for anything below 5 mappings. which covers 80% of the use-cases.
I mean, there are crazy people out there that do have 30 mappings, maybe. And
maybe there's 3 users that go into the hundreds (340 is maximum so struct idmap
still fits into cacheline) because of some LDAP crap or something.
See an ext4 filesystem to open/create 1,000,000 files. Then I looped through
all of the files calling fstat() on each of them 1000 times and calculated the
mean fstat() time for a single file.
| # MAPPINGS | PATCH-NEW |
|--------------|-----------|
| 0 mappings | 158 ns |
| 1 mappings | 157 ns |
| 2 mappings | 158 ns |
| 3 mappings | 161 ns |
| 5 mappings | 165 ns |
| 10 mappings | 199 ns |
| 50 mappings | 218 ns |
| 100 mappings | 229 ns |
| 200 mappings | 239 ns |
| 300 mappings | 240 ns |
| 340 mappings | 248 ns |
>
> >
> > Aside from optimizing this and the special semantics for this eventfs
> > stuff that you really should think twice of doing, here's one idea for
> > an extension that might alleviate some of the pain:
> >
> > If you need flexible dynamic ownership change to e.g., be able to
> > delegate (all, a directory, a single file of) tracefs to
> > unprivileged/containers/whatever then you might want to consider
> > supporting idmapped mounts for tracefs. Because then you can do stuff
> > like:
>
> I'm a novice here and have no idea on how id maps work ;-)
It's no magic and you don't even need to care about it. If you're on
util-linux 2.39 and any kernel post 5.12 the -o X-mount.idmap option
does all the details for you.
Though I still want an api where you can just pass the idmappings directly to
mount_setattr(). That's another topic though.
> have. Now, can we do that and still keep the dynamic creation of inodes and
> dentries?
Yes.
> Does this require having more than one dentry per inode?
No. It's just a topological change. The same subtree exposed at different
locations with the ability of exposing it with different permissions (without
any persistent filesystem-level changes).
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 that don't have a custom
->permission() handler. So there's nothing to do for us here.
* Idmapped mount handling for ->getattr() is done completely by the VFS
if the filesystem doesn't have a custom ->getattr() handler. So we're
done here.
* Tracefs doesn't support attribute changes via ->setattr() (chown(),
chmod etc.). So there's nothing to here.
* Eventfs does support attribute changes via ->setattr(). But it relies
on simple_setattr() which is already idmapped mount aware. So there's
nothing for us to do.
* Ownership is inherited from the parent inode (tracefs) or optionally
from stashed ownership information (eventfs). That means the idmapping
is irrelevant here. It's similar to the "inherit gid from parent
directory" logic we have in some circumstances. TL;DR nothing to do
here as well.
* 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 through to most helpers.
There's some subtlety in eventfs. Afaict, the directories and files for
the individual events are created on-demand during lookup or readdir.
The ownership of these events is again inherited from the parent inode
or recovered from stored state. In both cases the actual idmapping is
irrelevant.
The callchain here is:
eventfs_root_lookup("xfs", "events")
-> create_{dir,file}_dentry("xfs", "events")
-> create_{dir,file}("xfs", "events")
-> eventfs_start_creating("xfs", "events")
-> lookup_one_len("xfs", "events")
And the subtlety is that lookup_one_len() does permission checking on
the parent inode (IOW, if you want a dentry for "blech" under "events"
it'll do a permission check on events->d_inode) for exec permissions
and then goes on to give you a new dentry.
Usually this call would have to be changed to lookup_one() and the
idmapping be handed down to it. But I think that's irrelevant here.
Lookup generally doesn't need to be aware of idmappings at all. The
permission checking is done purely in the vfs via may_lookup() and the
idmapping is irrelevant because we always initialize inodes with the
filesystem level ownership (see the idmappings.rst) documentation if
you're interested in excessive details (otherwise you get inode aliases
which you really don't want).
For tracefs it would not matter for lookup per se but only because
tracefs seemingly creates inodes/dentries during lookup (and readdir()).
But imho the permission checking done in current eventfs_root_lookup()
via lookup_one_len() is meaningless in any way; possibly even
(conceptually) wrong.
Because, the actual permission checking for the creation of the eventfs
entries isn't really done during lookup or readdir, it's done when mkdir
is called:
mkdir /sys/kernel/tracing/instances/foo
Here, all possible entries beneath foo including "events" and further
below are recorded and stored. So once mkdir returns it basically means
that it succeeded with the creation of all the necessary directories and
files. For all purposes the foo/events/ directory and below have all the
entries that matter. They have been created. It's comparable to them not
being in the {d,i}cache, I guess.
When one goes and looksup stuff under foo/events/ or readdir the entries
in that directory:
fd = open("foo/events")
readdir(fd, ...)
then they are licensed to list an entry in that directory. So all that
needs to be done is to actually list those files in that directory. And
since they already exist (they were created during mkdir) we just need
to splice in inodes and dentries for them. But for that we shouldn't
check permissions on the directory again. Because we've done that
already correctly when the VFS called may_lookup().
IOW, the inode_permission() in lookup_one_len() that eventfs does is
redundant and just wrong.
Luckily, I don't think we need to even change anything because all
directories that eventfs creates always grant exec permissions to the
other group so lookup_one_len() will trivially succeed. IIUC.
Drafted-by-with-no-guarantees-whatsoever-that-this-wont-burn-the-house-down: Christian Brauner <[email protected]>
---
fs/tracefs/event_inode.c | 8 +-
fs/tracefs/inode.c | 38 +++--
fs/tracefs/internal.h | 3 +-
include/linux/tracefs.h | 20 +--
kernel/trace/ftrace.c | 43 +++---
kernel/trace/trace.c | 201 ++++++++++++++-------------
kernel/trace/trace.h | 22 +--
kernel/trace/trace_dynevent.c | 2 +-
kernel/trace/trace_events.c | 27 ++--
kernel/trace/trace_events_synth.c | 5 +-
kernel/trace/trace_functions.c | 6 +-
kernel/trace/trace_functions_graph.c | 4 +-
kernel/trace/trace_hwlat.c | 8 +-
kernel/trace/trace_kprobe.c | 4 +-
kernel/trace/trace_osnoise.c | 48 ++++---
kernel/trace/trace_printk.c | 4 +-
kernel/trace/trace_stack.c | 11 +-
kernel/trace/trace_stat.c | 6 +-
kernel/trace/trace_uprobe.c | 4 +-
19 files changed, 251 insertions(+), 213 deletions(-)
diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 2ccc849a5bda..e2f352bd8779 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -852,11 +852,11 @@ struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode
*
* See eventfs_create_dir() for use of @entries.
*/
-struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry *parent,
- const struct eventfs_entry *entries,
- int size, void *data)
+struct eventfs_inode *eventfs_create_events_dir(
+ struct mnt_idmap *idmap, const char *name, struct dentry *parent,
+ const struct eventfs_entry *entries, int size, void *data)
{
- struct dentry *dentry = tracefs_start_creating(name, parent);
+ struct dentry *dentry = tracefs_start_creating(idmap, name, parent);
struct eventfs_inode *ei;
struct tracefs_inode *ti;
struct inode *inode;
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index ae648deed019..f4f4904eb3a0 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -68,7 +68,7 @@ static const struct file_operations tracefs_file_operations = {
};
static struct tracefs_dir_ops {
- int (*mkdir)(const char *name);
+ int (*mkdir)(struct mnt_idmap *idmap, const char *name);
int (*rmdir)(const char *name);
} tracefs_ops __ro_after_init;
@@ -104,7 +104,7 @@ static int tracefs_syscall_mkdir(struct mnt_idmap *idmap,
* mkdir routine to handle races.
*/
inode_unlock(inode);
- ret = tracefs_ops.mkdir(name);
+ ret = tracefs_ops.mkdir(idmap, name);
inode_lock(inode);
kfree(name);
@@ -439,10 +439,12 @@ static struct file_system_type trace_fs_type = {
.name = "tracefs",
.mount = trace_mount,
.kill_sb = kill_litter_super,
+ .fs_flags = FS_ALLOW_IDMAP,
};
MODULE_ALIAS_FS("tracefs");
-struct dentry *tracefs_start_creating(const char *name, struct dentry *parent)
+struct dentry *tracefs_start_creating(struct mnt_idmap *idmap, const char *name,
+ struct dentry *parent)
{
struct dentry *dentry;
int error;
@@ -466,7 +468,7 @@ struct dentry *tracefs_start_creating(const char *name, struct dentry *parent)
if (unlikely(IS_DEADDIR(d_inode(parent))))
dentry = ERR_PTR(-ENOENT);
else
- dentry = lookup_one_len(name, parent, strlen(name));
+ dentry = lookup_one(idmap, name, parent, strlen(name));
if (!IS_ERR(dentry) && d_inode(dentry)) {
dput(dentry);
dentry = ERR_PTR(-EEXIST);
@@ -589,8 +591,9 @@ struct dentry *eventfs_end_creating(struct dentry *dentry)
* If tracefs is not enabled in the kernel, the value -%ENODEV will be
* returned.
*/
-struct dentry *tracefs_create_file(const char *name, umode_t mode,
- struct dentry *parent, void *data,
+struct dentry *tracefs_create_file(struct mnt_idmap *idmap, const char *name,
+ umode_t mode, struct dentry *parent,
+ void *data,
const struct file_operations *fops)
{
struct dentry *dentry;
@@ -602,7 +605,7 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
if (!(mode & S_IFMT))
mode |= S_IFREG;
BUG_ON(!S_ISREG(mode));
- dentry = tracefs_start_creating(name, parent);
+ dentry = tracefs_start_creating(idmap, name, parent);
if (IS_ERR(dentry))
return NULL;
@@ -621,10 +624,11 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
return tracefs_end_creating(dentry);
}
-static struct dentry *__create_dir(const char *name, struct dentry *parent,
+static struct dentry *__create_dir(struct mnt_idmap *idmap, const char *name,
+ struct dentry *parent,
const struct inode_operations *ops)
{
- struct dentry *dentry = tracefs_start_creating(name, parent);
+ struct dentry *dentry = tracefs_start_creating(idmap, name, parent);
struct inode *inode;
if (IS_ERR(dentry))
@@ -649,6 +653,10 @@ static struct dentry *__create_dir(const char *name, struct dentry *parent,
return tracefs_end_creating(dentry);
}
+const struct inode_operations tracefs_default_dir_inode_operations = {
+ .lookup = simple_lookup,
+};
+
/**
* tracefs_create_dir - create a directory in the tracefs filesystem
* @name: a pointer to a string containing the name of the directory to
@@ -666,12 +674,14 @@ static struct dentry *__create_dir(const char *name, struct dentry *parent,
* If tracing is not enabled in the kernel, the value -%ENODEV will be
* returned.
*/
-struct dentry *tracefs_create_dir(const char *name, struct dentry *parent)
+struct dentry *tracefs_create_dir(struct mnt_idmap *idmap, const char *name,
+ struct dentry *parent)
{
if (security_locked_down(LOCKDOWN_TRACEFS))
return NULL;
- return __create_dir(name, parent, &simple_dir_inode_operations);
+ return __create_dir(idmap, name, parent,
+ &tracefs_default_dir_inode_operations);
}
/**
@@ -693,7 +703,8 @@ struct dentry *tracefs_create_dir(const char *name, struct dentry *parent)
*/
__init struct dentry *tracefs_create_instance_dir(const char *name,
struct dentry *parent,
- int (*mkdir)(const char *name),
+ int (*mkdir)(struct mnt_idmap *idmap,
+ const char *name),
int (*rmdir)(const char *name))
{
struct dentry *dentry;
@@ -702,7 +713,8 @@ __init struct dentry *tracefs_create_instance_dir(const char *name,
if (WARN_ON(tracefs_ops.mkdir || tracefs_ops.rmdir))
return NULL;
- dentry = __create_dir(name, parent, &tracefs_dir_inode_operations);
+ dentry = __create_dir(&nop_mnt_idmap, name, parent,
+ &tracefs_dir_inode_operations);
if (!dentry)
return NULL;
diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
index ccee18ca66c7..64abc5fcc62f 100644
--- a/fs/tracefs/internal.h
+++ b/fs/tracefs/internal.h
@@ -70,8 +70,9 @@ static inline struct tracefs_inode *get_tracefs(const struct inode *inode)
return container_of(inode, struct tracefs_inode, vfs_inode);
}
-struct dentry *tracefs_start_creating(const char *name, struct dentry *parent);
struct dentry *tracefs_end_creating(struct dentry *dentry);
+struct dentry *tracefs_start_creating(struct mnt_idmap *idmap, const char *name,
+ struct dentry *parent);
struct dentry *tracefs_failed_creating(struct dentry *dentry);
struct inode *tracefs_get_inode(struct super_block *sb);
struct dentry *eventfs_start_creating(const char *name, struct dentry *parent);
diff --git a/include/linux/tracefs.h b/include/linux/tracefs.h
index 7a5fe17b6bf9..0457afbafc94 100644
--- a/include/linux/tracefs.h
+++ b/include/linux/tracefs.h
@@ -76,9 +76,9 @@ struct eventfs_entry {
struct eventfs_inode;
-struct eventfs_inode *eventfs_create_events_dir(const char *name, struct dentry *parent,
- const struct eventfs_entry *entries,
- int size, void *data);
+struct eventfs_inode *eventfs_create_events_dir(
+ struct mnt_idmap *idmap, const char *name, struct dentry *parent,
+ const struct eventfs_entry *entries, int size, void *data);
struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode *parent,
const struct eventfs_entry *entries,
@@ -87,16 +87,20 @@ struct eventfs_inode *eventfs_create_dir(const char *name, struct eventfs_inode
void eventfs_remove_events_dir(struct eventfs_inode *ei);
void eventfs_remove_dir(struct eventfs_inode *ei);
-struct dentry *tracefs_create_file(const char *name, umode_t mode,
- struct dentry *parent, void *data,
+struct dentry *tracefs_create_file(struct mnt_idmap *idmap, const char *name,
+ umode_t mode, struct dentry *parent,
+ void *data,
const struct file_operations *fops);
-struct dentry *tracefs_create_dir(const char *name, struct dentry *parent);
+struct dentry *tracefs_create_dir(struct mnt_idmap *idmap, const char *name,
+ struct dentry *parent);
void tracefs_remove(struct dentry *dentry);
-struct dentry *tracefs_create_instance_dir(const char *name, struct dentry *parent,
- int (*mkdir)(const char *name),
+struct dentry *tracefs_create_instance_dir(const char *name,
+ struct dentry *parent,
+ int (*mkdir)(struct mnt_idmap *idmap,
+ const char *name),
int (*rmdir)(const char *name));
bool tracefs_initialized(void);
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 8de8bec5f366..6bf9dc7c5c29 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1015,7 +1015,7 @@ static __init void ftrace_profile_tracefs(struct dentry *d_tracer)
}
}
- trace_create_file("function_profile_enabled",
+ trace_create_file(&nop_mnt_idmap, "function_profile_enabled",
TRACE_MODE_WRITE, d_tracer, NULL,
&ftrace_profile_fops);
}
@@ -6380,14 +6380,14 @@ static const struct file_operations ftrace_graph_notrace_fops = {
};
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-void ftrace_create_filter_files(struct ftrace_ops *ops,
+void ftrace_create_filter_files(struct mnt_idmap *idmap, struct ftrace_ops *ops,
struct dentry *parent)
{
- trace_create_file("set_ftrace_filter", TRACE_MODE_WRITE, parent,
+ trace_create_file(idmap, "set_ftrace_filter", TRACE_MODE_WRITE, parent,
ops, &ftrace_filter_fops);
- trace_create_file("set_ftrace_notrace", TRACE_MODE_WRITE, parent,
+ trace_create_file(idmap, "set_ftrace_notrace", TRACE_MODE_WRITE, parent,
ops, &ftrace_notrace_fops);
}
@@ -6413,28 +6413,26 @@ void ftrace_destroy_filter_files(struct ftrace_ops *ops)
static __init int ftrace_init_dyn_tracefs(struct dentry *d_tracer)
{
+ trace_create_file(&nop_mnt_idmap, "available_filter_functions",
+ TRACE_MODE_READ, d_tracer, NULL, &ftrace_avail_fops);
- trace_create_file("available_filter_functions", TRACE_MODE_READ,
- d_tracer, NULL, &ftrace_avail_fops);
+ trace_create_file(&nop_mnt_idmap, "available_filter_functions_addrs",
+ TRACE_MODE_READ, d_tracer, NULL,
+ &ftrace_avail_addrs_fops);
- trace_create_file("available_filter_functions_addrs", TRACE_MODE_READ,
- d_tracer, NULL, &ftrace_avail_addrs_fops);
-
- trace_create_file("enabled_functions", TRACE_MODE_READ,
+ trace_create_file(&nop_mnt_idmap, "enabled_functions", TRACE_MODE_READ,
d_tracer, NULL, &ftrace_enabled_fops);
- trace_create_file("touched_functions", TRACE_MODE_READ,
- d_tracer, NULL, &ftrace_touched_fops);
+ trace_create_file(&nop_mnt_idmap, "touched_functions", TRACE_MODE_READ,
+ d_tracer, NULL, &ftrace_touched_fops);
- ftrace_create_filter_files(&global_ops, d_tracer);
+ ftrace_create_filter_files(&nop_mnt_idmap, &global_ops, d_tracer);
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
- trace_create_file("set_graph_function", TRACE_MODE_WRITE, d_tracer,
- NULL,
- &ftrace_graph_fops);
- trace_create_file("set_graph_notrace", TRACE_MODE_WRITE, d_tracer,
- NULL,
- &ftrace_graph_notrace_fops);
+ trace_create_file(&nop_mnt_idmap, "set_graph_function",
+ TRACE_MODE_WRITE, d_tracer, NULL, &ftrace_graph_fops);
+ trace_create_file(&nop_mnt_idmap, "set_graph_notrace", TRACE_MODE_WRITE,
+ d_tracer, NULL, &ftrace_graph_notrace_fops);
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
return 0;
@@ -7858,11 +7856,12 @@ static const struct file_operations ftrace_no_pid_fops = {
.release = ftrace_pid_release,
};
-void ftrace_init_tracefs(struct trace_array *tr, struct dentry *d_tracer)
+void ftrace_init_tracefs(struct mnt_idmap *idmap, struct trace_array *tr,
+ struct dentry *d_tracer)
{
- trace_create_file("set_ftrace_pid", TRACE_MODE_WRITE, d_tracer,
+ trace_create_file(idmap, "set_ftrace_pid", TRACE_MODE_WRITE, d_tracer,
tr, &ftrace_pid_fops);
- trace_create_file("set_ftrace_notrace_pid", TRACE_MODE_WRITE,
+ trace_create_file(idmap, "set_ftrace_notrace_pid", TRACE_MODE_WRITE,
d_tracer, tr, &ftrace_no_pid_fops);
}
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 199df497db07..c0bd4b0dfe85 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1766,12 +1766,13 @@ static void latency_fsnotify_workfn_irq(struct irq_work *iwork)
queue_work(fsnotify_wq, &tr->fsnotify_work);
}
-static void trace_create_maxlat_file(struct trace_array *tr,
+static void trace_create_maxlat_file(struct mnt_idmap *idmap,
+ struct trace_array *tr,
struct dentry *d_tracer)
{
INIT_WORK(&tr->fsnotify_work, latency_fsnotify_workfn);
init_irq_work(&tr->fsnotify_irqwork, latency_fsnotify_workfn_irq);
- tr->d_max_latency = trace_create_file("tracing_max_latency",
+ tr->d_max_latency = trace_create_file(idmap, "tracing_max_latency",
TRACE_MODE_WRITE,
d_tracer, tr,
&tracing_max_lat_fops);
@@ -1804,8 +1805,8 @@ void latency_fsnotify(struct trace_array *tr)
#else /* !LATENCY_FS_NOTIFY */
-#define trace_create_maxlat_file(tr, d_tracer) \
- trace_create_file("tracing_max_latency", TRACE_MODE_WRITE, \
+#define trace_create_maxlat_file(idmap, tr, d_tracer) \
+ trace_create_file(idmap, "tracing_max_latency", TRACE_MODE_WRITE, \
d_tracer, tr, &tracing_max_lat_fops)
#endif
@@ -2124,7 +2125,8 @@ static inline int do_run_tracer_selftest(struct tracer *type)
}
#endif /* CONFIG_FTRACE_STARTUP_TEST */
-static void add_tracer_options(struct trace_array *tr, struct tracer *t);
+static void add_tracer_options(struct mnt_idmap *idmap, struct trace_array *tr,
+ struct tracer *t);
static void __init apply_trace_boot_options(void);
@@ -2191,7 +2193,7 @@ int __init register_tracer(struct tracer *type)
type->next = trace_types;
trace_types = type;
- add_tracer_options(&global_trace, type);
+ add_tracer_options(&nop_mnt_idmap, &global_trace, type);
out:
mutex_unlock(&trace_types_lock);
@@ -6245,14 +6247,14 @@ trace_insert_eval_map_file(struct module *mod, struct trace_eval_map **start,
mutex_unlock(&trace_eval_mutex);
}
-static void trace_create_eval_file(struct dentry *d_tracer)
+static void trace_create_eval_file(struct mnt_idmap *idmap, struct dentry *d_tracer)
{
- trace_create_file("eval_map", TRACE_MODE_READ, d_tracer,
+ trace_create_file(idmap, "eval_map", TRACE_MODE_READ, d_tracer,
NULL, &tracing_eval_map_fops);
}
#else /* CONFIG_TRACE_EVAL_MAP_FILE */
-static inline void trace_create_eval_file(struct dentry *d_tracer) { }
+static inline void trace_create_eval_file(struct mnt_idmap *idmap, struct dentry *d_tracer) { }
static inline void trace_insert_eval_map_file(struct module *mod,
struct trace_eval_map **start, int len) { }
#endif /* !CONFIG_TRACE_EVAL_MAP_FILE */
@@ -6454,7 +6456,7 @@ int tracing_update_buffers(struct trace_array *tr)
struct trace_option_dentry;
static void
-create_trace_option_files(struct trace_array *tr, struct tracer *tracer);
+create_trace_option_files(struct mnt_idmap *idmap, struct trace_array *tr, struct tracer *tracer);
/*
* Used to clear out the tracer before deletion of an instance.
@@ -6475,7 +6477,8 @@ static void tracing_set_nop(struct trace_array *tr)
static bool tracer_options_updated;
-static void add_tracer_options(struct trace_array *tr, struct tracer *t)
+static void add_tracer_options(struct mnt_idmap *idmap, struct trace_array *tr,
+ struct tracer *t)
{
/* Only enable if the directory has been created already. */
if (!tr->dir)
@@ -6485,7 +6488,7 @@ static void add_tracer_options(struct trace_array *tr, struct tracer *t)
if (!tracer_options_updated)
return;
- create_trace_option_files(tr, t);
+ create_trace_option_files(idmap, tr, t);
}
int tracing_set_tracer(struct trace_array *tr, const char *buf)
@@ -8845,7 +8848,8 @@ static struct dentry *tracing_get_dentry(struct trace_array *tr)
return tr->dir;
}
-static struct dentry *tracing_dentry_percpu(struct trace_array *tr, int cpu)
+static struct dentry *tracing_dentry_percpu(struct mnt_idmap *idmap,
+ struct trace_array *tr, int cpu)
{
struct dentry *d_tracer;
@@ -8856,7 +8860,7 @@ static struct dentry *tracing_dentry_percpu(struct trace_array *tr, int cpu)
if (IS_ERR(d_tracer))
return NULL;
- tr->percpu_dir = tracefs_create_dir("per_cpu", d_tracer);
+ tr->percpu_dir = tracefs_create_dir(idmap, "per_cpu", d_tracer);
MEM_FAIL(!tr->percpu_dir,
"Could not create tracefs directory 'per_cpu/%d'\n", cpu);
@@ -8864,11 +8868,13 @@ static struct dentry *tracing_dentry_percpu(struct trace_array *tr, int cpu)
return tr->percpu_dir;
}
-static struct dentry *
-trace_create_cpu_file(const char *name, umode_t mode, struct dentry *parent,
- void *data, long cpu, const struct file_operations *fops)
+static struct dentry *trace_create_cpu_file(struct mnt_idmap *idmap,
+ const char *name, umode_t mode,
+ struct dentry *parent, void *data,
+ long cpu,
+ const struct file_operations *fops)
{
- struct dentry *ret = trace_create_file(name, mode, parent, data, fops);
+ struct dentry *ret = trace_create_file(idmap, name, mode, parent, data, fops);
if (ret) /* See tracing_get_cpu() */
d_inode(ret)->i_cdev = (void *)(cpu + 1);
@@ -8876,9 +8882,9 @@ trace_create_cpu_file(const char *name, umode_t mode, struct dentry *parent,
}
static void
-tracing_init_tracefs_percpu(struct trace_array *tr, long cpu)
+tracing_init_tracefs_percpu(struct mnt_idmap *idmap, struct trace_array *tr, long cpu)
{
- struct dentry *d_percpu = tracing_dentry_percpu(tr, cpu);
+ struct dentry *d_percpu = tracing_dentry_percpu(idmap, tr, cpu);
struct dentry *d_cpu;
char cpu_dir[30]; /* 30 characters should be more than enough */
@@ -8886,34 +8892,34 @@ tracing_init_tracefs_percpu(struct trace_array *tr, long cpu)
return;
snprintf(cpu_dir, 30, "cpu%ld", cpu);
- d_cpu = tracefs_create_dir(cpu_dir, d_percpu);
+ d_cpu = tracefs_create_dir(idmap, cpu_dir, d_percpu);
if (!d_cpu) {
pr_warn("Could not create tracefs '%s' entry\n", cpu_dir);
return;
}
/* per cpu trace_pipe */
- trace_create_cpu_file("trace_pipe", TRACE_MODE_READ, d_cpu,
+ trace_create_cpu_file(idmap, "trace_pipe", TRACE_MODE_READ, d_cpu,
tr, cpu, &tracing_pipe_fops);
/* per cpu trace */
- trace_create_cpu_file("trace", TRACE_MODE_WRITE, d_cpu,
+ trace_create_cpu_file(idmap, "trace", TRACE_MODE_WRITE, d_cpu,
tr, cpu, &tracing_fops);
- trace_create_cpu_file("trace_pipe_raw", TRACE_MODE_READ, d_cpu,
+ trace_create_cpu_file(idmap, "trace_pipe_raw", TRACE_MODE_READ, d_cpu,
tr, cpu, &tracing_buffers_fops);
- trace_create_cpu_file("stats", TRACE_MODE_READ, d_cpu,
+ trace_create_cpu_file(idmap, "stats", TRACE_MODE_READ, d_cpu,
tr, cpu, &tracing_stats_fops);
- trace_create_cpu_file("buffer_size_kb", TRACE_MODE_READ, d_cpu,
+ trace_create_cpu_file(idmap, "buffer_size_kb", TRACE_MODE_READ, d_cpu,
tr, cpu, &tracing_entries_fops);
#ifdef CONFIG_TRACER_SNAPSHOT
- trace_create_cpu_file("snapshot", TRACE_MODE_WRITE, d_cpu,
+ trace_create_cpu_file(idmap, "snapshot", TRACE_MODE_WRITE, d_cpu,
tr, cpu, &snapshot_fops);
- trace_create_cpu_file("snapshot_raw", TRACE_MODE_READ, d_cpu,
+ trace_create_cpu_file(idmap, "snapshot_raw", TRACE_MODE_READ, d_cpu,
tr, cpu, &snapshot_raw_fops);
#endif
}
@@ -9088,23 +9094,21 @@ static const struct file_operations trace_options_core_fops = {
.llseek = generic_file_llseek,
};
-struct dentry *trace_create_file(const char *name,
- umode_t mode,
- struct dentry *parent,
- void *data,
- const struct file_operations *fops)
+struct dentry *trace_create_file(struct mnt_idmap *idmap, const char *name,
+ umode_t mode, struct dentry *parent,
+ void *data, const struct file_operations *fops)
{
struct dentry *ret;
- ret = tracefs_create_file(name, mode, parent, data, fops);
+ ret = tracefs_create_file(idmap, name, mode, parent, data, fops);
if (!ret)
pr_warn("Could not create tracefs '%s' entry\n", name);
return ret;
}
-
-static struct dentry *trace_options_init_dentry(struct trace_array *tr)
+static struct dentry *trace_options_init_dentry(struct mnt_idmap *idmap,
+ struct trace_array *tr)
{
struct dentry *d_tracer;
@@ -9115,7 +9119,7 @@ static struct dentry *trace_options_init_dentry(struct trace_array *tr)
if (IS_ERR(d_tracer))
return NULL;
- tr->options = tracefs_create_dir("options", d_tracer);
+ tr->options = tracefs_create_dir(idmap, "options", d_tracer);
if (!tr->options) {
pr_warn("Could not create tracefs directory 'options'\n");
return NULL;
@@ -9125,14 +9129,14 @@ static struct dentry *trace_options_init_dentry(struct trace_array *tr)
}
static void
-create_trace_option_file(struct trace_array *tr,
+create_trace_option_file(struct mnt_idmap *idmap, struct trace_array *tr,
struct trace_option_dentry *topt,
struct tracer_flags *flags,
struct tracer_opt *opt)
{
struct dentry *t_options;
- t_options = trace_options_init_dentry(tr);
+ t_options = trace_options_init_dentry(idmap, tr);
if (!t_options)
return;
@@ -9140,13 +9144,14 @@ create_trace_option_file(struct trace_array *tr,
topt->opt = opt;
topt->tr = tr;
- topt->entry = trace_create_file(opt->name, TRACE_MODE_WRITE,
+ topt->entry = trace_create_file(idmap, opt->name, TRACE_MODE_WRITE,
t_options, topt, &trace_options_fops);
}
-static void
-create_trace_option_files(struct trace_array *tr, struct tracer *tracer)
+static void create_trace_option_files(struct mnt_idmap *idmap,
+ struct trace_array *tr,
+ struct tracer *tracer)
{
struct trace_option_dentry *topts;
struct trace_options *tr_topts;
@@ -9198,7 +9203,7 @@ create_trace_option_files(struct trace_array *tr, struct tracer *tracer)
tr->nr_topts++;
for (cnt = 0; opts[cnt].name; cnt++) {
- create_trace_option_file(tr, &topts[cnt], flags,
+ create_trace_option_file(idmap, tr, &topts[cnt], flags,
&opts[cnt]);
MEM_FAIL(topts[cnt].entry == NULL,
"Failed to create trace option: %s",
@@ -9207,34 +9212,34 @@ create_trace_option_files(struct trace_array *tr, struct tracer *tracer)
}
static struct dentry *
-create_trace_option_core_file(struct trace_array *tr,
+create_trace_option_core_file(struct mnt_idmap *idmap, struct trace_array *tr,
const char *option, long index)
{
struct dentry *t_options;
- t_options = trace_options_init_dentry(tr);
+ t_options = trace_options_init_dentry(idmap, tr);
if (!t_options)
return NULL;
- return trace_create_file(option, TRACE_MODE_WRITE, t_options,
+ return trace_create_file(idmap, option, TRACE_MODE_WRITE, t_options,
(void *)&tr->trace_flags_index[index],
&trace_options_core_fops);
}
-static void create_trace_options_dir(struct trace_array *tr)
+static void create_trace_options_dir(struct mnt_idmap *idmap, struct trace_array *tr)
{
struct dentry *t_options;
bool top_level = tr == &global_trace;
int i;
- t_options = trace_options_init_dentry(tr);
+ t_options = trace_options_init_dentry(idmap, tr);
if (!t_options)
return;
for (i = 0; trace_options[i]; i++) {
if (top_level ||
!((1 << i) & TOP_LEVEL_TRACE_FLAGS))
- create_trace_option_core_file(tr, trace_options[i], i);
+ create_trace_option_core_file(idmap, tr, trace_options[i], i);
}
}
@@ -9342,8 +9347,8 @@ static const struct file_operations buffer_percent_fops = {
static struct dentry *trace_instance_dir;
-static void
-init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer);
+static void init_tracer_tracefs(struct mnt_idmap *idmap, struct trace_array *tr,
+ struct dentry *d_tracer);
static int
allocate_trace_buffer(struct trace_array *tr, struct array_buffer *buf, int size)
@@ -9426,19 +9431,20 @@ static void init_trace_flags_index(struct trace_array *tr)
tr->trace_flags_index[i] = i;
}
-static void __update_tracer_options(struct trace_array *tr)
+static void __update_tracer_options(struct mnt_idmap *idmap,
+ struct trace_array *tr)
{
struct tracer *t;
for (t = trace_types; t; t = t->next)
- add_tracer_options(tr, t);
+ add_tracer_options(idmap, tr, t);
}
static void update_tracer_options(struct trace_array *tr)
{
mutex_lock(&trace_types_lock);
tracer_options_updated = true;
- __update_tracer_options(tr);
+ __update_tracer_options(&nop_mnt_idmap, tr);
mutex_unlock(&trace_types_lock);
}
@@ -9470,27 +9476,28 @@ struct trace_array *trace_array_find_get(const char *instance)
return tr;
}
-static int trace_array_create_dir(struct trace_array *tr)
+static int trace_array_create_dir(struct mnt_idmap *idmap, struct trace_array *tr)
{
int ret;
- tr->dir = tracefs_create_dir(tr->name, trace_instance_dir);
+ tr->dir = tracefs_create_dir(idmap, tr->name, trace_instance_dir);
if (!tr->dir)
return -EINVAL;
- ret = event_trace_add_tracer(tr->dir, tr);
+ ret = event_trace_add_tracer(idmap, tr->dir, tr);
if (ret) {
tracefs_remove(tr->dir);
return ret;
}
- init_tracer_tracefs(tr, tr->dir);
- __update_tracer_options(tr);
+ init_tracer_tracefs(idmap, tr, tr->dir);
+ __update_tracer_options(idmap, tr);
return ret;
}
-static struct trace_array *trace_array_create(const char *name)
+static struct trace_array *trace_array_create(struct mnt_idmap *idmap,
+ const char *name)
{
struct trace_array *tr;
int ret;
@@ -9539,7 +9546,7 @@ static struct trace_array *trace_array_create(const char *name)
init_trace_flags_index(tr);
if (trace_instance_dir) {
- ret = trace_array_create_dir(tr);
+ ret = trace_array_create_dir(idmap, tr);
if (ret)
goto out_free_tr;
} else
@@ -9562,7 +9569,7 @@ static struct trace_array *trace_array_create(const char *name)
return ERR_PTR(ret);
}
-static int instance_mkdir(const char *name)
+static int instance_mkdir(struct mnt_idmap *idmap, const char *name)
{
struct trace_array *tr;
int ret;
@@ -9574,7 +9581,7 @@ static int instance_mkdir(const char *name)
if (trace_array_find(name))
goto out_unlock;
- tr = trace_array_create(name);
+ tr = trace_array_create(idmap, name);
ret = PTR_ERR_OR_ZERO(tr);
@@ -9612,7 +9619,7 @@ struct trace_array *trace_array_get_by_name(const char *name)
goto out_unlock;
}
- tr = trace_array_create(name);
+ tr = trace_array_create(&nop_mnt_idmap, name);
if (IS_ERR(tr))
tr = NULL;
@@ -9728,7 +9735,7 @@ static __init void create_trace_instances(struct dentry *d_tracer)
list_for_each_entry(tr, &ftrace_trace_arrays, list) {
if (!tr->name)
continue;
- if (MEM_FAIL(trace_array_create_dir(tr) < 0,
+ if (MEM_FAIL(trace_array_create_dir(&nop_mnt_idmap, tr) < 0,
"Failed to create instance directory\n"))
break;
}
@@ -9737,81 +9744,81 @@ static __init void create_trace_instances(struct dentry *d_tracer)
mutex_unlock(&event_mutex);
}
-static void
-init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer)
+static void init_tracer_tracefs(struct mnt_idmap *idmap, struct trace_array *tr,
+ struct dentry *d_tracer)
{
int cpu;
- trace_create_file("available_tracers", TRACE_MODE_READ, d_tracer,
+ trace_create_file(idmap, "available_tracers", TRACE_MODE_READ, d_tracer,
tr, &show_traces_fops);
- trace_create_file("current_tracer", TRACE_MODE_WRITE, d_tracer,
+ trace_create_file(idmap, "current_tracer", TRACE_MODE_WRITE, d_tracer,
tr, &set_tracer_fops);
- trace_create_file("tracing_cpumask", TRACE_MODE_WRITE, d_tracer,
+ trace_create_file(idmap, "tracing_cpumask", TRACE_MODE_WRITE, d_tracer,
tr, &tracing_cpumask_fops);
- trace_create_file("trace_options", TRACE_MODE_WRITE, d_tracer,
+ trace_create_file(idmap, "trace_options", TRACE_MODE_WRITE, d_tracer,
tr, &tracing_iter_fops);
- trace_create_file("trace", TRACE_MODE_WRITE, d_tracer,
+ trace_create_file(idmap, "trace", TRACE_MODE_WRITE, d_tracer,
tr, &tracing_fops);
- trace_create_file("trace_pipe", TRACE_MODE_READ, d_tracer,
+ trace_create_file(idmap, "trace_pipe", TRACE_MODE_READ, d_tracer,
tr, &tracing_pipe_fops);
- trace_create_file("buffer_size_kb", TRACE_MODE_WRITE, d_tracer,
+ trace_create_file(idmap, "buffer_size_kb", TRACE_MODE_WRITE, d_tracer,
tr, &tracing_entries_fops);
- trace_create_file("buffer_total_size_kb", TRACE_MODE_READ, d_tracer,
+ trace_create_file(idmap, "buffer_total_size_kb", TRACE_MODE_READ, d_tracer,
tr, &tracing_total_entries_fops);
- trace_create_file("free_buffer", 0200, d_tracer,
+ trace_create_file(idmap, "free_buffer", 0200, d_tracer,
tr, &tracing_free_buffer_fops);
- trace_create_file("trace_marker", 0220, d_tracer,
+ trace_create_file(idmap, "trace_marker", 0220, d_tracer,
tr, &tracing_mark_fops);
tr->trace_marker_file = __find_event_file(tr, "ftrace", "print");
- trace_create_file("trace_marker_raw", 0220, d_tracer,
+ trace_create_file(idmap, "trace_marker_raw", 0220, d_tracer,
tr, &tracing_mark_raw_fops);
- trace_create_file("trace_clock", TRACE_MODE_WRITE, d_tracer, tr,
+ trace_create_file(idmap, "trace_clock", TRACE_MODE_WRITE, d_tracer, tr,
&trace_clock_fops);
- trace_create_file("tracing_on", TRACE_MODE_WRITE, d_tracer,
+ trace_create_file(idmap, "tracing_on", TRACE_MODE_WRITE, d_tracer,
tr, &rb_simple_fops);
- trace_create_file("timestamp_mode", TRACE_MODE_READ, d_tracer, tr,
+ trace_create_file(idmap, "timestamp_mode", TRACE_MODE_READ, d_tracer, tr,
&trace_time_stamp_mode_fops);
tr->buffer_percent = 50;
- trace_create_file("buffer_percent", TRACE_MODE_WRITE, d_tracer,
+ trace_create_file(idmap, "buffer_percent", TRACE_MODE_WRITE, d_tracer,
tr, &buffer_percent_fops);
- create_trace_options_dir(tr);
+ create_trace_options_dir(idmap, tr);
#ifdef CONFIG_TRACER_MAX_TRACE
- trace_create_maxlat_file(tr, d_tracer);
+ trace_create_maxlat_file(idmap, tr, d_tracer);
#endif
- if (ftrace_create_function_files(tr, d_tracer))
+ if (ftrace_create_function_files(idmap, tr, d_tracer))
MEM_FAIL(1, "Could not allocate function filter files");
#ifdef CONFIG_TRACER_SNAPSHOT
- trace_create_file("snapshot", TRACE_MODE_WRITE, d_tracer,
+ trace_create_file(idmap, "snapshot", TRACE_MODE_WRITE, d_tracer,
tr, &snapshot_fops);
#endif
- trace_create_file("error_log", TRACE_MODE_WRITE, d_tracer,
+ trace_create_file(idmap, "error_log", TRACE_MODE_WRITE, d_tracer,
tr, &tracing_err_log_fops);
for_each_tracing_cpu(cpu)
- tracing_init_tracefs_percpu(tr, cpu);
+ tracing_init_tracefs_percpu(idmap, tr, cpu);
- ftrace_init_tracefs(tr, d_tracer);
+ ftrace_init_tracefs(idmap, tr, d_tracer);
}
static struct vfsmount *trace_automount(struct dentry *mntpt, void *ingore)
@@ -9991,32 +9998,32 @@ static __init void tracer_init_tracefs_work_func(struct work_struct *work)
event_trace_init();
- init_tracer_tracefs(&global_trace, NULL);
+ init_tracer_tracefs(&nop_mnt_idmap, &global_trace, NULL);
ftrace_init_tracefs_toplevel(&global_trace, NULL);
- trace_create_file("tracing_thresh", TRACE_MODE_WRITE, NULL,
+ trace_create_file(&nop_mnt_idmap, "tracing_thresh", TRACE_MODE_WRITE, NULL,
&global_trace, &tracing_thresh_fops);
- trace_create_file("README", TRACE_MODE_READ, NULL,
+ trace_create_file(&nop_mnt_idmap, "README", TRACE_MODE_READ, NULL,
NULL, &tracing_readme_fops);
- trace_create_file("saved_cmdlines", TRACE_MODE_READ, NULL,
+ trace_create_file(&nop_mnt_idmap, "saved_cmdlines", TRACE_MODE_READ, NULL,
NULL, &tracing_saved_cmdlines_fops);
- trace_create_file("saved_cmdlines_size", TRACE_MODE_WRITE, NULL,
+ trace_create_file(&nop_mnt_idmap, "saved_cmdlines_size", TRACE_MODE_WRITE, NULL,
NULL, &tracing_saved_cmdlines_size_fops);
- trace_create_file("saved_tgids", TRACE_MODE_READ, NULL,
+ trace_create_file(&nop_mnt_idmap, "saved_tgids", TRACE_MODE_READ, NULL,
NULL, &tracing_saved_tgids_fops);
- trace_create_eval_file(NULL);
+ trace_create_eval_file(&nop_mnt_idmap, NULL);
#ifdef CONFIG_MODULES
register_module_notifier(&trace_module_nb);
#endif
#ifdef CONFIG_DYNAMIC_FTRACE
- trace_create_file("dyn_ftrace_total_info", TRACE_MODE_READ, NULL,
+ trace_create_file(&nop_mnt_idmap, "dyn_ftrace_total_info", TRACE_MODE_READ, NULL,
NULL, &tracing_dyn_info_fops);
#endif
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 0489e72c8169..b8de94e4387d 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -622,9 +622,8 @@ bool tracing_is_disabled(void);
bool tracer_tracing_is_on(struct trace_array *tr);
void tracer_tracing_on(struct trace_array *tr);
void tracer_tracing_off(struct trace_array *tr);
-struct dentry *trace_create_file(const char *name,
- umode_t mode,
- struct dentry *parent,
+struct dentry *trace_create_file(struct mnt_idmap *idmap, const char *name,
+ umode_t mode, struct dentry *parent,
void *data,
const struct file_operations *fops);
@@ -1025,7 +1024,7 @@ static inline int ftrace_trace_task(struct trace_array *tr)
FTRACE_PID_IGNORE;
}
extern int ftrace_is_dead(void);
-int ftrace_create_function_files(struct trace_array *tr,
+int ftrace_create_function_files(struct mnt_idmap *idmap, struct trace_array *tr,
struct dentry *parent);
void ftrace_destroy_function_files(struct trace_array *tr);
int ftrace_allocate_ftrace_ops(struct trace_array *tr);
@@ -1033,7 +1032,8 @@ void ftrace_free_ftrace_ops(struct trace_array *tr);
void ftrace_init_global_array_ops(struct trace_array *tr);
void ftrace_init_array_ops(struct trace_array *tr, ftrace_func_t func);
void ftrace_reset_array_ops(struct trace_array *tr);
-void ftrace_init_tracefs(struct trace_array *tr, struct dentry *d_tracer);
+void ftrace_init_tracefs(struct mnt_idmap *idmap, struct trace_array *tr,
+ struct dentry *d_tracer);
void ftrace_init_tracefs_toplevel(struct trace_array *tr,
struct dentry *d_tracer);
void ftrace_clear_pids(struct trace_array *tr);
@@ -1046,7 +1046,7 @@ static inline int ftrace_trace_task(struct trace_array *tr)
}
static inline int ftrace_is_dead(void) { return 0; }
static inline int
-ftrace_create_function_files(struct trace_array *tr,
+ftrace_create_function_files(struct mnt_idmap *idmap, struct trace_array *tr,
struct dentry *parent)
{
return 0;
@@ -1060,7 +1060,7 @@ static inline void ftrace_destroy_function_files(struct trace_array *tr) { }
static inline __init void
ftrace_init_global_array_ops(struct trace_array *tr) { }
static inline void ftrace_reset_array_ops(struct trace_array *tr) { }
-static inline void ftrace_init_tracefs(struct trace_array *tr, struct dentry *d) { }
+static inline void ftrace_init_tracefs(struct mnt_idmap *idmap, struct trace_array *tr, struct dentry *d) { }
static inline void ftrace_init_tracefs_toplevel(struct trace_array *tr, struct dentry *d) { }
static inline void ftrace_clear_pids(struct trace_array *tr) { }
static inline int init_function_trace(void) { return 0; }
@@ -1114,7 +1114,7 @@ extern void clear_ftrace_function_probes(struct trace_array *tr);
int register_ftrace_command(struct ftrace_func_command *cmd);
int unregister_ftrace_command(struct ftrace_func_command *cmd);
-void ftrace_create_filter_files(struct ftrace_ops *ops,
+void ftrace_create_filter_files(struct mnt_idmap *idmap, struct ftrace_ops *ops,
struct dentry *parent);
void ftrace_destroy_filter_files(struct ftrace_ops *ops);
@@ -1141,7 +1141,7 @@ static inline void clear_ftrace_function_probes(struct trace_array *tr)
* The ops parameter passed in is usually undefined.
* This must be a macro.
*/
-#define ftrace_create_filter_files(ops, parent) do { } while (0)
+#define ftrace_create_filter_files(idmap, ops, parent) do { } while (0)
#define ftrace_destroy_filter_files(ops) do { } while (0)
#endif /* CONFIG_FUNCTION_TRACER && CONFIG_DYNAMIC_FTRACE */
@@ -1541,7 +1541,9 @@ extern void trace_event_enable_tgid_record(bool enable);
extern int event_trace_init(void);
extern int init_events(void);
-extern int event_trace_add_tracer(struct dentry *parent, struct trace_array *tr);
+extern int event_trace_add_tracer(struct mnt_idmap *idmap,
+ struct dentry *parent,
+ struct trace_array *tr);
extern int event_trace_del_tracer(struct trace_array *tr);
extern void __trace_early_add_events(struct trace_array *tr);
diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c
index 4376887e0d8a..c0caf4631f82 100644
--- a/kernel/trace/trace_dynevent.c
+++ b/kernel/trace/trace_dynevent.c
@@ -263,7 +263,7 @@ static __init int init_dynamic_event(void)
if (ret)
return 0;
- trace_create_file("dynamic_events", TRACE_MODE_WRITE, NULL,
+ trace_create_file(&nop_mnt_idmap, "dynamic_events", TRACE_MODE_WRITE, NULL,
NULL, &dynamic_events_ops);
return 0;
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index f29e815ca5b2..b41e0f26cee4 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -3768,7 +3768,8 @@ static int events_callback(const char *name, umode_t *mode, void **data,
/* Expects to have event_mutex held when called */
static int
-create_event_toplevel_files(struct dentry *parent, struct trace_array *tr)
+create_event_toplevel_files(struct mnt_idmap *idmap, struct dentry *parent,
+ struct trace_array *tr)
{
struct eventfs_inode *e_events;
struct dentry *entry;
@@ -3788,15 +3789,15 @@ create_event_toplevel_files(struct dentry *parent, struct trace_array *tr)
},
};
- entry = trace_create_file("set_event", TRACE_MODE_WRITE, parent,
+ entry = trace_create_file(idmap, "set_event", TRACE_MODE_WRITE, parent,
tr, &ftrace_set_event_fops);
if (!entry)
return -ENOMEM;
nr_entries = ARRAY_SIZE(events_entries);
- e_events = eventfs_create_events_dir("events", parent, events_entries,
- nr_entries, tr);
+ e_events = eventfs_create_events_dir(idmap, "events", parent,
+ events_entries, nr_entries, tr);
if (IS_ERR(e_events)) {
pr_warn("Could not create tracefs 'events' directory\n");
return -ENOMEM;
@@ -3804,12 +3805,11 @@ create_event_toplevel_files(struct dentry *parent, struct trace_array *tr)
/* There are not as crucial, just warn if they are not created */
- trace_create_file("set_event_pid", TRACE_MODE_WRITE, parent,
- tr, &ftrace_set_event_pid_fops);
+ trace_create_file(idmap, "set_event_pid", TRACE_MODE_WRITE, parent, tr,
+ &ftrace_set_event_pid_fops);
- trace_create_file("set_event_notrace_pid",
- TRACE_MODE_WRITE, parent, tr,
- &ftrace_set_event_notrace_pid_fops);
+ trace_create_file(idmap, "set_event_notrace_pid", TRACE_MODE_WRITE,
+ parent, tr, &ftrace_set_event_notrace_pid_fops);
tr->event_dir = e_events;
@@ -3829,13 +3829,14 @@ create_event_toplevel_files(struct dentry *parent, struct trace_array *tr)
*
* Must be called with event_mutex held.
*/
-int event_trace_add_tracer(struct dentry *parent, struct trace_array *tr)
+int event_trace_add_tracer(struct mnt_idmap *idmap, struct dentry *parent,
+ struct trace_array *tr)
{
int ret;
lockdep_assert_held(&event_mutex);
- ret = create_event_toplevel_files(parent, tr);
+ ret = create_event_toplevel_files(idmap, parent, tr);
if (ret)
goto out;
@@ -3862,7 +3863,7 @@ early_event_add_tracer(struct dentry *parent, struct trace_array *tr)
mutex_lock(&event_mutex);
- ret = create_event_toplevel_files(parent, tr);
+ ret = create_event_toplevel_files(&nop_mnt_idmap, parent, tr);
if (ret)
goto out_unlock;
@@ -4021,7 +4022,7 @@ __init int event_trace_init(void)
if (!tr)
return -ENODEV;
- trace_create_file("available_events", TRACE_MODE_READ,
+ trace_create_file(&nop_mnt_idmap, "available_events", TRACE_MODE_READ,
NULL, tr, &ftrace_avail_fops);
ret = early_event_add_tracer(NULL, tr);
diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
index e7af286af4f1..f8fbb0f4ef87 100644
--- a/kernel/trace/trace_events_synth.c
+++ b/kernel/trace/trace_events_synth.c
@@ -2318,8 +2318,9 @@ static __init int trace_events_synth_init(void)
if (err)
goto err;
- entry = tracefs_create_file("synthetic_events", TRACE_MODE_WRITE,
- NULL, NULL, &synth_events_fops);
+ entry = tracefs_create_file(&nop_mnt_idmap, "synthetic_events",
+ TRACE_MODE_WRITE, NULL, NULL,
+ &synth_events_fops);
if (!entry) {
err = -ENODEV;
goto err;
diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index 9f1bfbe105e8..266428151b60 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -77,8 +77,8 @@ void ftrace_free_ftrace_ops(struct trace_array *tr)
tr->ops = NULL;
}
-int ftrace_create_function_files(struct trace_array *tr,
- struct dentry *parent)
+int ftrace_create_function_files(struct mnt_idmap *idmap,
+ struct trace_array *tr, struct dentry *parent)
{
/*
* The top level array uses the "global_ops", and the files are
@@ -90,7 +90,7 @@ int ftrace_create_function_files(struct trace_array *tr,
if (!tr->ops)
return -EINVAL;
- ftrace_create_filter_files(tr->ops, parent);
+ ftrace_create_filter_files(idmap, tr->ops, parent);
return 0;
}
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index c35fbaab2a47..e7b4fa238eaf 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -1413,8 +1413,8 @@ static __init int init_graph_tracefs(void)
if (ret)
return 0;
- trace_create_file("max_graph_depth", TRACE_MODE_WRITE, NULL,
- NULL, &graph_depth_fops);
+ trace_create_file(&nop_mnt_idmap, "max_graph_depth", TRACE_MODE_WRITE,
+ NULL, NULL, &graph_depth_fops);
return 0;
}
diff --git a/kernel/trace/trace_hwlat.c b/kernel/trace/trace_hwlat.c
index b791524a6536..7020ee831929 100644
--- a/kernel/trace/trace_hwlat.c
+++ b/kernel/trace/trace_hwlat.c
@@ -775,25 +775,25 @@ static int init_tracefs(void)
if (ret)
return -ENOMEM;
- top_dir = tracefs_create_dir("hwlat_detector", NULL);
+ top_dir = tracefs_create_dir(&nop_mnt_idmap, "hwlat_detector", NULL);
if (!top_dir)
return -ENOMEM;
- hwlat_sample_window = tracefs_create_file("window", TRACE_MODE_WRITE,
+ hwlat_sample_window = tracefs_create_file(&nop_mnt_idmap, "window", TRACE_MODE_WRITE,
top_dir,
&hwlat_window,
&trace_min_max_fops);
if (!hwlat_sample_window)
goto err;
- hwlat_sample_width = tracefs_create_file("width", TRACE_MODE_WRITE,
+ hwlat_sample_width = tracefs_create_file(&nop_mnt_idmap, "width", TRACE_MODE_WRITE,
top_dir,
&hwlat_width,
&trace_min_max_fops);
if (!hwlat_sample_width)
goto err;
- hwlat_thread_mode = trace_create_file("mode", TRACE_MODE_WRITE,
+ hwlat_thread_mode = trace_create_file(&nop_mnt_idmap, "mode", TRACE_MODE_WRITE,
top_dir,
NULL,
&thread_mode_fops);
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 52f8b537dd0a..c74f0524b5d4 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1914,11 +1914,11 @@ static __init int init_kprobe_trace(void)
return 0;
/* Event list interface */
- trace_create_file("kprobe_events", TRACE_MODE_WRITE,
+ trace_create_file(&nop_mnt_idmap, "kprobe_events", TRACE_MODE_WRITE,
NULL, NULL, &kprobe_events_ops);
/* Profile interface */
- trace_create_file("kprobe_profile", TRACE_MODE_READ,
+ trace_create_file(&nop_mnt_idmap, "kprobe_profile", TRACE_MODE_READ,
NULL, NULL, &kprobe_profile_ops);
setup_boot_kprobe_events();
diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index bd0d01d00fb9..a7d073c2d97c 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c
@@ -2692,7 +2692,8 @@ static int init_timerlat_stack_tracefs(struct dentry *top_dir)
{
struct dentry *tmp;
- tmp = tracefs_create_file("print_stack", TRACE_MODE_WRITE, top_dir,
+ tmp = tracefs_create_file(&nop_mnt_idmap, "print_stack",
+ TRACE_MODE_WRITE, top_dir,
&osnoise_print_stack, &trace_min_max_fops);
if (!tmp)
return -ENOMEM;
@@ -2720,18 +2721,19 @@ static int osnoise_create_cpu_timerlat_fd(struct dentry *top_dir)
* Because osnoise/timerlat have a single workload, having
* multiple files like these are wast of memory.
*/
- per_cpu = tracefs_create_dir("per_cpu", top_dir);
+ per_cpu = tracefs_create_dir(idmap, "per_cpu", top_dir);
if (!per_cpu)
return -ENOMEM;
for_each_possible_cpu(cpu) {
snprintf(cpu_str, 30, "cpu%ld", cpu);
- cpu_dir = tracefs_create_dir(cpu_str, per_cpu);
+ cpu_dir = tracefs_create_dir(idmap, cpu_str, per_cpu);
if (!cpu_dir)
goto out_clean;
- timerlat_fd = trace_create_file("timerlat_fd", TRACE_MODE_READ,
- cpu_dir, NULL, &timerlat_fd_fops);
+ timerlat_fd = trace_create_file(&nop_mnt_idmap, "timerlat_fd",
+ TRACE_MODE_READ, cpu_dir, NULL,
+ &timerlat_fd_fops);
if (!timerlat_fd)
goto out_clean;
@@ -2754,8 +2756,9 @@ static int init_timerlat_tracefs(struct dentry *top_dir)
struct dentry *tmp;
int retval;
- tmp = tracefs_create_file("timerlat_period_us", TRACE_MODE_WRITE, top_dir,
- &timerlat_period, &trace_min_max_fops);
+ tmp = tracefs_create_file(&nop_mnt_idmap, "timerlat_period_us",
+ TRACE_MODE_WRITE, top_dir, &timerlat_period,
+ &trace_min_max_fops);
if (!tmp)
return -ENOMEM;
@@ -2789,36 +2792,43 @@ static int init_tracefs(void)
if (ret)
return -ENOMEM;
- top_dir = tracefs_create_dir("osnoise", NULL);
+ top_dir = tracefs_create_dir(&nop_mnt_idmap, "osnoise", NULL);
if (!top_dir)
return 0;
- tmp = tracefs_create_file("period_us", TRACE_MODE_WRITE, top_dir,
- &osnoise_period, &trace_min_max_fops);
+ tmp = tracefs_create_file(&nop_mnt_idmap, "period_us", TRACE_MODE_WRITE,
+ top_dir, &osnoise_period,
+ &trace_min_max_fops);
if (!tmp)
goto err;
- tmp = tracefs_create_file("runtime_us", TRACE_MODE_WRITE, top_dir,
- &osnoise_runtime, &trace_min_max_fops);
+ tmp = tracefs_create_file(&nop_mnt_idmap, "runtime_us",
+ TRACE_MODE_WRITE, top_dir, &osnoise_runtime,
+ &trace_min_max_fops);
if (!tmp)
goto err;
- tmp = tracefs_create_file("stop_tracing_us", TRACE_MODE_WRITE, top_dir,
- &osnoise_stop_tracing_in, &trace_min_max_fops);
+ tmp = tracefs_create_file(&nop_mnt_idmap, "stop_tracing_us",
+ TRACE_MODE_WRITE, top_dir,
+ &osnoise_stop_tracing_in,
+ &trace_min_max_fops);
if (!tmp)
goto err;
- tmp = tracefs_create_file("stop_tracing_total_us", TRACE_MODE_WRITE, top_dir,
- &osnoise_stop_tracing_total, &trace_min_max_fops);
+ tmp = tracefs_create_file(&nop_mnt_idmap, "stop_tracing_total_us",
+ TRACE_MODE_WRITE, top_dir,
+ &osnoise_stop_tracing_total,
+ &trace_min_max_fops);
if (!tmp)
goto err;
- tmp = trace_create_file("cpus", TRACE_MODE_WRITE, top_dir, NULL, &cpus_fops);
+ tmp = trace_create_file(&nop_mnt_idmap, "cpus", TRACE_MODE_WRITE,
+ top_dir, NULL, &cpus_fops);
if (!tmp)
goto err;
- tmp = trace_create_file("options", TRACE_MODE_WRITE, top_dir, NULL,
- &osnoise_options_fops);
+ tmp = trace_create_file(&nop_mnt_idmap, "options", TRACE_MODE_WRITE,
+ top_dir, NULL, &osnoise_options_fops);
if (!tmp)
goto err;
diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
index 29f6e95439b6..1a5cb6aa495e 100644
--- a/kernel/trace/trace_printk.c
+++ b/kernel/trace/trace_printk.c
@@ -384,8 +384,8 @@ static __init int init_trace_printk_function_export(void)
if (ret)
return 0;
- trace_create_file("printk_formats", TRACE_MODE_READ, NULL,
- NULL, &ftrace_formats_fops);
+ trace_create_file(&nop_mnt_idmap, "printk_formats", TRACE_MODE_READ,
+ NULL, NULL, &ftrace_formats_fops);
return 0;
}
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 5a48dba912ea..06e63086a081 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -559,15 +559,16 @@ static __init int stack_trace_init(void)
if (ret)
return 0;
- trace_create_file("stack_max_size", TRACE_MODE_WRITE, NULL,
- &stack_trace_max_size, &stack_max_size_fops);
+ trace_create_file(&nop_mnt_idmap, "stack_max_size", TRACE_MODE_WRITE,
+ NULL, &stack_trace_max_size, &stack_max_size_fops);
- trace_create_file("stack_trace", TRACE_MODE_READ, NULL,
+ trace_create_file(&nop_mnt_idmap, "stack_trace", TRACE_MODE_READ, NULL,
NULL, &stack_trace_fops);
#ifdef CONFIG_DYNAMIC_FTRACE
- trace_create_file("stack_trace_filter", TRACE_MODE_WRITE, NULL,
- &trace_ops, &stack_trace_filter_fops);
+ trace_create_file(&nop_mnt_idmap, "stack_trace_filter",
+ TRACE_MODE_WRITE, NULL, &trace_ops,
+ &stack_trace_filter_fops);
#endif
if (stack_trace_filter_buf[0])
diff --git a/kernel/trace/trace_stat.c b/kernel/trace/trace_stat.c
index bb247beec447..ca6e322468aa 100644
--- a/kernel/trace/trace_stat.c
+++ b/kernel/trace/trace_stat.c
@@ -282,7 +282,7 @@ static int tracing_stat_init(void)
if (ret)
return -ENODEV;
- stat_dir = tracefs_create_dir("trace_stat", NULL);
+ stat_dir = tracefs_create_dir(&nop_mnt_idmap, "trace_stat", NULL);
if (!stat_dir) {
pr_warn("Could not create tracefs 'trace_stat' entry\n");
return -ENOMEM;
@@ -297,8 +297,8 @@ static int init_stat_file(struct stat_session *session)
if (!stat_dir && (ret = tracing_stat_init()))
return ret;
- session->file = tracefs_create_file(session->ts->name, TRACE_MODE_WRITE,
- stat_dir, session,
+ session->file = tracefs_create_file(&nop_mnt_idmap, session->ts->name,
+ TRACE_MODE_WRITE, stat_dir, session,
&tracing_stat_fops);
if (!session->file)
return -ENOMEM;
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 99c051de412a..060160c63f43 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -1654,10 +1654,10 @@ static __init int init_uprobe_trace(void)
if (ret)
return 0;
- trace_create_file("uprobe_events", TRACE_MODE_WRITE, NULL,
+ trace_create_file(&nop_mnt_idmap, "uprobe_events", TRACE_MODE_WRITE, NULL,
NULL, &uprobe_events_ops);
/* Profile interface */
- trace_create_file("uprobe_profile", TRACE_MODE_READ, NULL,
+ trace_create_file(&nop_mnt_idmap, "uprobe_profile", TRACE_MODE_READ, NULL,
NULL, &uprobe_profile_ops);
return 0;
}
--
2.43.0
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?
> >
> > No, in fact tracing doesn't work well with containers as tracing is global
> > to the entire machine. It can work with privileged containers though.
>
> At least the tracefs interface is easily supportable within a delegation
> model. IOW, you have a privileged process that delegates relevant
> portions to a container via idmapped mounts _without_ doing the insane thing
> and making it mountable by a container aka the fs-to-CVE pipeline.
>
> >
> > The reason for this is because tracefs was based off of debugfs where the
> > files and directores are created at boot up and mounted later. The reason
> > to do this was to allow users to mount with gid=GID to allow a given group
> > to have access to tracing. Without this update, tracefs would ignore it
> > like debugfs and proc does today.
> >
> > I think its time I explain the purpose of tracefs and how it came to be.
> >
> > The tracing system required a way to control tracing and read the traces.
> > It could have just used a new system like perf (although
> > /sys/kernel/debug/tracing predates perf), where it created a single ioctl()
> > like system call do do everything.
> >
> > As the ftrace tracing came from PREEMPT_RT latency tracer and my own logdev
> > tracer, which both have an embedded background, I chose an interface that
> > could work with just an unmodified version of busybox. That is, I wanted it
> > to work with just cat and echo.
> >
> > The main difference with tracefs compared to other file systems is that it
> > is a control interface, where writes happen as much as reads. The data read
> > is controlled. The closest thing I can think of is how cgroups work.
> >
> > As tracing is a privileged operation, but something that could be changed
> > to allow a group to have access to, I wanted to make it easy for an admin
> > to decide who gets to do what at boot up via the /etc/fstab file.
>
> Yeah, ok. I think you could achieve the same thing via idmapped mounts. You
> just need to swap out the mnt on /sys/kernel/tracing with an idmapped mount.
>
> mount(8) should just give you the ability to specify "map the ids I explicitly
> want to remap to something else and for the rest use the identity mapping". I
> wanted that for other reasons anyway.
>
> So in one of the next versions of mount(8) you can then do (where --beneath
> means place the mount beneath the current one and --replace is
> self-explanatory):
>
> sudo mount --beneath -o X-mount.idmap='g:0:1234:1 u:0:0:1' /sys/kernel/tracing
> sudo umount /sys/kernel/tracing
>
> or as a shortcut provided by mount(8):
>
> sudo mount --replace -o X-mount.idmap='g:0:1234:1 u:0:0:1' /sys/kernel/tracing
>
> In both cases you replace the mount without unmounting tracefs.
>
> I can illustrate this right now though:
>
> user1@localhost:~$ sudo mount --bind -o X-mount.idmap='g:0:1000:1 u:0:1000:1' /sys/kernel/tracing/ /mnt/
>
> # This is a tool I wrote for testing the patchset I wrote back then.
> user1@localhost:~/data/move-mount-beneath$ sudo ./move-mount --beneath --detached /mnt /sys/kernel/tracing
> Mounting beneath top mount
> Creating anonymous mount
> Attaching mount /mnt -> /sys/kernel/tracing
> Creating single detached mount
>
> user1@localhost:~/data/move-mount-beneath$
>
> # Now there's two mounts stacked on top of each other.
> user1@localhost:~/data/move-mount-beneath$ findmnt | grep tracing
> | `-/sys/kernel/tracing tracefs tracefs rw,nosuid,nodev,noexec,relatime,idmapped
> | `-/sys/kernel/tracing tracefs tracefs rw,nosuid,nodev,noexec,relatime
>
> user1@localhost:~/data/move-mount-beneath$ sudo ls -al /sys/kernel/tracing/| head
> total 0
> drwx------ 6 root root 0 Jan 7 13:33 .
> drwxr-xr-x 16 root root 0 Jan 7 13:33 ..
> -r--r----- 1 root root 0 Jan 7 13:33 README
> -r--r----- 1 root root 0 Jan 7 13:33 available_events
> -r--r----- 1 root root 0 Jan 7 13:33 available_filter_functions
> -r--r----- 1 root root 0 Jan 7 13:33 available_filter_functions_addrs
> -r--r----- 1 root root 0 Jan 7 13:33 available_tracers
> -rw-r----- 1 root root 0 Jan 7 13:33 buffer_percent
> -rw-r----- 1 root root 0 Jan 7 13:33 buffer_size_kb
>
> # Reveal updated mount
> user1@localhost:~/data/move-mount-beneath$ sudo umount /sys/kernel/tracing
>
> user1@localhost:~/data/move-mount-beneath$ findmnt | grep tracing
> | `-/sys/kernel/tracing tracefs tracefs rw,nosuid,nodev,noexec,relatime,idmapped
>
> user1@localhost:~/data/move-mount-beneath$ sudo ls -al /sys/kernel/tracing/| head
> total 0
> drwx------ 6 user1 user1 0 Jan 7 13:33 .
> drwxr-xr-x 16 root root 0 Jan 7 13:33 ..
> -r--r----- 1 user1 user1 0 Jan 7 13:33 README
> -r--r----- 1 user1 user1 0 Jan 7 13:33 available_events
> -r--r----- 1 user1 user1 0 Jan 7 13:33 available_filter_functions
> -r--r----- 1 user1 user1 0 Jan 7 13:33 available_filter_functions_addrs
> -r--r----- 1 user1 user1 0 Jan 7 13:33 available_tracers
> -rw-r----- 1 user1 user1 0 Jan 7 13:33 buffer_percent
> -rw-r----- 1 user1 user1 0 Jan 7 13:33 buffer_size_kb
>
> sudo umount -l /sys/kernel/tracing
>
> and reveal the new mount with updated permissions and at no point in time will
> you have had to unmount tracefs itself. No chown needed, no remount needed that
> has to touch all inodes.
>
> I did perf numbers for this when I implemented this and there's no meaningful
> perf impact for anything below 5 mappings. which covers 80% of the use-cases.
>
> I mean, there are crazy people out there that do have 30 mappings, maybe. And
> maybe there's 3 users that go into the hundreds (340 is maximum so struct idmap
> still fits into cacheline) because of some LDAP crap or something.
>
> See an ext4 filesystem to open/create 1,000,000 files. Then I looped through
> all of the files calling fstat() on each of them 1000 times and calculated the
> mean fstat() time for a single file.
>
> | # MAPPINGS | PATCH-NEW |
> |--------------|-----------|
> | 0 mappings | 158 ns |
> | 1 mappings | 157 ns |
> | 2 mappings | 158 ns |
> | 3 mappings | 161 ns |
> | 5 mappings | 165 ns |
> | 10 mappings | 199 ns |
> | 50 mappings | 218 ns |
> | 100 mappings | 229 ns |
> | 200 mappings | 239 ns |
> | 300 mappings | 240 ns |
> | 340 mappings | 248 ns |
>
> >
> > >
> > > Aside from optimizing this and the special semantics for this eventfs
> > > stuff that you really should think twice of doing, here's one idea for
> > > an extension that might alleviate some of the pain:
> > >
> > > If you need flexible dynamic ownership change to e.g., be able to
> > > delegate (all, a directory, a single file of) tracefs to
> > > unprivileged/containers/whatever then you might want to consider
> > > supporting idmapped mounts for tracefs. Because then you can do stuff
> > > like:
> >
> > I'm a novice here and have no idea on how id maps work ;-)
>
> It's no magic and you don't even need to care about it. If you're on
> util-linux 2.39 and any kernel post 5.12 the -o X-mount.idmap option
> does all the details for you.
>
> Though I still want an api where you can just pass the idmappings directly to
> mount_setattr(). That's another topic though.
>
> > have. Now, can we do that and still keep the dynamic creation of inodes and
> > dentries?
>
> Yes.
>
> > Does this require having more than one dentry per inode?
>
> No. It's just a topological change. The same subtree exposed at different
> locations with the ability of exposing it with different permissions (without
> any persistent filesystem-level changes).
>
> 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 that don't have a custom
> ->permission() handler. So there's nothing to do for us here.
>
> * Idmapped mount handling for ->getattr() is done completely by the VFS
> if the filesystem doesn't have a custom ->getattr() handler. So we're
> done here.
>
> * Tracefs doesn't support attribute changes via ->setattr() (chown(),
> chmod etc.). So there's nothing to here.
>
> * Eventfs does support attribute changes via ->setattr(). But it relies
> on simple_setattr() which is already idmapped mount aware. So there's
> nothing for us to do.
>
> * Ownership is inherited from the parent inode (tracefs) or optionally
> from stashed ownership information (eventfs). That means the idmapping
> is irrelevant here. It's similar to the "inherit gid from parent
> directory" logic we have in some circumstances. TL;DR nothing to do
> here as well.
>
> * 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 through to most helpers.
>
> There's some subtlety in eventfs. Afaict, the directories and files for
> the individual events are created on-demand during lookup or readdir.
>
> The ownership of these events is again inherited from the parent inode
> or recovered from stored state. In both cases the actual idmapping is
> irrelevant.
>
> The callchain here is:
>
> eventfs_root_lookup("xfs", "events")
> -> create_{dir,file}_dentry("xfs", "events")
> -> create_{dir,file}("xfs", "events")
> -> eventfs_start_creating("xfs", "events")
> -> lookup_one_len("xfs", "events")
>
> And the subtlety is that lookup_one_len() does permission checking on
> the parent inode (IOW, if you want a dentry for "blech" under "events"
> it'll do a permission check on events->d_inode) for exec permissions
> and then goes on to give you a new dentry.
>
> Usually this call would have to be changed to lookup_one() and the
> idmapping be handed down to it. But I think that's irrelevant here.
>
> Lookup generally doesn't need to be aware of idmappings at all. The
> permission checking is done purely in the vfs via may_lookup() and the
> idmapping is irrelevant because we always initialize inodes with the
> filesystem level ownership (see the idmappings.rst) documentation if
> you're interested in excessive details (otherwise you get inode aliases
> which you really don't want).
>
> For tracefs it would not matter for lookup per se but only because
> tracefs seemingly creates inodes/dentries during lookup (and readdir()).
>
> But imho the permission checking done in current eventfs_root_lookup()
> via lookup_one_len() is meaningless in any way; possibly even
> (conceptually) wrong.
>
> Because, the actual permission checking for the creation of the eventfs
> entries isn't really done during lookup or readdir, it's done when mkdir
> is called:
>
> mkdir /sys/kernel/tracing/instances/foo
>
> Here, all possible entries beneath foo including "events" and further
> below are recorded and stored. So once mkdir returns it basically means
> that it succeeded with the creation of all the necessary directories and
> files. For all purposes the foo/events/ directory and below have all the
> entries that matter. They have been created. It's comparable to them not
> being in the {d,i}cache, I guess.
>
> When one goes and looksup stuff under foo/events/ or readdir the entries
> in that directory:
>
> fd = open("foo/events")
> readdir(fd, ...)
>
> then they are licensed to list an entry in that directory. So all that
> needs to be done is to actually list those files in that directory. And
> since they already exist (they were created during mkdir) we just need
> to splice in inodes and dentries for them. But for that we shouldn't
> check permissions on the directory again. Because we've done that
> already correctly when the VFS called may_lookup().
>
> IOW, the inode_permission() in lookup_one_len() that eventfs does is
> redundant and just wrong.
>
> Luckily, I don't think we need to even change anything because all
> directories that eventfs creates always grant exec permissions to the
> other group so lookup_one_len() will trivially succeed. IIUC.
>
> Drafted-by-with-no-guarantees-whatsoever-that-this-wont-burn-the-house-down: Christian Brauner <[email protected]>
Actually, I think that patch can be way simpler for a similar reason
than I mentioned before. In the regular system call for tracefs we have:
mkdir /sys/kernel/tracing/instances/foo
-> do_mkdirat("foo")
-> i_op->mkdir::tracefs_syscall_mkdir(child_dentry::foo, parent_inode)
-> char *name = get_dname(child_dentry::foo) // duplicate the dentries name
-> tracefs_ops.mkdir::instance_mkdir("foo")
-> trace_array_create("foo")
-> trace_array_create_dir("foo")
-> tracefs_create_dir("foo", trace_instance_dir)
-> __create_dir("foo", trace_instance_dir)
-> tracefs_start_creating("foo", trace_instance_dir)
-> lookup_one_len("foo", trace_instance_dir) // perform the lookup again and find @child_dentry again that the vfs already created
So afaict, the vfs has created you the correct target dentry and
performed the required permission checks already.
You then go on to retrieve and duplicate the name of the dentry and pass
that name down. And then you perform a lookup via lookup_one_len() on
the instances directory. And here lookup_one_len() performs the
permission checking again that the vfs already did in may_lookup() when
it walked up to the "instances" directory.
The reason I'm saying this is that for tracefs _the only reason_ we need
that idmapping in any of these helpers at all is because of the
superfluous permission check in lookup_one_len(). Because for actual
ownership of the inode it's completely irrelevant because you inherit
the ownership anyway. IOW, the callers fs{g,u}id etc is entirely
irrelevant.
So basically if you massage the system call path to simply reuse the
dentry that the vfs already given you instead of looking it up again
later the whole patch is literally (baring anything minor I forgot):
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index ae648deed019..2de3ec114793 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -439,6 +439,7 @@ static struct file_system_type trace_fs_type = {
.name = "tracefs",
.mount = trace_mount,
.kill_sb = kill_litter_super,
+ .fs_flags = FS_ALLOW_IDMAP,
};
MODULE_ALIAS_FS("tracefs");
Because tracefs simply inherits ownership from parent directories or
from stashed info the idmapping is entirely immaterial on the creation
side of things. As soon as the permission checking in the VFS has
succeeded we know that things are alright.
So really, the complexity is getting in your way here. The
tracefs_ops.mkdir thing is only used in a single location and that is
kernel/trace/trace.c: trace_instance_dir = tracefs_create_instance_dir("instances", d_tracer,
and there's only one callback instance_mkdir().
otherwise that callback ops stuff is completely unused iiuc. If that's
the case it would make a lot more sense to just reuse everything that
the VFS has given you in the regular system call path than to do that
"copy the name and do the lookup again including permission checks
later" dance.
Idk, maybe I'm seeing things and there's an obvious reason why my
thinking is all wrong and that permission check is required. I just
don't see it though. But I'm sure people will tell me what my error is.
At least for the system call path it shouldn't be.
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
> > > > 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 the entire machine. It can work with privileged containers though.
> >
> > At least the tracefs interface is easily supportable within a delegation
> > model. IOW, you have a privileged process that delegates relevant
> > portions to a container via idmapped mounts _without_ doing the insane thing
> > and making it mountable by a container aka the fs-to-CVE pipeline.
> >
> > >
> > > The reason for this is because tracefs was based off of debugfs where the
> > > files and directores are created at boot up and mounted later. The reason
> > > to do this was to allow users to mount with gid=GID to allow a given group
> > > to have access to tracing. Without this update, tracefs would ignore it
> > > like debugfs and proc does today.
> > >
> > > I think its time I explain the purpose of tracefs and how it came to be.
> > >
> > > The tracing system required a way to control tracing and read the traces.
> > > It could have just used a new system like perf (although
> > > /sys/kernel/debug/tracing predates perf), where it created a single ioctl()
> > > like system call do do everything.
> > >
> > > As the ftrace tracing came from PREEMPT_RT latency tracer and my own logdev
> > > tracer, which both have an embedded background, I chose an interface that
> > > could work with just an unmodified version of busybox. That is, I wanted it
> > > to work with just cat and echo.
> > >
> > > The main difference with tracefs compared to other file systems is that it
> > > is a control interface, where writes happen as much as reads. The data read
> > > is controlled. The closest thing I can think of is how cgroups work.
> > >
> > > As tracing is a privileged operation, but something that could be changed
> > > to allow a group to have access to, I wanted to make it easy for an admin
> > > to decide who gets to do what at boot up via the /etc/fstab file.
> >
> > Yeah, ok. I think you could achieve the same thing via idmapped mounts. You
> > just need to swap out the mnt on /sys/kernel/tracing with an idmapped mount.
> >
> > mount(8) should just give you the ability to specify "map the ids I explicitly
> > want to remap to something else and for the rest use the identity mapping". I
> > wanted that for other reasons anyway.
> >
> > So in one of the next versions of mount(8) you can then do (where --beneath
> > means place the mount beneath the current one and --replace is
> > self-explanatory):
> >
> > sudo mount --beneath -o X-mount.idmap='g:0:1234:1 u:0:0:1' /sys/kernel/tracing
> > sudo umount /sys/kernel/tracing
> >
> > or as a shortcut provided by mount(8):
> >
> > sudo mount --replace -o X-mount.idmap='g:0:1234:1 u:0:0:1' /sys/kernel/tracing
> >
> > In both cases you replace the mount without unmounting tracefs.
> >
> > I can illustrate this right now though:
> >
> > user1@localhost:~$ sudo mount --bind -o X-mount.idmap='g:0:1000:1 u:0:1000:1' /sys/kernel/tracing/ /mnt/
> >
> > # This is a tool I wrote for testing the patchset I wrote back then.
> > user1@localhost:~/data/move-mount-beneath$ sudo ./move-mount --beneath --detached /mnt /sys/kernel/tracing
> > Mounting beneath top mount
> > Creating anonymous mount
> > Attaching mount /mnt -> /sys/kernel/tracing
> > Creating single detached mount
> >
> > user1@localhost:~/data/move-mount-beneath$
> >
> > # Now there's two mounts stacked on top of each other.
> > user1@localhost:~/data/move-mount-beneath$ findmnt | grep tracing
> > | `-/sys/kernel/tracing tracefs tracefs rw,nosuid,nodev,noexec,relatime,idmapped
> > | `-/sys/kernel/tracing tracefs tracefs rw,nosuid,nodev,noexec,relatime
> >
> > user1@localhost:~/data/move-mount-beneath$ sudo ls -al /sys/kernel/tracing/| head
> > total 0
> > drwx------ 6 root root 0 Jan 7 13:33 .
> > drwxr-xr-x 16 root root 0 Jan 7 13:33 ..
> > -r--r----- 1 root root 0 Jan 7 13:33 README
> > -r--r----- 1 root root 0 Jan 7 13:33 available_events
> > -r--r----- 1 root root 0 Jan 7 13:33 available_filter_functions
> > -r--r----- 1 root root 0 Jan 7 13:33 available_filter_functions_addrs
> > -r--r----- 1 root root 0 Jan 7 13:33 available_tracers
> > -rw-r----- 1 root root 0 Jan 7 13:33 buffer_percent
> > -rw-r----- 1 root root 0 Jan 7 13:33 buffer_size_kb
> >
> > # Reveal updated mount
> > user1@localhost:~/data/move-mount-beneath$ sudo umount /sys/kernel/tracing
> >
> > user1@localhost:~/data/move-mount-beneath$ findmnt | grep tracing
> > | `-/sys/kernel/tracing tracefs tracefs rw,nosuid,nodev,noexec,relatime,idmapped
> >
> > user1@localhost:~/data/move-mount-beneath$ sudo ls -al /sys/kernel/tracing/| head
> > total 0
> > drwx------ 6 user1 user1 0 Jan 7 13:33 .
> > drwxr-xr-x 16 root root 0 Jan 7 13:33 ..
> > -r--r----- 1 user1 user1 0 Jan 7 13:33 README
> > -r--r----- 1 user1 user1 0 Jan 7 13:33 available_events
> > -r--r----- 1 user1 user1 0 Jan 7 13:33 available_filter_functions
> > -r--r----- 1 user1 user1 0 Jan 7 13:33 available_filter_functions_addrs
> > -r--r----- 1 user1 user1 0 Jan 7 13:33 available_tracers
> > -rw-r----- 1 user1 user1 0 Jan 7 13:33 buffer_percent
> > -rw-r----- 1 user1 user1 0 Jan 7 13:33 buffer_size_kb
> >
> > sudo umount -l /sys/kernel/tracing
> >
> > and reveal the new mount with updated permissions and at no point in time will
> > you have had to unmount tracefs itself. No chown needed, no remount needed that
> > has to touch all inodes.
> >
> > I did perf numbers for this when I implemented this and there's no meaningful
> > perf impact for anything below 5 mappings. which covers 80% of the use-cases.
> >
> > I mean, there are crazy people out there that do have 30 mappings, maybe. And
> > maybe there's 3 users that go into the hundreds (340 is maximum so struct idmap
> > still fits into cacheline) because of some LDAP crap or something.
> >
> > See an ext4 filesystem to open/create 1,000,000 files. Then I looped through
> > all of the files calling fstat() on each of them 1000 times and calculated the
> > mean fstat() time for a single file.
> >
> > | # MAPPINGS | PATCH-NEW |
> > |--------------|-----------|
> > | 0 mappings | 158 ns |
> > | 1 mappings | 157 ns |
> > | 2 mappings | 158 ns |
> > | 3 mappings | 161 ns |
> > | 5 mappings | 165 ns |
> > | 10 mappings | 199 ns |
> > | 50 mappings | 218 ns |
> > | 100 mappings | 229 ns |
> > | 200 mappings | 239 ns |
> > | 300 mappings | 240 ns |
> > | 340 mappings | 248 ns |
> >
> > >
> > > >
> > > > Aside from optimizing this and the special semantics for this eventfs
> > > > stuff that you really should think twice of doing, here's one idea for
> > > > an extension that might alleviate some of the pain:
> > > >
> > > > If you need flexible dynamic ownership change to e.g., be able to
> > > > delegate (all, a directory, a single file of) tracefs to
> > > > unprivileged/containers/whatever then you might want to consider
> > > > supporting idmapped mounts for tracefs. Because then you can do stuff
> > > > like:
> > >
> > > I'm a novice here and have no idea on how id maps work ;-)
> >
> > It's no magic and you don't even need to care about it. If you're on
> > util-linux 2.39 and any kernel post 5.12 the -o X-mount.idmap option
> > does all the details for you.
> >
> > Though I still want an api where you can just pass the idmappings directly to
> > mount_setattr(). That's another topic though.
> >
> > > have. Now, can we do that and still keep the dynamic creation of inodes and
> > > dentries?
> >
> > Yes.
> >
> > > Does this require having more than one dentry per inode?
> >
> > No. It's just a topological change. The same subtree exposed at different
> > locations with the ability of exposing it with different permissions (without
> > any persistent filesystem-level changes).
> >
> > 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 that don't have a custom
> > ->permission() handler. So there's nothing to do for us here.
> >
> > * Idmapped mount handling for ->getattr() is done completely by the VFS
> > if the filesystem doesn't have a custom ->getattr() handler. So we're
> > done here.
> >
> > * Tracefs doesn't support attribute changes via ->setattr() (chown(),
> > chmod etc.). So there's nothing to here.
> >
> > * Eventfs does support attribute changes via ->setattr(). But it relies
> > on simple_setattr() which is already idmapped mount aware. So there's
> > nothing for us to do.
> >
> > * Ownership is inherited from the parent inode (tracefs) or optionally
> > from stashed ownership information (eventfs). That means the idmapping
> > is irrelevant here. It's similar to the "inherit gid from parent
> > directory" logic we have in some circumstances. TL;DR nothing to do
> > here as well.
> >
> > * 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 through to most helpers.
> >
> > There's some subtlety in eventfs. Afaict, the directories and files for
> > the individual events are created on-demand during lookup or readdir.
> >
> > The ownership of these events is again inherited from the parent inode
> > or recovered from stored state. In both cases the actual idmapping is
> > irrelevant.
> >
> > The callchain here is:
> >
> > eventfs_root_lookup("xfs", "events")
> > -> create_{dir,file}_dentry("xfs", "events")
> > -> create_{dir,file}("xfs", "events")
> > -> eventfs_start_creating("xfs", "events")
> > -> lookup_one_len("xfs", "events")
> >
> > And the subtlety is that lookup_one_len() does permission checking on
> > the parent inode (IOW, if you want a dentry for "blech" under "events"
> > it'll do a permission check on events->d_inode) for exec permissions
> > and then goes on to give you a new dentry.
> >
> > Usually this call would have to be changed to lookup_one() and the
> > idmapping be handed down to it. But I think that's irrelevant here.
> >
> > Lookup generally doesn't need to be aware of idmappings at all. The
> > permission checking is done purely in the vfs via may_lookup() and the
> > idmapping is irrelevant because we always initialize inodes with the
> > filesystem level ownership (see the idmappings.rst) documentation if
> > you're interested in excessive details (otherwise you get inode aliases
> > which you really don't want).
> >
> > For tracefs it would not matter for lookup per se but only because
> > tracefs seemingly creates inodes/dentries during lookup (and readdir()).
> >
> > But imho the permission checking done in current eventfs_root_lookup()
> > via lookup_one_len() is meaningless in any way; possibly even
> > (conceptually) wrong.
> >
> > Because, the actual permission checking for the creation of the eventfs
> > entries isn't really done during lookup or readdir, it's done when mkdir
> > is called:
> >
> > mkdir /sys/kernel/tracing/instances/foo
> >
> > Here, all possible entries beneath foo including "events" and further
> > below are recorded and stored. So once mkdir returns it basically means
> > that it succeeded with the creation of all the necessary directories and
> > files. For all purposes the foo/events/ directory and below have all the
> > entries that matter. They have been created. It's comparable to them not
> > being in the {d,i}cache, I guess.
> >
> > When one goes and looksup stuff under foo/events/ or readdir the entries
> > in that directory:
> >
> > fd = open("foo/events")
> > readdir(fd, ...)
> >
> > then they are licensed to list an entry in that directory. So all that
> > needs to be done is to actually list those files in that directory. And
> > since they already exist (they were created during mkdir) we just need
> > to splice in inodes and dentries for them. But for that we shouldn't
> > check permissions on the directory again. Because we've done that
> > already correctly when the VFS called may_lookup().
> >
> > IOW, the inode_permission() in lookup_one_len() that eventfs does is
> > redundant and just wrong.
> >
> > Luckily, I don't think we need to even change anything because all
> > directories that eventfs creates always grant exec permissions to the
> > other group so lookup_one_len() will trivially succeed. IIUC.
> >
> > Drafted-by-with-no-guarantees-whatsoever-that-this-wont-burn-the-house-down: Christian Brauner <[email protected]>
>
> Actually, I think that patch can be way simpler for a similar reason
> than I mentioned before. In the regular system call for tracefs we have:
>
> mkdir /sys/kernel/tracing/instances/foo
>
> -> do_mkdirat("foo")
> -> i_op->mkdir::tracefs_syscall_mkdir(child_dentry::foo, parent_inode)
> -> char *name = get_dname(child_dentry::foo) // duplicate the dentries name
> -> tracefs_ops.mkdir::instance_mkdir("foo")
> -> trace_array_create("foo")
> -> trace_array_create_dir("foo")
> -> tracefs_create_dir("foo", trace_instance_dir)
> -> __create_dir("foo", trace_instance_dir)
> -> tracefs_start_creating("foo", trace_instance_dir)
> -> lookup_one_len("foo", trace_instance_dir) // perform the lookup again and find @child_dentry again that the vfs already created
>
> So afaict, the vfs has created you the correct target dentry and
> performed the required permission checks already.
>
> You then go on to retrieve and duplicate the name of the dentry and pass
> that name down. And then you perform a lookup via lookup_one_len() on
> the instances directory. And here lookup_one_len() performs the
> permission checking again that the vfs already did in may_lookup() when
> it walked up to the "instances" directory.
>
> The reason I'm saying this is that for tracefs _the only reason_ we need
> that idmapping in any of these helpers at all is because of the
> superfluous permission check in lookup_one_len(). Because for actual
> ownership of the inode it's completely irrelevant because you inherit
> the ownership anyway. IOW, the callers fs{g,u}id etc is entirely
> irrelevant.
>
> So basically if you massage the system call path to simply reuse the
> dentry that the vfs already given you instead of looking it up again
> later the whole patch is literally (baring anything minor I forgot):
>
>
> diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> index ae648deed019..2de3ec114793 100644
> --- a/fs/tracefs/inode.c
> +++ b/fs/tracefs/inode.c
> @@ -439,6 +439,7 @@ static struct file_system_type trace_fs_type = {
> .name = "tracefs",
> .mount = trace_mount,
> .kill_sb = kill_litter_super,
> + .fs_flags = FS_ALLOW_IDMAP,
> };
> MODULE_ALIAS_FS("tracefs");
>
> Because tracefs simply inherits ownership from parent directories or
> from stashed info the idmapping is entirely immaterial on the creation
> side of things. As soon as the permission checking in the VFS has
> succeeded we know that things are alright.
>
> So really, the complexity is getting in your way here. The
> tracefs_ops.mkdir thing is only used in a single location and that is
>
> kernel/trace/trace.c: trace_instance_dir = tracefs_create_instance_dir("instances", d_tracer,
>
> and there's only one callback instance_mkdir().
>
> otherwise that callback ops stuff is completely unused iiuc. If that's
> the case it would make a lot more sense to just reuse everything that
> the VFS has given you in the regular system call path than to do that
> "copy the name and do the lookup again including permission checks
> later" dance.
>
> Idk, maybe I'm seeing things and there's an obvious reason why my
> thinking is all wrong and that permission check is required. I just
> don't see it though. But I'm sure people will tell me what my error is.
>
> At least for the system call path it shouldn't be.
I guess an argument would be that the check is somewhat required because
you create a whole hierarchy of directories and files beneath the top
level directory that the system call path creates.
But then would you ever want to fail creating any directories or files
beneath /sys/kernel/tracing/instances/foo/* if the VFS told you that you
have the required permission to create the top level directory
/sys/kernel/tracing/instances/foo?
I think that isn't the case and it isn't even possible currently. So
even for the directories and files beneath the top level directory the
permission check is irrelevant iiuc.
Anyway, I don't think you necessarily need to change any of what you do
right now. The first version of the patch will work with what you
currently have and is correct. It's just more complex because of that
additional permission checking that I think isn't needed. Because it's
not that the user has any control over the creation of the subtree
beneath /sys/kernel/tracing/instances/foo/*. But again, I might be
completely off.
On Sun, 7 Jan 2024 13:42:39 +0100
Christian Brauner <[email protected]> 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 that don't have a custom
> ->permission() handler. So there's nothing to do for us here.
>
> * Idmapped mount handling for ->getattr() is done completely by the VFS
> if the filesystem doesn't have a custom ->getattr() handler. So we're
> done here.
>
> * Tracefs doesn't support attribute changes via ->setattr() (chown(),
> chmod etc.). So there's nothing to here.
>
> * Eventfs does support attribute changes via ->setattr(). But it relies
> on simple_setattr() which is already idmapped mount aware. So there's
> nothing for us to do.
>
> * Ownership is inherited from the parent inode (tracefs) or optionally
> from stashed ownership information (eventfs). That means the idmapping
> is irrelevant here. It's similar to the "inherit gid from parent
> directory" logic we have in some circumstances. TL;DR nothing to do
> here as well.
The reason ownership is inherited from the parent is because the inodes
are created at boot up before user space starts.
eventfs inodes are created on demand after user space so it needs to
either check the "default" ownership and permissions, or if the user
changed a specific file/directory, it must save it and use that again
if the inode/dentry are reclaimed and then referenced again and
recreated.
>
> * 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 through to most helpers.
>
> There's some subtlety in eventfs. Afaict, the directories and files for
> the individual events are created on-demand during lookup or readdir.
>
> The ownership of these events is again inherited from the parent inode
> or recovered from stored state. In both cases the actual idmapping is
> irrelevant.
>
> The callchain here is:
>
> eventfs_root_lookup("xfs", "events")
> -> create_{dir,file}_dentry("xfs", "events")
> -> create_{dir,file}("xfs", "events")
> -> eventfs_start_creating("xfs", "events")
> -> lookup_one_len("xfs", "events")
>
> And the subtlety is that lookup_one_len() does permission checking on
> the parent inode (IOW, if you want a dentry for "blech" under "events"
> it'll do a permission check on events->d_inode) for exec permissions
> and then goes on to give you a new dentry.
>
> Usually this call would have to be changed to lookup_one() and the
> idmapping be handed down to it. But I think that's irrelevant here.
>
> Lookup generally doesn't need to be aware of idmappings at all. The
> permission checking is done purely in the vfs via may_lookup() and the
> idmapping is irrelevant because we always initialize inodes with the
> filesystem level ownership (see the idmappings.rst) documentation if
> you're interested in excessive details (otherwise you get inode aliases
> which you really don't want).
>
> For tracefs it would not matter for lookup per se but only because
> tracefs seemingly creates inodes/dentries during lookup (and readdir()).
tracefs creates the inodes/dentries at boot up, it's eventfs that does
it on demand during lookup.
For inodes/dentries:
/sys/kernel/tracing/* is all created at boot up, except for "events".
/sys/kernel/tracing/events/* is created on demand.
>
> But imho the permission checking done in current eventfs_root_lookup()
> via lookup_one_len() is meaningless in any way; possibly even
> (conceptually) wrong.
>
> Because, the actual permission checking for the creation of the eventfs
> entries isn't really done during lookup or readdir, it's done when mkdir
> is called:
>
> mkdir /sys/kernel/tracing/instances/foo
No. that creates a entire new tracefs instance, which happens to
include another eventfs directory.
The eventsfs directory is in "/sys/kernel/tracing/events" and
"/sys/kernel/tracing/instances/*/events"
eventfs has 10s of thousands of files and directories which is why I
changed it to be on-demand inode/dentry creation and also reclaiming
when no longer accessed.
# ls /sys/kernel/tracing/events/
will create the inodes and dentries, and a memory stress program will
free those created inodes and dentries.
>
> Here, all possible entries beneath foo including "events" and further
> below are recorded and stored. So once mkdir returns it basically means
> that it succeeded with the creation of all the necessary directories and
> files. For all purposes the foo/events/ directory and below have all the
> entries that matter. They have been created. It's comparable to them not
> being in the {d,i}cache, I guess.
No. Only the meta data is created for the eventfs directory with a
mkdir instances/foo. The inodes and dentries are not there.
>
> When one goes and looksup stuff under foo/events/ or readdir the entries
> in that directory:
>
> fd = open("foo/events")
> readdir(fd, ...)
>
> then they are licensed to list an entry in that directory. So all that
> needs to be done is to actually list those files in that directory. And
> since they already exist (they were created during mkdir) we just need
> to splice in inodes and dentries for them. But for that we shouldn't
> check permissions on the directory again. Because we've done that
> already correctly when the VFS called may_lookup().
No they do not exist.
>
> IOW, the inode_permission() in lookup_one_len() that eventfs does is
> redundant and just wrong.
I don't think so.
>
> Luckily, I don't think we need to even change anything because all
> directories that eventfs creates always grant exec permissions to the
> other group so lookup_one_len() will trivially succeed. IIUC.
>
> Drafted-by-with-no-guarantees-whatsoever-that-this-wont-burn-the-house-down: Christian Brauner <[email protected]>
Checkout this branch:
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git eventfs-show-files
It creates a file "/sys/kernel/tracing/show_events_dentries" That shows
when inodes and dentries are created and freed.
I use this to make sure that reclaim actually does reclaim them. Linus
did not like this file, but it has become very useful to make sure
things are working properly as there is no other way to know if the
inodes and dentries are reclaimed or not. I may see if he'll let me add
it with a debug config option.
-- Steve
On Sun, 7 Jan 2024 13:29:12 -0500
Steven Rostedt <[email protected]> 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 one rationale to do eventfs was so
that the instance directories wouldn't recreate the same 10thousands
event inodes and dentries for every mkdir done.
-- Steve
> > * 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 through to most helpers.
> >
> > There's some subtlety in eventfs. Afaict, the directories and files for
> > the individual events are created on-demand during lookup or readdir.
> >
> > The ownership of these events is again inherited from the parent inode
> > or recovered from stored state. In both cases the actual idmapping is
> > irrelevant.
> >
> > The callchain here is:
> >
> > eventfs_root_lookup("xfs", "events")
> > -> create_{dir,file}_dentry("xfs", "events")
> > -> create_{dir,file}("xfs", "events")
> > -> eventfs_start_creating("xfs", "events")
> > -> lookup_one_len("xfs", "events")
> >
> > And the subtlety is that lookup_one_len() does permission checking on
> > the parent inode (IOW, if you want a dentry for "blech" under "events"
> > it'll do a permission check on events->d_inode) for exec permissions
> > and then goes on to give you a new dentry.
> >
> > Usually this call would have to be changed to lookup_one() and the
> > idmapping be handed down to it. But I think that's irrelevant here.
> >
> > Lookup generally doesn't need to be aware of idmappings at all. The
> > permission checking is done purely in the vfs via may_lookup() and the
> > idmapping is irrelevant because we always initialize inodes with the
> > filesystem level ownership (see the idmappings.rst) documentation if
> > you're interested in excessive details (otherwise you get inode aliases
> > which you really don't want).
> >
> > For tracefs it would not matter for lookup per se but only because
> > tracefs seemingly creates inodes/dentries during lookup (and readdir()).
>
> tracefs creates the inodes/dentries at boot up, it's eventfs that does
> it on demand during lookup.
>
> For inodes/dentries:
>
> /sys/kernel/tracing/* is all created at boot up, except for "events".
Yes.
> /sys/kernel/tracing/events/* is created on demand.
Yes.
>
> >
> > But imho the permission checking done in current eventfs_root_lookup()
> > via lookup_one_len() is meaningless in any way; possibly even
> > (conceptually) wrong.
> >
> > Because, the actual permission checking for the creation of the eventfs
> > entries isn't really done during lookup or readdir, it's done when mkdir
> > is called:
> >
> > mkdir /sys/kernel/tracing/instances/foo
>
> No. that creates a entire new tracefs instance, which happens to
> include another eventfs directory.
Yes, I'm aware of all that.
> No. Only the meta data is created for the eventfs directory with a
> mkdir instances/foo. The inodes and dentries are not there.
I know, that is what I'm saying.
>
> >
> > When one goes and looksup stuff under foo/events/ or readdir the entries
> > in that directory:
> >
> > fd = open("foo/events")
> > readdir(fd, ...)
> >
> > then they are licensed to list an entry in that directory. So all that
> > needs to be done is to actually list those files in that directory. And
> > since they already exist (they were created during mkdir) we just need
> > to splice in inodes and dentries for them. But for that we shouldn't
> > check permissions on the directory again. Because we've done that
> > already correctly when the VFS called may_lookup().
>
> No they do not exist.
I am aware.
>
> >
> > 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 completely directory layout is determined. You're just
splicing in dentries and inodes during lookup and readdir.
If mkdir /sys/kernel/tracing/instances/foo has succeeded and you later
do a lookup/readdir on
ls -al /sys/kernel/tracing/instances/foo/events
Why should the creation of the dentries and inodes ever fail due to a
permission failure? The vfs did already verify that you had the required
permissions to list entries in that directory. Why should filling up
/sys/kernel/tracing/instances/foo/events ever fail then? It shouldn't
That tracefs instance would be half-functional. And again, right now
that inode_permission() check cannot even fail.
On Sun, Jan 07, 2024 at 01:32:28PM -0500, Steven Rostedt wrote:
> On Sun, 7 Jan 2024 13:29:12 -0500
> Steven Rostedt <[email protected]> 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 one rationale to do eventfs was so
Every instance/foo/ tracefs instances also contains an events directory
and thus a eventfs portion. Eventfs is just a subtree of tracefs. It's
not a separate filesystem. Both eventfs and tracefs are on the same
single, system wide superblock.
> that the instance directories wouldn't recreate the same 10thousands
> event inodes and dentries for every mkdir done.
I know but that's irrelevant to what I'm trying to tell you.
A mkdir /sys/kernel/tracing/instances/foo creates a new tracefs
instance. With or without the on-demand dentry and inode creation for
the eventfs portion that tracefs "instance" has now been created in its
entirety including all the required information for someone to later
come along and perform a lookup on /sys/kernel/tracing/instances/foo/events.
All you've done is to defer the addition of the dentries and inodes when
someone does actually look at the events directory of the tracefs
instance.
Whether you choose to splice in the dentries and inodes for the eventfs
portion during lookup and readdir or if you had chosen to not do the
on-demand thing at all and the entries were created at the same time as
the mkdir call are equivalent from the perspective of permission
checking.
If you have the required permissions to look at the events directory
then there's no reason why listing the directory entries in there should
fail. This can't even happen right now.
On Mon, 8 Jan 2024 12:04:54 +0100
Christian Brauner <[email protected]> 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 completely directory layout is determined. You're just
> splicing in dentries and inodes during lookup and readdir.
>
> If mkdir /sys/kernel/tracing/instances/foo has succeeded and you later
> do a lookup/readdir on
>
> ls -al /sys/kernel/tracing/instances/foo/events
>
> Why should the creation of the dentries and inodes ever fail due to a
> permission failure?
They shouldn't.
> The vfs did already verify that you had the required
> permissions to list entries in that directory. Why should filling up
> /sys/kernel/tracing/instances/foo/events ever fail then? It shouldn't
> That tracefs instance would be half-functional. And again, right now
> that inode_permission() check cannot even fail.
And it shouldn't. But without dentries and inodes, how does VFS know what
is allowed to open the files?
-- Steve
On Mon, 8 Jan 2024 12:32:46 +0100
Christian Brauner <[email protected]> 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 <[email protected]> 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 one rationale to do eventfs was so
>
> Every instance/foo/ tracefs instances also contains an events directory
> and thus a eventfs portion. Eventfs is just a subtree of tracefs. It's
> not a separate filesystem. Both eventfs and tracefs are on the same
> single, system wide superblock.
>
> > that the instance directories wouldn't recreate the same 10thousands
> > event inodes and dentries for every mkdir done.
>
> I know but that's irrelevant to what I'm trying to tell you.
>
> A mkdir /sys/kernel/tracing/instances/foo creates a new tracefs
> instance. With or without the on-demand dentry and inode creation for
> the eventfs portion that tracefs "instance" has now been created in its
> entirety including all the required information for someone to later
> come along and perform a lookup on /sys/kernel/tracing/instances/foo/events.
>
> All you've done is to defer the addition of the dentries and inodes when
> someone does actually look at the events directory of the tracefs
> instance.
>
> Whether you choose to splice in the dentries and inodes for the eventfs
> portion during lookup and readdir or if you had chosen to not do the
> on-demand thing at all and the entries were created at the same time as
> the mkdir call are equivalent from the perspective of permission
> checking.
>
> If you have the required permissions to look at the events directory
> then there's no reason why listing the directory entries in there should
> fail. This can't even happen right now.
Ah, I think I know where the confusion lies. The tracing information in
kernel/trace/*.c doesn't keep track of permission. It relies totally on
fs/tracefs/* to do so. If someone does 'chmod' or 'chown' or mounts with
'gid=xxx' then it's up to tracefs to maintain that information and not the
tracing subsystem. The tracing subsystem only gives the "default"
permissions (before boot finishes).
The difference between normal file systems and pseudo file systems like
debugfs and tracefs, is that normal file systems keep the permission
information stored on the external device. That is, when the inodes and
dentries are created, the information is retrieved from the stored file
system.
I think this may actually be a failure of debugfs (and tracefs as it was
based on debugfs), in that the inodes and dentries are created at the same
time the "files" backing them are. Which is normally at boot up and before
the file system is mounted.
That is, inodes and dentries are actually coupled with the data they
represent. It's not a cache for a back store like a hard drive partition.
To create a file in debugfs you do:
struct dentry *debugfs_create_file(const char *name, umode_t mode,
struct dentry *parent, void *data,
const struct file_operations *fops)
That is, you pass a the name, the mode, the parent dentry, data, and the
fops and that will create an inode and dentry (which is returned).
This happens at boot up before user space is running and before debugfs is
even mounted.
Because debugfs is mostly for debugging, people don't care about how it's
mounted. It is usually restricted to root only access. Especially since
there's a lot of sensitive information that shouldn't be exposed to
non-privileged users.
The reason tracefs came about is that people asked me to be able to have
access to tracing without needing to even enable debugfs. They also want to
easily make it accessible to non root users and talking with Kees Cook, he
recommended using ACL. But because it inherited a lot from debugfs, I
started doing these tricks like walking the dentry tree to make it work a
bit better. Because the dentries and inodes were created before mount, I
had to play these tricks.
But as Linus pointed out, that was the wrong way to do that. The right way
was to use .getattr and .permission callbacks to figure out what the
permissions to the files are.
This has nothing to do with the creation of the files, it's about who has
access to the files that the inodes point to.
This sounds like another topic to bring up at LSFMM ;-) "Can we
standardize pseudo file systems like debugfs and tracefs to act more like
real file systems, and have inodes and dentries act as cache and not be so
coupled to the data?"
-- Steve
On Mon, Jan 08, 2024 at 10:23:31AM -0500, Steven Rostedt wrote:
> On Mon, 8 Jan 2024 12:04:54 +0100
> Christian Brauner <[email protected]> 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 completely directory layout is determined. You're just
> > splicing in dentries and inodes during lookup and readdir.
> >
> > If mkdir /sys/kernel/tracing/instances/foo has succeeded and you later
> > do a lookup/readdir on
> >
> > ls -al /sys/kernel/tracing/instances/foo/events
> >
> > Why should the creation of the dentries and inodes ever fail due to a
> > permission failure?
>
> They shouldn't.
>
> > The vfs did already verify that you had the required
> > permissions to list entries in that directory. Why should filling up
> > /sys/kernel/tracing/instances/foo/events ever fail then? It shouldn't
> > That tracefs instance would be half-functional. And again, right now
> > that inode_permission() check cannot even fail.
>
> And it shouldn't. But without dentries and inodes, how does VFS know what
> is allowed to open the files?
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
/sys/kernel/tracing/instances/foo/events/* and below precisely because
ownership is always inherited from the parent dentry and recorded in the
metadata struct eventfs_inode.
So say someone does:
open("/sys/kernel/tracing/instances/foo/events/xfs");
and say this is the first time that someone accesses that events/
directory.
When the open pathwalk is done, the vfs will determine via
[1] may_lookup(inode_of(events))
whether you are able to list entries such as "xfs" in that directory.
The vfs checks inode_permission(MAY_EXEC) on "events" and if that holds
it ends up calling i_op->eventfs_root_lookup(events).
At this point tracefs/eventfs adds the inodes for all entries in that
"events" directory including "xfs" based on the metadata it recorded
during the mkdir. Since now someone is actually interested in them. And
it initializes the inodes with ownership and everything and adds the
dentries that belong into that directory.
Nothing here depends on the permissions of the caller. The only
permission that mattered was done in the VFS in [1]. If the caller has
permissions to enter a directory they can lookup and list its contents.
And its contents where determined/fixed etc when mkdir was called.
So we just need to add the required objects into the caches (inode,
dentry) whose addition we intentionally defered until someone actually
needed them.
So, eventfs_root_lookup() now initializes the inodes with the ownership
from the stored metadata or from the parent dentry and splices in inodes
and dentries. No permission checking is needed for this because it is
always a recheck of what the vfs did in [1].
We now return to the vfs and path walk continues to the final component
that you actually want to open which is that "xfs" directory in this
example. We check the permissions on that inode via may_open("xfs") and
we open that directory returning an fd to userspace ultimately.
(I'm going by memory since I need to step out the door.)
On Wed, 10 Jan 2024 12:45:36 +0100
Christian Brauner <[email protected]> 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
> /sys/kernel/tracing/instances/foo/events/* and below precisely because
> ownership is always inherited from the parent dentry and recorded in the
> metadata struct eventfs_inode.
>
> So say someone does:
>
> open("/sys/kernel/tracing/instances/foo/events/xfs");
>
> and say this is the first time that someone accesses that events/
> directory.
>
> When the open pathwalk is done, the vfs will determine via
>
> [1] may_lookup(inode_of(events))
>
> whether you are able to list entries such as "xfs" in that directory.
> The vfs checks inode_permission(MAY_EXEC) on "events" and if that holds
> it ends up calling i_op->eventfs_root_lookup(events).
>
> At this point tracefs/eventfs adds the inodes for all entries in that
> "events" directory including "xfs" based on the metadata it recorded
> during the mkdir. Since now someone is actually interested in them. And
> it initializes the inodes with ownership and everything and adds the
> dentries that belong into that directory.
>
> Nothing here depends on the permissions of the caller. The only
> permission that mattered was done in the VFS in [1]. If the caller has
> permissions to enter a directory they can lookup and list its contents.
> And its contents where determined/fixed etc when mkdir was called.
>
> So we just need to add the required objects into the caches (inode,
> dentry) whose addition we intentionally defered until someone actually
> needed them.
>
> So, eventfs_root_lookup() now initializes the inodes with the ownership
> from the stored metadata or from the parent dentry and splices in inodes
> and dentries. No permission checking is needed for this because it is
> always a recheck of what the vfs did in [1].
>
> We now return to the vfs and path walk continues to the final component
> that you actually want to open which is that "xfs" directory in this
> example. We check the permissions on that inode via may_open("xfs") and
> we open that directory returning an fd to userspace ultimately.
>
> (I'm going by memory since I need to step out the door.)
So, let's say we do:
chgrp -R rostedt /sys/kernel/tracing/
But I don't want rostedt to have access to xfs
chgrp -R root /sys/kernel/tracing/events/xfs
Both actions will create the inodes and dentries of all files and
directories (because of "-R"). But once that is done, the ref counts go to
zero. They stay around until reclaim. But then I open Chrome ;-) and it
reclaims all the dentries and inodes, so we are back to here we were on
boot.
Now as rostedt I do:
ls /sys/kernel/tracing/events/xfs
The VFS layer doesn't know if I have permission to that or not, because all
the inodes and dentries have been freed. It has to call back to eventfs to
find out. Which the eventfs_root_lookup() and eventfs_iterate_shared() will
recreated the inodes with the proper permission.
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.
-- Steve
On Wed, 10 Jan 2024 08:07:46 -0500
Steven Rostedt <[email protected]> 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.
Thanks!
-- Steve
On Wed, 10 Jan 2024 10:52:51 -0500
Steven Rostedt <[email protected]> wrote:
> I'll apply this patch too, as it appears to work with this code.
I meant "appears to work without this code".
-- Steve
On Wed, 10 Jan 2024 10:52:51 -0500
Steven Rostedt <[email protected]> wrote:
> On Wed, 10 Jan 2024 08:07:46 -0500
> Steven Rostedt <[email protected]> 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
On Wed, Jan 10, 2024 at 08:07:46AM -0500, Steven Rostedt wrote:
> On Wed, 10 Jan 2024 12:45:36 +0100
> Christian Brauner <[email protected]> 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
> > /sys/kernel/tracing/instances/foo/events/* and below precisely because
> > ownership is always inherited from the parent dentry and recorded in the
> > metadata struct eventfs_inode.
> >
> > So say someone does:
> >
> > open("/sys/kernel/tracing/instances/foo/events/xfs");
> >
> > and say this is the first time that someone accesses that events/
> > directory.
> >
> > When the open pathwalk is done, the vfs will determine via
> >
> > [1] may_lookup(inode_of(events))
> >
> > whether you are able to list entries such as "xfs" in that directory.
> > The vfs checks inode_permission(MAY_EXEC) on "events" and if that holds
> > it ends up calling i_op->eventfs_root_lookup(events).
> >
> > At this point tracefs/eventfs adds the inodes for all entries in that
> > "events" directory including "xfs" based on the metadata it recorded
> > during the mkdir. Since now someone is actually interested in them. And
> > it initializes the inodes with ownership and everything and adds the
> > dentries that belong into that directory.
> >
> > Nothing here depends on the permissions of the caller. The only
> > permission that mattered was done in the VFS in [1]. If the caller has
> > permissions to enter a directory they can lookup and list its contents.
> > And its contents where determined/fixed etc when mkdir was called.
> >
> > So we just need to add the required objects into the caches (inode,
> > dentry) whose addition we intentionally defered until someone actually
> > needed them.
> >
> > So, eventfs_root_lookup() now initializes the inodes with the ownership
> > from the stored metadata or from the parent dentry and splices in inodes
> > and dentries. No permission checking is needed for this because it is
> > always a recheck of what the vfs did in [1].
> >
> > We now return to the vfs and path walk continues to the final component
> > that you actually want to open which is that "xfs" directory in this
> > example. We check the permissions on that inode via may_open("xfs") and
> > we open that directory returning an fd to userspace ultimately.
> >
> > (I'm going by memory since I need to step out the door.)
>
> So, let's say we do:
>
> chgrp -R rostedt /sys/kernel/tracing/
The rostedt group needs exec permissions and "other" cannot have exec
permissions otherwise you can trivially list the entries even if it's
owned by root:
chmod 750 /sys/kernel/tracing
user1@localhost:~$ ls -aln /sys/kernel/ | grep tracing
drwxr-x--- 6 0 1000 0 Jan 11 18:23 tracing
>
> But I don't want rostedt to have access to xfs
>
> chgrp -R root /sys/kernel/tracing/events/xfs
chmod 750 /sys/kernel/tracing/events/xfs
user1@localhost:~$ ls -aln /sys/kernel/tracing/events/ | grep xfs
drwxr-x--- 601 0 0 0 Jan 11 18:24 xfs
This ensure that if a user is in the group and the group has exec perms
lookup is possible (For root this will usually work because
CAP_DAC_READ_SEARCH overrides the exec requirement.).
>
> Both actions will create the inodes and dentries of all files and
> directories (because of "-R"). But once that is done, the ref counts go to
> zero. They stay around until reclaim. But then I open Chrome ;-) and it
> reclaims all the dentries and inodes, so we are back to here we were on
> boot.
>
> Now as rostedt I do:
>
> ls /sys/kernel/tracing/events/xfs
>
> The VFS layer doesn't know if I have permission to that or not, because all
> the inodes and dentries have been freed. It has to call back to eventfs to
> find out. Which the eventfs_root_lookup() and eventfs_iterate_shared() will
> recreated the inodes with the proper permission.
Very roughly, ignoring most of the complexity of lookup and focussing on
the permission checking:
When a caller looks up an entry in a directory then the VFS will call
inode_permission(MAY_EXEC) on the directory the caller is trying to
perform that lookup in.
If the caller wants to lookup the "events" entry in the "tracing"
directory then the VFS will call inode_permission(MAY_EXEC, "tracing")
and then - assuming it's not in the cache - call into the lookup method
of the filesystem.
After the VFS has determined that the caller has the permissions to
lookup the "events" entry in the "tracing" directory no further
permission checks are needed.
Path lookup now moves on to the next part which is looking up the "xfs"
entry in the "events" directory. So again, VFS calls
inode_permission(MAY_EXEC, "events") finds that caller has the permissions
and then goes on to call into eventfs_root_lookup().
The VFS has already given the caller a dentry for "xfs" and is passing
that down to tracefs/eventfs. So eventfs gets
eventfs_rootfs_lookup(@dir[events], @dentry[xfs], ...)
eventfs now wades through the recorded metadata entries for the
@dir[events] directory and finds a matching metadata entry for "xfs". It
allocates an inode for it and initializes the owner, mode etc based on
the recorded information. The lookup_one_len() call finds the dentry
that was just added by the VFS.
The "xfs" dentry/inode is now in the cache. The VFS moves on. It calls
inode_permission(MAY_EXEC, "xfs"). It sees that based on the mode you
don't have access to that directory. Lookup fails.
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").
The readdir case is similar only that instead of generating an inode for
a single entry in the directory like in lookup, you're generating inodes
and dentries for multiple entries. And you might end up calling
eventfs_start_creating() like 600 times for xfs/ for example. So you end
up calling lookup_one_len() and thus inode_permission() 600 times. And
this inode_permission() check is the same check that the VFS already
performed: inode_permission(MAY_EXEC, "xfs").
You can even break readdir semantics via tracefs right now:
// 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
chown("/sys/kernel/tracing/events/xfs", 0, 0);
// Or just remove exec bit for the group and restrict to owner
chmod("/sys/kernel/tracing/events/xfs", 700);
// Drop caches to force an eventfs_root_lookup() on everything
write("/proc/sys/vm/drop_caches", "3", 1);
// Returns 0 even though directory has a lot of entries and we should be
// able to list them
getdents64(fd, ...);
And the failure is in the inode_permission(MAY_EXEC, "xfs") call in
lookup_one_len() in eventfs_start_creating() which now fails.
But that's not how this is supposed to work. Once you hold an fd to a
directory you can read it's entries even if the permissions change.
There's no DAC read-time checks on the directory after that. But this is
exactly what happens on eventfs. So it's even broken right now imho.
I've tested this:
user1@localhost:~/data/scripts$ ./readdir_fail /sys/kernel/tracing/events/xfs/
inode# file type d_reclen d_off d_name
4861 directory 24 1 .
1027 directory 24 2 ..
So what I meant is something completely untested, possibly ugly, drafted
like the below.
But tbh, I suspect that eventfs_root_lookup() could even simply be
rewritten to reuse the dentry it was provided from the VFS directly
instead of even having to go through lookup_one_len() via
eventfs_start_creating() at all. Then only the ->iterate_shared
implementation needs to call the noperm lookup variant I hastily drafted
below. But idk, I haven't given that part much thought.
fs/namei.c | 46 ++++++++++++++++++++++++++++++++++++++++---
fs/tracefs/inode.c | 2 +-
include/linux/namei.h | 1 +
3 files changed, 45 insertions(+), 4 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 963576e67f62..0bfd87e79c5d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2656,9 +2656,8 @@ int vfs_path_lookup(struct dentry *dentry, struct vfsmount *mnt,
}
EXPORT_SYMBOL(vfs_path_lookup);
-static int lookup_one_common(struct mnt_idmap *idmap,
- const char *name, struct dentry *base, int len,
- struct qstr *this)
+static int lookup_one_common_noperm(const char *name, struct dentry *base,
+ int len, struct qstr *this)
{
this->name = name;
this->len = len;
@@ -2686,6 +2685,19 @@ static int lookup_one_common(struct mnt_idmap *idmap,
return err;
}
+ return 0;
+}
+
+static inline int lookup_one_common(struct mnt_idmap *idmap,
+ const char *name, struct dentry *base, int len,
+ struct qstr *this)
+{
+ int ret;
+
+ ret = lookup_one_common_noperm(name, base, len, this);
+ if (ret)
+ return ret;
+
return inode_permission(idmap, base->d_inode, MAY_EXEC);
}
@@ -2776,6 +2788,34 @@ struct dentry *lookup_one(struct mnt_idmap *idmap, const char *name,
}
EXPORT_SYMBOL(lookup_one);
+/**
+ * lookup_one_noperm - filesystem helper to lookup single pathname component
+ * @name: pathname component to lookup
+ * @base: base directory to lookup from
+ * @len: maximum length @len should be interpreted to
+ *
+ * Note that this routine is purely a helper for filesystem usage and should
+ * not be called by generic code. This performs no permission checking on @base.
+ *
+ * The caller must hold base->i_mutex.
+ */
+struct dentry *lookup_one_noperm(const char *name, struct dentry *base, int len)
+{
+ struct dentry *dentry;
+ struct qstr this;
+ int err;
+
+ WARN_ON_ONCE(!inode_is_locked(base->d_inode));
+
+ err = lookup_one_common_noperm(name, base, len, &this);
+ if (err)
+ return ERR_PTR(err);
+
+ dentry = lookup_dcache(&this, base, 0);
+ return dentry ? dentry : __lookup_slow(&this, base, 0);
+}
+EXPORT_SYMBOL(lookup_one_noperm);
+
/**
* lookup_one_unlocked - filesystem helper to lookup single pathname component
* @idmap: idmap of the mount the lookup is performed from
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index e1b172c0e091..fa04aa6a5ce7 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -524,7 +524,7 @@ struct dentry *eventfs_start_creating(const char *name, struct dentry *parent)
if (unlikely(IS_DEADDIR(parent->d_inode)))
dentry = ERR_PTR(-ENOENT);
else
- dentry = lookup_one_len(name, parent, strlen(name));
+ dentry = lookup_one_noperm(name, parent, strlen(name));
if (!IS_ERR(dentry) && dentry->d_inode) {
dput(dentry);
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 3100371b5e32..a1491da37cbe 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -77,6 +77,7 @@ extern struct dentry *lookup_one_len(const char *, struct dentry *, int);
extern struct dentry *lookup_one_len_unlocked(const char *, struct dentry *, int);
extern struct dentry *lookup_positive_unlocked(const char *, struct dentry *, int);
struct dentry *lookup_one(struct mnt_idmap *, const char *, struct dentry *, int);
+struct dentry *lookup_one_noperm(const char *, struct dentry *, int);
struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap,
const char *name, struct dentry *base,
int len);
--
2.43.0
On Thu, 11 Jan 2024 22:01:32 +0100
Christian Brauner <[email protected]> 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()
> _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/[email protected]/
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:
static int eventfs_permission(struct mnt_idmap *idmap,
struct inode *inode, int mask)
{
set_top_events_ownership(inode);
return generic_permission(idmap, inode, mask);
}
Where the "set_top_events_ownership() is a nop for everything but the
"events" directory.
I guess I could have two ops:
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_dir_inode_operations = {
.lookup = eventfs_root_lookup,
.setattr = eventfs_set_attr,
.getattr = eventfs_get_attr,
};
And use the second one for all dentries below the root, but I figured it's
not that big of a deal if I called the permissions on all. Perhaps I should
do it with two?
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.
-- Steve
On Thu, Jan 11, 2024 at 04:53:19PM -0500, Steven Rostedt wrote:
> On Thu, 11 Jan 2024 22:01:32 +0100
> Christian Brauner <[email protected]> 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()
> > _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/[email protected]/
>
> 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.
On Fri, 12 Jan 2024 09:27:24 +0100
Christian Brauner <[email protected]> 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 <[email protected]> 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()
> > > _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/[email protected]/
> >
> > 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);
> }
Yes I know that, because that's where I knew what to call in the non
"events" dir case.
>
> > 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).
I guess I'm confused to what you are having an issue with. Is it just
that the permission check gets called twice?
>
> 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.
That's because I didn't see how it was related to the way I fixed the
mount=gid issue. Are you only concerned because of the check in
eventfs_start_creating()?
Yes, you posted code that would make things act funny for some code
that I see no real world use case for. Yeah, it may not act "properly"
but I'm not sure that's bad.
Here, I'll paste it back:
> // 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
> chown("/sys/kernel/tracing/events/xfs", 0, 0);
>
> // Or just remove exec bit for the group and restrict to owner
> chmod("/sys/kernel/tracing/events/xfs", 700);
>
> // Drop caches to force an eventfs_root_lookup() on everything
> write("/proc/sys/vm/drop_caches", "3", 1);
This requires opening the directory, then having it's permissions
change, and then immediately dropping the caches.
>
> // Returns 0 even though directory has a lot of entries and we should be
> // able to list them
> getdents64(fd, ...);
And do we care?
Since tracing exposes internal kernel information, perhaps this is a
feature and not a bug. If someone who had access to the tracing system
and you wanted to stop them, if they had a directory open that they no
longer have access to, you don't want them to see what's left in the
directory.
In other words, I like the idea that the getends64(fd, ...) will fail!
If there's a file underneath that wasn't change, and the admin thought
that just keeping the top directory permissions off is good enough,
then that attacker having that directory open before the directory had
it's file permissions change is a way to still have access to the files
below it.
>
> And the failure is in the inode_permission(MAY_EXEC, "xfs") call in
> lookup_one_len() in eventfs_start_creating() which now fails.
And I think is a good thing!
Again, tracefs is special. It gives you access and possibly control to
the kernel behavior. I like the fact that as soon as someone loses
permission to a directory, they immediately lose it.
-- Steve
On Fri, 12 Jan 2024 08:53:44 -0500
Steven Rostedt <[email protected]> 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
> > chown("/sys/kernel/tracing/events/xfs", 0, 0);
> >
> > // Or just remove exec bit for the group and restrict to owner
> > chmod("/sys/kernel/tracing/events/xfs", 700);
> >
> > // Drop caches to force an eventfs_root_lookup() on everything
> > write("/proc/sys/vm/drop_caches", "3", 1);
>
> This requires opening the directory, then having it's permissions
> change, and then immediately dropping the caches.
>
> >
> > // Returns 0 even though directory has a lot of entries and we should be
> > // able to list them
> > getdents64(fd, ...);
>
> And do we care?
Hmm, maybe the issue you have is with the inconsistency of the action?
For this to fail, it would require the admin to do both change the
permission and to flush caches. If you don't flush the caches then the
task with the dir open can still read it regardless. If the dentries
were already created.
In that case I'm fine if we change the creation of the dentries to not
check the permission.
But for now, it's just a weird side effect that I don't really see how
it would affect any user's workflow.
-- Steve