kernel/cgroup: use kernfs_create_dir_ns()

Handling of S_ISGID is usually done by inode_init_owner() in all other filesystems, but kernfs doesn't use that function. In kernfs, struct kernfs_node is the primary data structure, and struct inode is only created from it on demand. Therefore, inode_init_owner() can't be used and we need to imitate its behavior.

S_ISGID support is useful for the cgroup filesystem; it allows subtrees managed by an unprivileged process to retain a certain owner gid, which then enables sharing access to the subtree with another unprivileged process.

Signed-off-by Max Kellermann <[email protected]>

Acked-by Tejun Heo <[email protected]>

--
v1 -> v2: minor coding style fix (comment)

---
 fs/kernfs/dir.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

fs/kernfs/dir.c +5 -0

index 8b2bd65d70e7..62d39ecf0a46 100644
@@ -676,6 +676,18 @@ struct kernfs_node *kernfs_new_node(struct kernfs_node *parent,
 {
 	struct kernfs_node *kn;
 
+	if (parent->mode & S_ISGID) {
+		/* this code block imitates inode_init_owner() for
+		 * kernfs
+		 */
+

 kernel/cgroup/cgroup.c | 31 ++++---------------------------
 1 file changed, 4 insertions(+), 27 deletions(-)

By passing the fsugid to kernfs_create_dir_ns(), we don't need cgroup_kn_set_ugid() any longer. That function was added for exactly this purpose by commit 49957f8e2a43 ("cgroup: newly created dirs and files should be owned by the creator").

Eliminating this piece of duplicate code means we benefit from future improvements to kernfs_create_dir_ns(); for example, both are lacking S_ISGID support currently, which my next patch will add to kernfs_create_dir_ns(). It cannot (easily) be added to cgroup_kn_set_ugid() because we can't dereference struct kernfs_iattrs from there.

Signed-off-by Max Kellermann <[email protected]>

Acked-by Tejun Heo <[email protected]>

--
v1 -> v2: 12-digit commit id

---
 kernel/cgroup/cgroup.c | 31 ++++---------------------------
 1 file changed, 4 insertions(+), 27 deletions(-)

kernel/cgroup/cgroup.c +4 -23

index 4b9ff41ca603..a844b421fd83 100644
@@ -4169,20 +4169,6 @@ static struct kernfs_ops cgroup_kf_ops = {
 	.seq_show		= cgroup_seqfile_show,
 };
 
-/* set uid and gid of cgroup dirs and files to that of the creator */
-static int cgroup_kn_set_ugid(struct kernfs_node *kn)
-{
-	struct iattr iattr = { .ia_valid = ATTR_UID | ATTR_GID,
-			       .ia_uid = current_fsuid(),
-			       .ia_gid = current_fsgid(), };
-
-	if (uid_eq(iattr.ia_uid, GLOBAL_ROOT_UID) &&
-	    gid_eq(iattr.ia_gid, GLOBAL_ROOT_GID))
-		return 0;
-
-	return kernfs_setattr(kn, &iattr);
-}
-
 static void cgroup_file_notify_timer(struct timer_list *timer)
 {
 	cgroup_file_notify(container_of(timer, struct cgroup_file,

@@ -4195,25 +4181,18 @@ static int cgroup_add_file(struct cgroup_subsys_state *css, struct cgroup *cgrp,
 	char name[CGROUP_FILE_NAME_MAX];
 	struct kernfs_node *kn;
 	struct lock_class_key *key = NULL;
-	int ret;
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 	key = &cft->lockdep_key;
 #endif
 	kn = __kernfs_create_file(cgrp->kn, cgroup_file_name(cgrp, cft, name),
 				  cgroup_file_mode(cft),
-				  GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
+				  current_fsuid(), current_fsgid(),
 				  0, cft->kf_ops, cft,
 				  NULL, key);
 	if (IS_ERR(kn))
 		return PTR_ERR(kn);
 
-	ret = cgroup_kn_set_ugid(kn);
-	if (ret) {
-		kernfs_remove(kn);
-		return ret;
-	}
-
 	if (cft->file_offset) {
 		struct cgroup_file *cfile = (void *)css + cft->file_offset;
 

@@ -5616,7 +5595,9 @@ static struct cgroup *cgroup_create(struct cgroup *parent, const char *name,
 		goto out_cancel_ref;
 
 	/* create the directory */
-	kn = kernfs_create_dir(parent->kn, name, mode, cgrp);
+	kn = kernfs_create_dir_ns(parent->kn, name, mode,
+				  current_fsuid(), current_fsgid(),
+				  cgrp, NULL);
 	if (IS_ERR(kn)) {
 		ret = PTR_ERR(kn);
 		goto out_stat_exit;

By passing the fsugid to kernfs_create_dir_ns(), we don't need cgroup_kn_set_ugid() any longer. That function was added for exactly this purpose by commit 49957f8e2a43 ("cgroup: newly created dirs and files should be owned by the creator").

Eliminating this piece of duplicate code means we benefit from future improvements to kernfs_create_dir_ns(); for example, both are lacking S_ISGID support currently, which my next patch will add to kernfs_create_dir_ns(). It cannot (easily) be added to cgroup_kn_set_ugid() because we can't dereference struct kernfs_iattrs from there.

Signed-off-by Max Kellermann <[email protected]>

Acked-by Tejun Heo <[email protected]>

Quoted 12 lines · Max Kellermann

By passing the fsugid to kernfs_create_dir_ns(), we don't need cgroup_kn_set_ugid() any longer. That function was added for exactly this purpose by commit 49957f8e2a43 ("cgroup: newly created dirs and files should be owned by the creator").

Eliminating this piece of duplicate code means we benefit from future improvements to kernfs_create_dir_ns(); for example, both are lacking S_ISGID support currently, which my next patch will add to kernfs_create_dir_ns(). It cannot (easily) be added to cgroup_kn_set_ugid() because we can't dereference struct kernfs_iattrs from there.

Quoted 2 lines

Signed-off-by Max Kellermann <[email protected]>

Acked-by Tejun Heo <[email protected]>

Applied to cgroup/for-6.8. Greg, can you please take the second patch?