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(-)
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 4b9ff41ca603..a844b421fd83 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -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;
@@ -5761,10 +5742,6 @@ int cgroup_mkdir(struct kernfs_node *parent_kn, const char *name, umode_t mode)
*/
kernfs_get(cgrp->kn);
- ret = cgroup_kn_set_ugid(cgrp->kn);
- if (ret)
- goto out_destroy;
-
ret = css_populate_dir(&cgrp->self);
if (ret)
goto out_destroy;
--
2.39.2
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(+)
diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 8b2bd65d70e7..62d39ecf0a46 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -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
+ */
+
+ if (parent->iattr)
+ gid = parent->iattr->ia_gid;
+
+ if (flags & KERNFS_DIR)
+ mode |= S_ISGID;
+ }
+
kn = __kernfs_new_node(kernfs_root(parent), parent,
name, mode, uid, gid, flags);
if (kn) {
--
2.39.2
On Fri, Dec 08, 2023 at 10:33:09AM +0100, Max Kellermann <[email protected]> wrote:
> kernel/cgroup/cgroup.c | 31 ++++---------------------------
> 1 file changed, 4 insertions(+), 27 deletions(-)
Nice,
Reviewed-by: Michal Koutn? <[email protected]>
On Fri, Dec 08, 2023 at 10:33:09AM +0100, Max Kellermann wrote:
> 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(-)
>
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 4b9ff41ca603..a844b421fd83 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -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;
> @@ -5761,10 +5742,6 @@ int cgroup_mkdir(struct kernfs_node *parent_kn, const char *name, umode_t mode)
> */
> kernfs_get(cgrp->kn);
>
> - ret = cgroup_kn_set_ugid(cgrp->kn);
> - if (ret)
> - goto out_destroy;
> -
> ret = css_populate_dir(&cgrp->self);
> if (ret)
> goto out_destroy;
No noticeable regressions with this patch applied.
Tested-by: Bagas Sanjaya <[email protected]>
--
An old man doll... just what I always wanted! - Clara
On Fri, Dec 08, 2023 at 10:33:10AM +0100, Max Kellermann wrote:
> 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(+)
>
> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index 8b2bd65d70e7..62d39ecf0a46 100644
> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c
> @@ -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
> + */
> +
> + if (parent->iattr)
> + gid = parent->iattr->ia_gid;
> +
> + if (flags & KERNFS_DIR)
> + mode |= S_ISGID;
> + }
> +
> kn = __kernfs_new_node(kernfs_root(parent), parent,
> name, mode, uid, gid, flags);
> if (kn) {
No noticeable regressions with this patch applied.
Tested-by: Bagas Sanjaya <[email protected]>
--
An old man doll... just what I always wanted! - Clara
On Fri, Dec 08, 2023 at 10:33:09AM +0100, Max Kellermann wrote:
> 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]>
Applied to cgroup/for-6.8. Greg, can you please take the second patch?
Thanks.
--
tejun
On Fri, Dec 22, 2023 at 06:29:43AM +0900, Tejun Heo wrote:
> On Fri, Dec 08, 2023 at 10:33:09AM +0100, Max Kellermann wrote:
> > 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]>
>
> Applied to cgroup/for-6.8. Greg, can you please take the second patch?
Both are already in my tree, thanks.
greg k-h