Received-SPF: pass (google.com: domain of linux-kernel+bounces-7907-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 21 Dec 2023 13:56:36 +0700
From: Bagas Sanjaya <bagasdotme@gmail.com>
To: Max Kellermann <max.kellermann@ionos.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Tejun Heo <tj@kernel.org>, Zefan Li <lizefan.x@bytedance.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Linux CGroups <cgroups@vger.kernel.org>
Subject: Re: [PATCH v2 1/2] kernel/cgroup: use kernfs_create_dir_ns()
Message-ID: <ZYPhpKOewFkYIDFL@archie.me>
References: <20231208093310.297233-1-max.kellermann@ionos.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="MF3osqI6+GUgm1Na"
Content-Disposition: inline
In-Reply-To: <20231208093310.297233-1-max.kellermann@ionos.com>


--MF3osqI6+GUgm1Na
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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").
>=20
> 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.
>=20
> Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
> Acked-by: Tejun Heo <tj@kernel.org>
> --
> v1 -> v2: 12-digit commit id
> ---
>  kernel/cgroup/cgroup.c | 31 ++++---------------------------
>  1 file changed, 4 insertions(+), 27 deletions(-)
>=20
> 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 =3D {
>  	.seq_show		=3D cgroup_seqfile_show,
>  };
> =20
> -/* 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 =3D { .ia_valid =3D ATTR_UID | ATTR_GID,
> -			       .ia_uid =3D current_fsuid(),
> -			       .ia_gid =3D 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_s=
tate *css, struct cgroup *cgrp,
>  	char name[CGROUP_FILE_NAME_MAX];
>  	struct kernfs_node *kn;
>  	struct lock_class_key *key =3D NULL;
> -	int ret;
> =20
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
>  	key =3D &cft->lockdep_key;
>  #endif
>  	kn =3D __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);
> =20
> -	ret =3D cgroup_kn_set_ugid(kn);
> -	if (ret) {
> -		kernfs_remove(kn);
> -		return ret;
> -	}
> -
>  	if (cft->file_offset) {
>  		struct cgroup_file *cfile =3D (void *)css + cft->file_offset;
> =20
> @@ -5616,7 +5595,9 @@ static struct cgroup *cgroup_create(struct cgroup *=
parent, const char *name,
>  		goto out_cancel_ref;
> =20
>  	/* create the directory */
> -	kn =3D kernfs_create_dir(parent->kn, name, mode, cgrp);
> +	kn =3D kernfs_create_dir_ns(parent->kn, name, mode,
> +				  current_fsuid(), current_fsgid(),
> +				  cgrp, NULL);
>  	if (IS_ERR(kn)) {
>  		ret =3D PTR_ERR(kn);
>  		goto out_stat_exit;
> @@ -5761,10 +5742,6 @@ int cgroup_mkdir(struct kernfs_node *parent_kn, co=
nst char *name, umode_t mode)
>  	 */
>  	kernfs_get(cgrp->kn);
> =20
> -	ret =3D cgroup_kn_set_ugid(cgrp->kn);
> -	if (ret)
> -		goto out_destroy;
> -
>  	ret =3D css_populate_dir(&cgrp->self);
>  	if (ret)
>  		goto out_destroy;

No noticeable regressions with this patch applied.

Tested-by: Bagas Sanjaya <bagasdotme@gmail.com>

--=20
An old man doll... just what I always wanted! - Clara

--MF3osqI6+GUgm1Na
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQSSYQ6Cy7oyFNCHrUH2uYlJVVFOowUCZYPhnwAKCRD2uYlJVVFO
o90vAQDDn55gsGPiaCKeuPg75TssDbT5TrTVA4bncC2D1RsxcAEAxMKDFyns+uYR
wZ6IxDR+1liL6ADVLXNTLi1xVPi5OA8=
=FLCG
-----END PGP SIGNATURE-----

--MF3osqI6+GUgm1Na--