[LTP] [PATCH v1 1/3] tst_safe_file_at: Add SAFE_FCHOWNAT macro

[LTP] [PATCH v1 1/3] tst_safe_file_at: Add SAFE_FCHOWNAT macro

[Yang Xu]

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 include/tst_safe_file_at.h |  9 +++++++++
 lib/tst_safe_file_at.c     | 20 ++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/include/tst_safe_file_at.h b/include/tst_safe_file_at.h
index 8df34227f..e253198e6 100644
--- a/include/tst_safe_file_at.h
+++ b/include/tst_safe_file_at.h
@@ -25,6 +25,10 @@
 #define SAFE_UNLINKAT(dirfd, path, flags)				\
 	safe_unlinkat(__FILE__, __LINE__, (dirfd), (path), (flags))
 
+#define SAFE_FCHOWNAT(dirfd, path, owner, group, flags)			\
+	safe_fchownat(__FILE__, __LINE__,				\
+			(dirfd), (path), (owner), (group), (flags))
+
 const char *tst_decode_fd(const int fd)
 			  __attribute__((warn_unused_result));
 
@@ -58,4 +62,9 @@ int safe_unlinkat(const char *const file, const int lineno,
 		  const int dirfd, const char *const path, const int flags)
 		  __attribute__ ((nonnull));
 
+int safe_fchownat(const char *const file, const int lineno,
+		  const int dirfd, const char *const path, uid_t owner,
+		  gid_t group, int flags)
+		  __attribute__ ((nonnull));
+
 #endif
diff --git a/lib/tst_safe_file_at.c b/lib/tst_safe_file_at.c
index ca8ef2f68..6370a68e5 100644
--- a/lib/tst_safe_file_at.c
+++ b/lib/tst_safe_file_at.c
@@ -195,3 +195,23 @@ int safe_unlinkat(const char *const file, const int lineno,
 
 	return rval;
 }
+
+int safe_fchownat(const char *const file, const int lineno,
+		  const int dirfd, const char *const path, uid_t owner, gid_t group, int flags)
+{
+	int rval;
+
+	rval = fchownat(dirfd, path, owner, group, flags);
+
+	if (rval == -1) {
+		tst_brk_(file, lineno, TBROK | TERRNO,
+			 "fchownat(%d<%s>, '%s', %d, %d, %d) failed", dirfd,
+			 tst_decode_fd(dirfd), path, owner, group, flags);
+	} else if (rval) {
+		tst_brk_(file, lineno, TBROK | TERRNO,
+			 "Invalid fchownat(%d<%s>, '%s', %d, %d, %d) return value %d",
+			 dirfd, tst_decode_fd(dirfd), path, owner, group, flags, rval);
+	}
+
+	return rval;
+}
-- 
2.27.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v1 2/3] tst_cgroup: Add safe_cg_open and safe_cg_fchown functions

[Yang Xu]

safe_cg_open is used to open the sub control's file ie cgroup.procs
and returns the fd.

safe_cg_fchown is used to use fchownat to change file's uid and gid.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 include/tst_cgroup.h | 15 +++++++++++++++
 lib/tst_cgroup.c     | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
index d06847cc6..292c9baa4 100644
--- a/include/tst_cgroup.h
+++ b/include/tst_cgroup.h
@@ -188,6 +188,21 @@ ssize_t safe_cg_read(const char *const file, const int lineno,
 			 char *const out, const size_t len)
 			 __attribute__ ((nonnull));
 
+#define SAFE_CG_OPEN(cg, file_name, flags)			\
+	safe_cg_open(__FILE__, __LINE__, (cg), (file_name), (flags))
+
+int safe_cg_open(const char *const file, const int lineno,
+		 const struct tst_cg_group *const cg,
+		 const char *const file_name, int flags);
+
+#define SAFE_CG_FCHOWN(cg, file_name, owner, group)		\
+	safe_cg_fchown(__FILE__, __LINE__,			\
+			   (cg), (file_name), (owner), (group))
+
+void safe_cg_fchown(const char *const file, const int lineno,
+		    const struct tst_cg_group *const cg,
+		    const char *const file_name, uid_t owner, gid_t group);
+
 #define SAFE_CG_PRINTF(cg, file_name, fmt, ...)			\
 	safe_cg_printf(__FILE__, __LINE__,				\
 			   (cg), (file_name), (fmt), __VA_ARGS__)
diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index 1cfd79243..dedc0f65b 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -1297,6 +1297,45 @@ ssize_t safe_cg_read(const char *const file, const int lineno,
 	return read_ret;
 }
 
+int safe_cg_open(const char *const file, const int lineno,
+			const struct tst_cg_group *cg,
+			const char *const file_name, int flags)
+{
+	const struct cgroup_file *const cfile =
+		cgroup_file_find(file, lineno, file_name);
+	struct cgroup_dir *const *dir;
+	const char *alias;
+	int fd;
+
+	for_each_dir(cg, cfile->ctrl_indx, dir) {
+		alias = cgroup_file_alias(cfile, *dir);
+		if (!alias)
+			continue;
+
+		fd = safe_openat(file, lineno, (*dir)->dir_fd, alias, flags);
+	}
+
+	return fd;
+}
+
+void safe_cg_fchown(const char *const file, const int lineno,
+			const struct tst_cg_group *cg,
+			const char *const file_name, uid_t owner, gid_t group)
+{
+	const struct cgroup_file *const cfile =
+		cgroup_file_find(file, lineno, file_name);
+	struct cgroup_dir *const *dir;
+	const char *alias;
+
+	for_each_dir(cg, cfile->ctrl_indx, dir) {
+		alias = cgroup_file_alias(cfile, *dir);
+		if (!alias)
+			continue;
+
+		safe_fchownat(file, lineno, (*dir)->dir_fd, alias, owner, group, 0);
+	}
+}
+
 void safe_cg_printf(const char *const file, const int lineno,
 			const struct tst_cg_group *cg,
 			const char *const file_name,
-- 
2.27.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v1 3/3] memcontrol05: copy from kernel selftest test_cgcore_lesser_euid_open

[Yang Xu]

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
TODO: In the future, also add a regression test in cgroup namespace[1]
if I have free time. Or, I will add it on v2.
[1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bf35a787
 runtest/controllers                           |  1 +
 testcases/kernel/controllers/memcg/.gitignore |  1 +
 .../kernel/controllers/memcg/memcontrol05.c   | 90 +++++++++++++++++++
 3 files changed, 92 insertions(+)
 create mode 100644 testcases/kernel/controllers/memcg/memcontrol05.c

diff --git a/runtest/controllers b/runtest/controllers
index 22d482050..5c51a414a 100644
--- a/runtest/controllers
+++ b/runtest/controllers
@@ -21,6 +21,7 @@ memcontrol01 memcontrol01
 memcontrol02 memcontrol02
 memcontrol03 memcontrol03
 memcontrol04 memcontrol04
+memcontrol05 memcontrol05
 
 cgroup_fj_function_debug cgroup_fj_function.sh debug
 cgroup_fj_function_cpuset cgroup_fj_function.sh cpuset
diff --git a/testcases/kernel/controllers/memcg/.gitignore b/testcases/kernel/controllers/memcg/.gitignore
index 3883cede6..8b9f6005c 100644
--- a/testcases/kernel/controllers/memcg/.gitignore
+++ b/testcases/kernel/controllers/memcg/.gitignore
@@ -9,3 +9,4 @@ memcontrol01
 memcontrol02
 memcontrol03
 memcontrol04
+memcontrol05
diff --git a/testcases/kernel/controllers/memcg/memcontrol05.c b/testcases/kernel/controllers/memcg/memcontrol05.c
new file mode 100644
index 000000000..24976b602
--- /dev/null
+++ b/testcases/kernel/controllers/memcg/memcontrol05.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved.
+ * Author: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * When a task is writing to an fd opened by a different task, the perm check
+ * should use the credentials of the latter task.
+ *
+ * It is copy from kernel selftests cgroup test_core test_cgcore_lesser_euid_open
+ * subcase. The difference is that kernel selftest only supports cgroup v2 but
+ * here we also support cgroup v1 and v2.
+ *
+ * It is a regression test for
+ *
+ * commit e57457641613fef0d147ede8bd6a3047df588b95
+ * Author: Tejun Heo <tj@kernel.org>
+ * Date:   Thu Jan 6 11:02:29 2022 -1000
+ *
+ * cgroup: Use open-time cgroup namespace for process migration perm checks
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <sys/types.h>
+#include <pwd.h>
+#include "tst_test.h"
+#include "tst_safe_file_at.h"
+
+static struct tst_cg_group *cg_child;
+static uid_t nobody_uid, save_uid;
+
+static void test_lesser_euid_open(void)
+{
+	int fd;
+
+	cg_child = tst_cg_group_mk(tst_cg, "child");
+	if (!SAFE_FORK()) {
+		SAFE_CG_FCHOWN(cg_child, "cgroup.procs", nobody_uid, -1);
+		SAFE_SETEUID(nobody_uid);
+
+		fd = SAFE_CG_OPEN(cg_child, "cgroup.procs", O_RDWR);
+		SAFE_SETEUID(save_uid);
+
+		TEST(write(fd, "0", 1));
+		if (TST_RET >= 0 || TST_ERR != EACCES)
+			tst_res(TFAIL, "lesser_euid_open failed");
+		else
+			tst_res(TPASS | TTERRNO, "less_euid_open passed");
+
+		SAFE_CLOSE(fd);
+		exit(0);
+	}
+
+	tst_reap_children();
+	cg_child = tst_cg_group_rm(cg_child);
+}
+
+static void setup(void)
+{
+	struct passwd *pw;
+
+	pw = SAFE_GETPWNAM("nobody");
+	nobody_uid = pw->pw_uid;
+	save_uid = geteuid();
+}
+
+static void cleanup(void)
+{
+	if (cg_child) {
+		SAFE_CG_PRINTF(tst_cg_drain, "cgroup.procs", "%d", getpid());
+		cg_child = tst_cg_group_rm(cg_child);
+	}
+}
+
+static struct tst_test test = {
+	.setup = setup,
+	.cleanup = cleanup,
+	.test_all = test_lesser_euid_open,
+	.forks_child = 1,
+	.needs_root = 1,
+	.needs_cgroup_ctrls = (const char *const[]){ "memory", NULL },
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "e57457641613"},
+		{}
+	},
+};
-- 
2.27.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1 2/3] tst_cgroup: Add safe_cg_open and safe_cg_fchown functions

[Richard Palethorpe]

Hello,

Yang Xu <xuyang2018.jy@fujitsu.com> writes:

> safe_cg_open is used to open the sub control's file ie cgroup.procs
> and returns the fd.
>
> safe_cg_fchown is used to use fchownat to change file's uid and gid.
>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  include/tst_cgroup.h | 15 +++++++++++++++
>  lib/tst_cgroup.c     | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
>
> diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
> index d06847cc6..292c9baa4 100644
> --- a/include/tst_cgroup.h
> +++ b/include/tst_cgroup.h
> @@ -188,6 +188,21 @@ ssize_t safe_cg_read(const char *const file, const int lineno,
>  			 char *const out, const size_t len)
>  			 __attribute__ ((nonnull));
>  
> +#define SAFE_CG_OPEN(cg, file_name, flags)			\
> +	safe_cg_open(__FILE__, __LINE__, (cg), (file_name), (flags))
> +
> +int safe_cg_open(const char *const file, const int lineno,
> +		 const struct tst_cg_group *const cg,
> +		 const char *const file_name, int flags);
> +
> +#define SAFE_CG_FCHOWN(cg, file_name, owner, group)		\
> +	safe_cg_fchown(__FILE__, __LINE__,			\
> +			   (cg), (file_name), (owner), (group))
> +
> +void safe_cg_fchown(const char *const file, const int lineno,
> +		    const struct tst_cg_group *const cg,
> +		    const char *const file_name, uid_t owner, gid_t group);
> +
>  #define SAFE_CG_PRINTF(cg, file_name, fmt, ...)			\
>  	safe_cg_printf(__FILE__, __LINE__,				\
>  			   (cg), (file_name), (fmt), __VA_ARGS__)
> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
> index 1cfd79243..dedc0f65b 100644
> --- a/lib/tst_cgroup.c
> +++ b/lib/tst_cgroup.c
> @@ -1297,6 +1297,45 @@ ssize_t safe_cg_read(const char *const file, const int lineno,
>  	return read_ret;
>  }
>  
> +int safe_cg_open(const char *const file, const int lineno,
> +			const struct tst_cg_group *cg,
> +			const char *const file_name, int flags)
> +{
> +	const struct cgroup_file *const cfile =
> +		cgroup_file_find(file, lineno, file_name);
> +	struct cgroup_dir *const *dir;
> +	const char *alias;
> +	int fd;
> +
> +	for_each_dir(cg, cfile->ctrl_indx, dir) {
> +		alias = cgroup_file_alias(cfile, *dir);
> +		if (!alias)
> +			continue;
> +
> +		fd = safe_openat(file, lineno, (*dir)->dir_fd, alias,
> flags);

This will only return the last fd that gets opened. That's OK if the
file only exists on a single V1 controller, but if it exists on multiple
controllers (e.g. any cgroup.* file) then we will open multiple files
and only return the fd for the last of them.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1 2/3] tst_cgroup: Add safe_cg_open and safe_cg_fchown functions

[xuyang2018.jy]

Hi Richard
> Hello,
> 
> Yang Xu <xuyang2018.jy@fujitsu.com> writes:
> 
>> safe_cg_open is used to open the sub control's file ie cgroup.procs
>> and returns the fd.
>>
>> safe_cg_fchown is used to use fchownat to change file's uid and gid.
>>
>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>> ---
>>   include/tst_cgroup.h | 15 +++++++++++++++
>>   lib/tst_cgroup.c     | 39 +++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 54 insertions(+)
>>
>> diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
>> index d06847cc6..292c9baa4 100644
>> --- a/include/tst_cgroup.h
>> +++ b/include/tst_cgroup.h
>> @@ -188,6 +188,21 @@ ssize_t safe_cg_read(const char *const file, const int lineno,
>>   			 char *const out, const size_t len)
>>   			 __attribute__ ((nonnull));
>>   
>> +#define SAFE_CG_OPEN(cg, file_name, flags)			\
>> +	safe_cg_open(__FILE__, __LINE__, (cg), (file_name), (flags))
>> +
>> +int safe_cg_open(const char *const file, const int lineno,
>> +		 const struct tst_cg_group *const cg,
>> +		 const char *const file_name, int flags);
>> +
>> +#define SAFE_CG_FCHOWN(cg, file_name, owner, group)		\
>> +	safe_cg_fchown(__FILE__, __LINE__,			\
>> +			   (cg), (file_name), (owner), (group))
>> +
>> +void safe_cg_fchown(const char *const file, const int lineno,
>> +		    const struct tst_cg_group *const cg,
>> +		    const char *const file_name, uid_t owner, gid_t group);
>> +
>>   #define SAFE_CG_PRINTF(cg, file_name, fmt, ...)			\
>>   	safe_cg_printf(__FILE__, __LINE__,				\
>>   			   (cg), (file_name), (fmt), __VA_ARGS__)
>> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
>> index 1cfd79243..dedc0f65b 100644
>> --- a/lib/tst_cgroup.c
>> +++ b/lib/tst_cgroup.c
>> @@ -1297,6 +1297,45 @@ ssize_t safe_cg_read(const char *const file, const int lineno,
>>   	return read_ret;
>>   }
>>   
>> +int safe_cg_open(const char *const file, const int lineno,
>> +			const struct tst_cg_group *cg,
>> +			const char *const file_name, int flags)
>> +{
>> +	const struct cgroup_file *const cfile =
>> +		cgroup_file_find(file, lineno, file_name);
>> +	struct cgroup_dir *const *dir;
>> +	const char *alias;
>> +	int fd;
>> +
>> +	for_each_dir(cg, cfile->ctrl_indx, dir) {
>> +		alias = cgroup_file_alias(cfile, *dir);
>> +		if (!alias)
>> +			continue;
>> +
>> +		fd = safe_openat(file, lineno, (*dir)->dir_fd, alias,
>> flags);
> 
> This will only return the last fd that gets opened. That's OK if the
> file only exists on a single V1 controller, but if it exists on multiple
> controllers (e.g. any cgroup.* file) then we will open multiple files
> and only return the fd for the last of them.

Sorry for the late reply. I just copy these code from safe_cg_printf.
So safe_cg_printf have the same situation that write value to muliple 
files under the created cgroup directory.

So what should we do? Add a fd arrat member in  cgroup_file  struct?

Best Regards
Yang Xu

> 

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1 2/3] tst_cgroup: Add safe_cg_open and safe_cg_fchown functions

[Richard Palethorpe]

Hello,

"xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com> writes:

> Hi Richard
>> Hello,
>> 
>> Yang Xu <xuyang2018.jy@fujitsu.com> writes:
>> 
>>> safe_cg_open is used to open the sub control's file ie cgroup.procs
>>> and returns the fd.
>>>
>>> safe_cg_fchown is used to use fchownat to change file's uid and gid.
>>>
>>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>>> ---
>>>   include/tst_cgroup.h | 15 +++++++++++++++
>>>   lib/tst_cgroup.c     | 39 +++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 54 insertions(+)
>>>
>>> diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
>>> index d06847cc6..292c9baa4 100644
>>> --- a/include/tst_cgroup.h
>>> +++ b/include/tst_cgroup.h
>>> @@ -188,6 +188,21 @@ ssize_t safe_cg_read(const char *const file, const int lineno,
>>>   			 char *const out, const size_t len)
>>>   			 __attribute__ ((nonnull));
>>>   
>>> +#define SAFE_CG_OPEN(cg, file_name, flags)			\
>>> +	safe_cg_open(__FILE__, __LINE__, (cg), (file_name), (flags))
>>> +
>>> +int safe_cg_open(const char *const file, const int lineno,
>>> +		 const struct tst_cg_group *const cg,
>>> +		 const char *const file_name, int flags);
>>> +
>>> +#define SAFE_CG_FCHOWN(cg, file_name, owner, group)		\
>>> +	safe_cg_fchown(__FILE__, __LINE__,			\
>>> +			   (cg), (file_name), (owner), (group))
>>> +
>>> +void safe_cg_fchown(const char *const file, const int lineno,
>>> +		    const struct tst_cg_group *const cg,
>>> +		    const char *const file_name, uid_t owner, gid_t group);
>>> +
>>>   #define SAFE_CG_PRINTF(cg, file_name, fmt, ...)			\
>>>   	safe_cg_printf(__FILE__, __LINE__,				\
>>>   			   (cg), (file_name), (fmt), __VA_ARGS__)
>>> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
>>> index 1cfd79243..dedc0f65b 100644
>>> --- a/lib/tst_cgroup.c
>>> +++ b/lib/tst_cgroup.c
>>> @@ -1297,6 +1297,45 @@ ssize_t safe_cg_read(const char *const file, const int lineno,
>>>   	return read_ret;
>>>   }
>>>   
>>> +int safe_cg_open(const char *const file, const int lineno,
>>> +			const struct tst_cg_group *cg,
>>> +			const char *const file_name, int flags)
>>> +{
>>> +	const struct cgroup_file *const cfile =
>>> +		cgroup_file_find(file, lineno, file_name);
>>> +	struct cgroup_dir *const *dir;
>>> +	const char *alias;
>>> +	int fd;
>>> +
>>> +	for_each_dir(cg, cfile->ctrl_indx, dir) {
>>> +		alias = cgroup_file_alias(cfile, *dir);
>>> +		if (!alias)
>>> +			continue;
>>> +
>>> +		fd = safe_openat(file, lineno, (*dir)->dir_fd, alias,
>>> flags);
>> 
>> This will only return the last fd that gets opened. That's OK if the
>> file only exists on a single V1 controller, but if it exists on multiple
>> controllers (e.g. any cgroup.* file) then we will open multiple files
>> and only return the fd for the last of them.
>
> Sorry for the late reply. I just copy these code from safe_cg_printf.
> So safe_cg_printf have the same situation that write value to muliple 
> files under the created cgroup directory.
>
> So what should we do? Add a fd arrat member in  cgroup_file  struct?

I'm not sure what you mean, but I can see a few options.

1. Pass a pointer to a function which takes the fd, for e.g.

safe_cg_open(..., void (*const on_open)(int fd))
{
        ...
        for_each_dir(...) {
            ...
            fd = safe_openat(...);
            on_open(fd);
            close(fd);
        }
        ...
}

2. Allocate and return an array of open fd's
3. Pass an array as large as the maximum number controllers which is
   populated with open fd's.

Which option to pick I think depends on what results in the simplest
test code.

>
> Best Regards
> Yang Xu
>
>> 


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1 2/3] tst_cgroup: Add safe_cg_open and safe_cg_fchown functions

[xuyang2018.jy]

Hi Richard

> Hello,
> 
> "xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com> writes:
> 
>> Hi Richard
>>> Hello,
>>>
>>> Yang Xu <xuyang2018.jy@fujitsu.com> writes:
>>>
>>>> safe_cg_open is used to open the sub control's file ie cgroup.procs
>>>> and returns the fd.
>>>>
>>>> safe_cg_fchown is used to use fchownat to change file's uid and gid.
>>>>
>>>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>>>> ---
>>>>    include/tst_cgroup.h | 15 +++++++++++++++
>>>>    lib/tst_cgroup.c     | 39 +++++++++++++++++++++++++++++++++++++++
>>>>    2 files changed, 54 insertions(+)
>>>>
>>>> diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
>>>> index d06847cc6..292c9baa4 100644
>>>> --- a/include/tst_cgroup.h
>>>> +++ b/include/tst_cgroup.h
>>>> @@ -188,6 +188,21 @@ ssize_t safe_cg_read(const char *const file, const int lineno,
>>>>    			 char *const out, const size_t len)
>>>>    			 __attribute__ ((nonnull));
>>>>    
>>>> +#define SAFE_CG_OPEN(cg, file_name, flags)			\
>>>> +	safe_cg_open(__FILE__, __LINE__, (cg), (file_name), (flags))
>>>> +
>>>> +int safe_cg_open(const char *const file, const int lineno,
>>>> +		 const struct tst_cg_group *const cg,
>>>> +		 const char *const file_name, int flags);
>>>> +
>>>> +#define SAFE_CG_FCHOWN(cg, file_name, owner, group)		\
>>>> +	safe_cg_fchown(__FILE__, __LINE__,			\
>>>> +			   (cg), (file_name), (owner), (group))
>>>> +
>>>> +void safe_cg_fchown(const char *const file, const int lineno,
>>>> +		    const struct tst_cg_group *const cg,
>>>> +		    const char *const file_name, uid_t owner, gid_t group);
>>>> +
>>>>    #define SAFE_CG_PRINTF(cg, file_name, fmt, ...)			\
>>>>    	safe_cg_printf(__FILE__, __LINE__,				\
>>>>    			   (cg), (file_name), (fmt), __VA_ARGS__)
>>>> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
>>>> index 1cfd79243..dedc0f65b 100644
>>>> --- a/lib/tst_cgroup.c
>>>> +++ b/lib/tst_cgroup.c
>>>> @@ -1297,6 +1297,45 @@ ssize_t safe_cg_read(const char *const file, const int lineno,
>>>>    	return read_ret;
>>>>    }
>>>>    
>>>> +int safe_cg_open(const char *const file, const int lineno,
>>>> +			const struct tst_cg_group *cg,
>>>> +			const char *const file_name, int flags)
>>>> +{
>>>> +	const struct cgroup_file *const cfile =
>>>> +		cgroup_file_find(file, lineno, file_name);
>>>> +	struct cgroup_dir *const *dir;
>>>> +	const char *alias;
>>>> +	int fd;
>>>> +
>>>> +	for_each_dir(cg, cfile->ctrl_indx, dir) {
>>>> +		alias = cgroup_file_alias(cfile, *dir);
>>>> +		if (!alias)
>>>> +			continue;
>>>> +
>>>> +		fd = safe_openat(file, lineno, (*dir)->dir_fd, alias,
>>>> flags);
>>>
>>> This will only return the last fd that gets opened. That's OK if the
>>> file only exists on a single V1 controller, but if it exists on multiple
>>> controllers (e.g. any cgroup.* file) then we will open multiple files
>>> and only return the fd for the last of them.
>>
>> Sorry for the late reply. I just copy these code from safe_cg_printf.
>> So safe_cg_printf have the same situation that write value to muliple
>> files under the created cgroup directory.
>>
>> So what should we do? Add a fd arrat member in  cgroup_file  struct?
> 
> I'm not sure what you mean, but I can see a few options.
> 
> 1. Pass a pointer to a function which takes the fd, for e.g.
> 
> safe_cg_open(..., void (*const on_open)(int fd))
> {
>          ...
>          for_each_dir(...) {
>              ...
>              fd = safe_openat(...);
>              on_open(fd);
>              close(fd);
>          }
>          ...
> }
> 
> 2. Allocate and return an array of open fd's

This way is also in my mind. But we can also filter them by cgroup 
version and detect the root ctrl name. I have sent a RFC v2 patch.

Best Regards
Yang Xu
> 3. Pass an array as large as the maximum number controllers which is
>     populated with open fd's.
> 
> Which option to pick I think depends on what results in the simplest
> test code.
> 
>>
>> Best Regards
>> Yang Xu
>>
>>>
> 
> 

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [RFC v2 1/3] tst_safe_file_at: Add SAFE_FCHOWNAT macro

[Yang Xu]

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 include/tst_safe_file_at.h |  9 +++++++++
 lib/tst_safe_file_at.c     | 20 ++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/include/tst_safe_file_at.h b/include/tst_safe_file_at.h
index 8df34227f..e253198e6 100644
--- a/include/tst_safe_file_at.h
+++ b/include/tst_safe_file_at.h
@@ -25,6 +25,10 @@
 #define SAFE_UNLINKAT(dirfd, path, flags)				\
 	safe_unlinkat(__FILE__, __LINE__, (dirfd), (path), (flags))
 
+#define SAFE_FCHOWNAT(dirfd, path, owner, group, flags)			\
+	safe_fchownat(__FILE__, __LINE__,				\
+			(dirfd), (path), (owner), (group), (flags))
+
 const char *tst_decode_fd(const int fd)
 			  __attribute__((warn_unused_result));
 
@@ -58,4 +62,9 @@ int safe_unlinkat(const char *const file, const int lineno,
 		  const int dirfd, const char *const path, const int flags)
 		  __attribute__ ((nonnull));
 
+int safe_fchownat(const char *const file, const int lineno,
+		  const int dirfd, const char *const path, uid_t owner,
+		  gid_t group, int flags)
+		  __attribute__ ((nonnull));
+
 #endif
diff --git a/lib/tst_safe_file_at.c b/lib/tst_safe_file_at.c
index ca8ef2f68..6370a68e5 100644
--- a/lib/tst_safe_file_at.c
+++ b/lib/tst_safe_file_at.c
@@ -195,3 +195,23 @@ int safe_unlinkat(const char *const file, const int lineno,
 
 	return rval;
 }
+
+int safe_fchownat(const char *const file, const int lineno,
+		  const int dirfd, const char *const path, uid_t owner, gid_t group, int flags)
+{
+	int rval;
+
+	rval = fchownat(dirfd, path, owner, group, flags);
+
+	if (rval == -1) {
+		tst_brk_(file, lineno, TBROK | TERRNO,
+			 "fchownat(%d<%s>, '%s', %d, %d, %d) failed", dirfd,
+			 tst_decode_fd(dirfd), path, owner, group, flags);
+	} else if (rval) {
+		tst_brk_(file, lineno, TBROK | TERRNO,
+			 "Invalid fchownat(%d<%s>, '%s', %d, %d, %d) return value %d",
+			 dirfd, tst_decode_fd(dirfd), path, owner, group, flags, rval);
+	}
+
+	return rval;
+}
-- 
2.23.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [RFC v2 2/3] tst_cgroup: Add safe_cg_open and safe_cg_fchown functions

[Yang Xu]

safe_cg_open is used to open the sub control's file ie cgroup.procs
and returns the fd.

safe_cg_fchown is used to use fchownat to change file's uid and gid.

To avoid the problem that if the file only exists on a single V1 controller,
but if it exists on multiple controllers (e.g. any cgroup.* file) then we w
ill open multiple filesand only return the fd for the last of them, introduce
the common_cgroup_file function and add ctrl_name agrument.

The common_cgroup_file is used to detect whether this file can exists on
each controller. If the created cg is v2, we don't have this problem. If
the created cg is v1, then we need to use common_cgroup_file. Also need to
compare root cgroup ctrl whether equal to the ctrl_name agrument. So we
know that we are opening a correct controller cgroup* file instead of
last.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 include/tst_cgroup.h |  38 +++++++++++++++
 lib/tst_cgroup.c     | 111 ++++++++++++++++++++++++++++++++++++-------
 2 files changed, 131 insertions(+), 18 deletions(-)

diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
index d06847cc6..e75d88254 100644
--- a/include/tst_cgroup.h
+++ b/include/tst_cgroup.h
@@ -82,6 +82,7 @@
 #define TST_CGROUP_H
 
 #include <sys/types.h>
+#include <stdbool.h>
 
 /* CGroups Kernel API version */
 enum tst_cg_ver {
@@ -89,6 +90,25 @@ enum tst_cg_ver {
 	TST_CG_V2 = 2,
 };
 
+/* Controller sub-systems */
+enum cgroup_ctrl_indx {
+	CTRL_MEMORY = 1,
+	CTRL_CPU,
+	CTRL_CPUSET,
+	CTRL_IO,
+	CTRL_PIDS,
+	CTRL_HUGETLB,
+	CTRL_CPUACCT,
+	CTRL_DEVICES,
+	CTRL_FREEZER,
+	CTRL_NETCLS,
+	CTRL_NETPRIO,
+	CTRL_BLKIO,
+	CTRL_MISC,
+	CTRL_PERFEVENT,
+	CTRL_DEBUG
+};
+
 /* Used to specify CGroup hierarchy configuration options, allowing a
  * test to request a particular CGroup structure.
  */
@@ -188,6 +208,24 @@ ssize_t safe_cg_read(const char *const file, const int lineno,
 			 char *const out, const size_t len)
 			 __attribute__ ((nonnull));
 
+#define SAFE_CG_OPEN(cg, file_name, ctrl_indx, flags)		\
+	safe_cg_open(__FILE__, __LINE__,				\
+			(cg), (file_name), (ctrl_indx), (flags))
+
+int safe_cg_open(const char *const file, const int lineno,
+		    const struct tst_cg_group *const cg,
+		    const char *const file_name,
+		    enum cgroup_ctrl_indx ctrl_indx, int flags);
+
+#define SAFE_CG_FCHOWN(cg, file_name, ctrl_indx, owner, group)	\
+	safe_cg_fchown(__FILE__, __LINE__,				\
+			   (cg), (file_name), (ctrl_indx), (owner), (group))
+
+void safe_cg_fchown(const char *const file, const int lineno,
+		    const struct tst_cg_group *const cg,
+		    const char *const file_name,
+		    enum cgroup_ctrl_indx ctrl_indx, uid_t owner, gid_t group);
+
 #define SAFE_CG_PRINTF(cg, file_name, fmt, ...)			\
 	safe_cg_printf(__FILE__, __LINE__,				\
 			   (cg), (file_name), (fmt), __VA_ARGS__)
diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index 1cfd79243..946b945f0 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -77,24 +77,6 @@ struct cgroup_root {
 	int no_cpuset_prefix:1;
 };
 
-/* Controller sub-systems */
-enum cgroup_ctrl_indx {
-	CTRL_MEMORY = 1,
-	CTRL_CPU,
-	CTRL_CPUSET,
-	CTRL_IO,
-	CTRL_PIDS,
-	CTRL_HUGETLB,
-	CTRL_CPUACCT,
-	CTRL_DEVICES,
-	CTRL_FREEZER,
-	CTRL_NETCLS,
-	CTRL_NETPRIO,
-	CTRL_BLKIO,
-	CTRL_MISC,
-	CTRL_PERFEVENT,
-	CTRL_DEBUG
-};
 #define CTRLS_MAX CTRL_DEBUG
 
 /* At most we can have one cgroup V1 tree for each controller and one
@@ -1192,6 +1174,25 @@ static const char *cgroup_file_alias(const struct cgroup_file *const cfile,
 	return cfile->file_name_v1;
 }
 
+__attribute__ ((nonnull, warn_unused_result))
+static bool common_cgroup_file(const char *alias,
+			       const struct cgroup_dir *const dir)
+{
+	int i;
+
+	if (dir->dir_root->ver != TST_CG_V1) {
+		for (i = 0; cgroup_ctrl_files[i].file_name; i++)
+			if (!strcmp(cgroup_ctrl_files[i].file_name, alias))
+				return true;
+	} else {
+		for (i = 0; cgroup_ctrl_files[i].file_name_v1; i++)
+			if (!strcmp(cgroup_ctrl_files[i].file_name_v1, alias))
+				return true;
+	}
+
+	return false;
+}
+
 int safe_cg_has(const char *const file, const int lineno,
 		    const struct tst_cg_group *cg,
 		    const char *const file_name)
@@ -1297,6 +1298,80 @@ ssize_t safe_cg_read(const char *const file, const int lineno,
 	return read_ret;
 }
 
+int safe_cg_open(const char *const file, const int lineno,
+			const struct tst_cg_group *cg,
+			const char *const file_name,
+			enum cgroup_ctrl_indx ctrl_indx, int flags)
+{
+	const struct cgroup_file *const cfile =
+		cgroup_file_find(file, lineno, file_name);
+	struct cgroup_dir *const *dir;
+	const char *alias;
+	int fd, dismatch_ctrl = 0;
+
+	for_each_dir(cg, cfile->ctrl_indx, dir) {
+		alias = cgroup_file_alias(cfile, *dir);
+		if (!alias)
+			continue;
+
+		/*
+		 * For the file that exists on multiple v1 controllers (e.g. any cgroup.* file)
+		 * we need to ensure the v1 controller is belong to the specified controller.
+		 */
+		if ((*dir)->dir_root->ver == TST_CG_V1 && common_cgroup_file(alias, *dir)) {
+			if ((*dir)->ctrl_field == (uint32_t)1 << ctrl_indx) {
+				fd = safe_openat(file, lineno, (*dir)->dir_fd, alias, flags);
+				return fd;
+			}
+			dismatch_ctrl = (*dir)->ctrl_field;
+		} else {
+			fd = safe_openat(file, lineno, (*dir)->dir_fd, alias, flags);
+			return fd;
+		}
+	}
+
+	tst_brk_(file, lineno, TBROK, "%s is not in correct v1 controller expected %d but got %d",
+					file_name, 1 << ctrl_indx, dismatch_ctrl);
+	return -1;
+}
+
+void safe_cg_fchown(const char *const file, const int lineno,
+			const struct tst_cg_group *cg,
+			const char *const file_name,
+			enum cgroup_ctrl_indx ctrl_indx,
+			uid_t owner, gid_t group)
+{
+	const struct cgroup_file *const cfile =
+		cgroup_file_find(file, lineno, file_name);
+	struct cgroup_dir *const *dir;
+	const char *alias;
+	int dismatch_ctrl = 0;
+
+	for_each_dir(cg, cfile->ctrl_indx, dir) {
+		alias = cgroup_file_alias(cfile, *dir);
+		if (!alias)
+			continue;
+
+		/*
+		 * For the file that exists on multiple v1 controllers (e.g. any cgroup.* file)
+		 * we need to ensure the v1 controller is belong to the specified controller.
+		 */
+		if ((*dir)->dir_root->ver == TST_CG_V1 && common_cgroup_file(alias, *dir)) {
+			if ((*dir)->ctrl_field == (uint32_t)1 << ctrl_indx) {
+				safe_fchownat(file, lineno, (*dir)->dir_fd, alias, owner, group, 0);
+				return;
+			}
+			dismatch_ctrl = (*dir)->ctrl_field;
+		} else {
+			safe_fchownat(file, lineno, (*dir)->dir_fd, alias, owner, group, 0);
+			return;
+		}
+	}
+
+	tst_brk_(file, lineno, TBROK, "%s is not in correct v1 controller expected %d but got %d",
+					file_name, 1 << ctrl_indx, dismatch_ctrl);
+}
+
 void safe_cg_printf(const char *const file, const int lineno,
 			const struct tst_cg_group *cg,
 			const char *const file_name,
-- 
2.23.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [RFC v2 3/3] memcontrol05: copy from kernel selftest test_cgcore_lesser_euid_open

[Yang Xu]

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 runtest/controllers                           |  1 +
 testcases/kernel/controllers/memcg/.gitignore |  1 +
 .../kernel/controllers/memcg/memcontrol05.c   | 90 +++++++++++++++++++
 3 files changed, 92 insertions(+)
 create mode 100644 testcases/kernel/controllers/memcg/memcontrol05.c

diff --git a/runtest/controllers b/runtest/controllers
index 22d482050..5c51a414a 100644
--- a/runtest/controllers
+++ b/runtest/controllers
@@ -21,6 +21,7 @@ memcontrol01 memcontrol01
 memcontrol02 memcontrol02
 memcontrol03 memcontrol03
 memcontrol04 memcontrol04
+memcontrol05 memcontrol05
 
 cgroup_fj_function_debug cgroup_fj_function.sh debug
 cgroup_fj_function_cpuset cgroup_fj_function.sh cpuset
diff --git a/testcases/kernel/controllers/memcg/.gitignore b/testcases/kernel/controllers/memcg/.gitignore
index 3883cede6..8b9f6005c 100644
--- a/testcases/kernel/controllers/memcg/.gitignore
+++ b/testcases/kernel/controllers/memcg/.gitignore
@@ -9,3 +9,4 @@ memcontrol01
 memcontrol02
 memcontrol03
 memcontrol04
+memcontrol05
diff --git a/testcases/kernel/controllers/memcg/memcontrol05.c b/testcases/kernel/controllers/memcg/memcontrol05.c
new file mode 100644
index 000000000..6343536ff
--- /dev/null
+++ b/testcases/kernel/controllers/memcg/memcontrol05.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved.
+ * Author: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * When a task is writing to an fd opened by a different task, the perm check
+ * should use the credentials of the latter task.
+ *
+ * It is copy from kernel selftests cgroup test_core test_cgcore_lesser_euid_open
+ * subcase. The difference is that kernel selftest only supports cgroup v2 but
+ * here we also support cgroup v1 and v2.
+ *
+ * It is a regression test for
+ *
+ * commit e57457641613fef0d147ede8bd6a3047df588b95
+ * Author: Tejun Heo <tj@kernel.org>
+ * Date:   Thu Jan 6 11:02:29 2022 -1000
+ *
+ * cgroup: Use open-time cgroup namespace for process migration perm checks
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <sys/types.h>
+#include <pwd.h>
+#include "tst_test.h"
+#include "tst_safe_file_at.h"
+
+static struct tst_cg_group *cg_child;
+static uid_t nobody_uid, save_uid;
+
+static void test_lesser_euid_open(void)
+{
+	int fd;
+
+	cg_child = tst_cg_group_mk(tst_cg, "child");
+	if (!SAFE_FORK()) {
+		SAFE_CG_FCHOWN(cg_child, "cgroup.procs", CTRL_MEMORY, nobody_uid, -1);
+		SAFE_SETEUID(nobody_uid);
+
+		fd = SAFE_CG_OPEN(cg_child, "cgroup.procs", CTRL_MEMORY, O_RDWR);
+		SAFE_SETEUID(save_uid);
+
+		TEST(write(fd, "0", 1));
+		if (TST_RET >= 0 || TST_ERR != EACCES)
+			tst_res(TFAIL, "%s failed", __func__);
+		else
+			tst_res(TPASS | TTERRNO, "%s passed", __func__);
+
+		SAFE_CLOSE(fd);
+		exit(0);
+	}
+
+	tst_reap_children();
+	cg_child = tst_cg_group_rm(cg_child);
+}
+
+static void setup(void)
+{
+	struct passwd *pw;
+
+	pw = SAFE_GETPWNAM("nobody");
+	nobody_uid = pw->pw_uid;
+	save_uid = geteuid();
+}
+
+static void cleanup(void)
+{
+	if (cg_child) {
+		SAFE_CG_PRINTF(tst_cg_drain, "cgroup.procs", "%d", getpid());
+		cg_child = tst_cg_group_rm(cg_child);
+	}
+}
+
+static struct tst_test test = {
+	.setup = setup,
+	.cleanup = cleanup,
+	.test_all = test_lesser_euid_open,
+	.forks_child = 1,
+	.needs_root = 1,
+	.needs_cgroup_ctrls = (const char *const[]){ "memory", NULL},
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "e57457641613"},
+		{}
+	},
+};
-- 
2.23.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1 2/3] tst_cgroup: Add safe_cg_open and safe_cg_fchown functions

[Richard Palethorpe]

Hello,

OK, I see the new patches. However I just realised these tests are part
of test_cgcore. These are not related to memcontrol. They should go in
controllers/cgroup/core01.c.

Let's start at the beginning and look at the original selftest.

static int test_cgcore_lesser_euid_open(const char *root)
{
	const uid_t test_euid = 65534;	/* usually nobody, any !root is fine */
	int ret = KSFT_FAIL;
	char *cg_test_a = NULL, *cg_test_b = NULL;
	char *cg_test_a_procs = NULL, *cg_test_b_procs = NULL;
	int cg_test_b_procs_fd = -1;
	uid_t saved_uid;

	cg_test_a = cg_name(root, "cg_test_a");
	cg_test_b = cg_name(root, "cg_test_b");

	if (!cg_test_a || !cg_test_b)
		goto cleanup;

	cg_test_a_procs = cg_name(cg_test_a, "cgroup.procs");
	cg_test_b_procs = cg_name(cg_test_b, "cgroup.procs");

	if (!cg_test_a_procs || !cg_test_b_procs)
		goto cleanup;

	if (cg_create(cg_test_a) || cg_create(cg_test_b))
		goto cleanup;

So we create two subgroups this translates to

cg_child_a = tst_cg_group_mk(tst_cg, 'a');
cg_child_b = tst_cg_group_mk(tst_cg, 'b');


	if (cg_enter_current(cg_test_a))
		goto cleanup;

This writes "0" to the cgroup.procs file which I guess is a shortcut for
writing the current processes PID. We have to be careful here incase
this behaviour is different on V1. Probably this translates to

SAFE_CG_PRINT(cg_child_a, "cgroup.procs", "0");

It's not clear why the current PID is moved to this CG. It may be to
ensure we have permission to move to a sibling CGroup and to eliminate
other possible reasons for getting EACCES later on.

	if (chown(cg_test_a_procs, test_euid, -1) ||
	    chown(cg_test_b_procs, test_euid, -1))
		goto cleanup;

Then the procs files are chowned to nobody. I see no reason this
function can not be implemented in the same way safe_cg_printf is. We
don't need to check which controller the file belongs to, just chown all
of them.

	saved_uid = geteuid();
	if (seteuid(test_euid))
		goto cleanup;

Then the current procs effective uid is changed. We need to make sure to
set this back before doing cleanup (or fork like you did originally).

	cg_test_b_procs_fd = open(cg_test_b_procs, O_RDWR);

	if (seteuid(saved_uid))
		goto cleanup;

Then the file is opened and the euid set back

	if (cg_test_b_procs_fd < 0)
		goto cleanup;

	if (write(cg_test_b_procs_fd, "0", 1) >= 0 || errno != EACCES)
		goto cleanup;

Then we try to move to CG B and expect to get EACCES (because of the
permissions present when opening the file).

	ret = KSFT_PASS;
...


Probably we want to run this test on any controllers which are
available. Currently the API doesn't support that. We need some way of
specifying that the test will use any available controllers in
tst_cg_require and/or tst_test.

Then we need to handle setting the euid between open and writing which
stops us from using safe_cg_printf.

Probably the API shouldn't try to handle stuff this wierd. Instead we
can create a function like

int n = tst_cg_group_dirfds(int *dirfds)

which copies tst_cgroup_group.dirs[i].dir_fd into dirfds (which can be
statically allocated in the test code if we export CTRLS_MAX).

Then we can do

seteuid(nobody);

for (i = 0; i < n; i++) {
    procfds[i] = openat(dirfds[i], "cgroup.procs")
}

seteuid(saved_euid);

for (i = 0; i < n; i++) {
    ret = write(procfds[i], "0", 1);
    if (ret >= 0)
       tst_res(TFAIL...)
    ...

    close(procfds[i]);
}

and whatever else is required by tests which are doing something unusual
with the CG hierarchy.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1 2/3] tst_cgroup: Add safe_cg_open and safe_cg_fchown functions

[xuyang2018.jy]

Hi Richard

> Hello,
> 
> OK, I see the new patches. However I just realised these tests are part
> of test_cgcore. These are not related to memcontrol. They should go in
> controllers/cgroup/core01.c.
> 
> Let's start at the beginning and look at the original selftest.
> 
> static int test_cgcore_lesser_euid_open(const char *root)
> {
> 	const uid_t test_euid = 65534;	/* usually nobody, any !root is fine */
> 	int ret = KSFT_FAIL;
> 	char *cg_test_a = NULL, *cg_test_b = NULL;
> 	char *cg_test_a_procs = NULL, *cg_test_b_procs = NULL;
> 	int cg_test_b_procs_fd = -1;
> 	uid_t saved_uid;
> 
> 	cg_test_a = cg_name(root, "cg_test_a");
> 	cg_test_b = cg_name(root, "cg_test_b");
> 
> 	if (!cg_test_a || !cg_test_b)
> 		goto cleanup;
> 
> 	cg_test_a_procs = cg_name(cg_test_a, "cgroup.procs");
> 	cg_test_b_procs = cg_name(cg_test_b, "cgroup.procs");
> 
> 	if (!cg_test_a_procs || !cg_test_b_procs)
> 		goto cleanup;
> 
> 	if (cg_create(cg_test_a) || cg_create(cg_test_b))
> 		goto cleanup;
> 
> So we create two subgroups this translates to
> 
> cg_child_a = tst_cg_group_mk(tst_cg, 'a');
> cg_child_b = tst_cg_group_mk(tst_cg, 'b');
> 
> 
> 	if (cg_enter_current(cg_test_a))
> 		goto cleanup;
> 
> This writes "0" to the cgroup.procs file which I guess is a shortcut for
> writing the current processes PID. We have to be careful here incase
> this behaviour is different on V1. Probably this translates to
> 
> SAFE_CG_PRINT(cg_child_a, "cgroup.procs", "0");
> 
> It's not clear why the current PID is moved to this CG. It may be to
> ensure we have permission to move to a sibling CGroup and to eliminate
> other possible reasons for getting EACCES later on.

Yes, it's not clear.

But from kernel commit[1], it just check open fd's perms instead of 
current. So I don't use a and b two cgroup .  Also, it indeed fail on
unfixed kernel  by using only one a cgrpup.

[1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1756d7994 

> 
> 	if (chown(cg_test_a_procs, test_euid, -1) ||
> 	    chown(cg_test_b_procs, test_euid, -1))
> 		goto cleanup;
> 
> Then the procs files are chowned to nobody. I see no reason this
> function can not be implemented in the same way safe_cg_printf is. We
> don't need to check which controller the file belongs to, just chown all
> of them.
> 
> 	saved_uid = geteuid();
> 	if (seteuid(test_euid))
> 		goto cleanup;

Yes, we can chown all cgroup procs files ie SAFE_CG_PRINTF did.

> 
> Then the current procs effective uid is changed. We need to make sure to
> set this back before doing cleanup (or fork like you did originally).
> 
> 	cg_test_b_procs_fd = open(cg_test_b_procs, O_RDWR);
> 
> 	if (seteuid(saved_uid))
> 		goto cleanup;
> 
> Then the file is opened and the euid set back
> 
> 	if (cg_test_b_procs_fd < 0)
> 		goto cleanup;
> 
> 	if (write(cg_test_b_procs_fd, "0", 1) >= 0 || errno != EACCES)
> 		goto cleanup;
> 
> Then we try to move to CG B and expect to get EACCES (because of the
> permissions present when opening the file).
> 
> 	ret = KSFT_PASS;
> ...
> 
> 
> Probably we want to run this test on any controllers which are
> available. Currently the API doesn't support that. We need some way of
> specifying that the test will use any available controllers in
> tst_cg_require and/or tst_test.

I will move this case into cgroup/cg_core01.c, kernel selftest seems use 
memory controller, I guess we can also use memory controller because it 
is most common.
> 
> Then we need to handle setting the euid between open and writing which
> stops us from using safe_cg_printf.
> 
> Probably the API shouldn't try to handle stuff this wierd. Instead we
> can create a function like
> 
> int n = tst_cg_group_dirfds(int *dirfds)
> 
> which copies tst_cgroup_group.dirs[i].dir_fd into dirfds (which can be
> statically allocated in the test code if we export CTRLS_MAX).
> 
> Then we can do
> 
> seteuid(nobody);
> 
> for (i = 0; i < n; i++) {
>      procfds[i] = openat(dirfds[i], "cgroup.procs")
> }
> 
> seteuid(saved_euid);
> 
> for (i = 0; i < n; i++) {
>      ret = write(procfds[i], "0", 1);
>      if (ret >= 0)
>         tst_res(TFAIL...)
>      ...
> 
>      close(procfds[i]);
> }
> 
> and whatever else is required by tests which are doing something unusual
> with the CG hierarchy.

OK. Will  try.

Best Regards
Yang Xu
> 

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1 2/3] tst_cgroup: Add safe_cg_open and safe_cg_fchown functions

[xuyang2018.jy]

Hi Richard

> Hi Richard
> 
>> Hello,
>>
>> OK, I see the new patches. However I just realised these tests are part
>> of test_cgcore. These are not related to memcontrol. They should go in
>> controllers/cgroup/core01.c.
>>
>> Let's start at the beginning and look at the original selftest.
>>
>> static int test_cgcore_lesser_euid_open(const char *root)
>> {
>> 	const uid_t test_euid = 65534;	/* usually nobody, any !root is fine */
>> 	int ret = KSFT_FAIL;
>> 	char *cg_test_a = NULL, *cg_test_b = NULL;
>> 	char *cg_test_a_procs = NULL, *cg_test_b_procs = NULL;
>> 	int cg_test_b_procs_fd = -1;
>> 	uid_t saved_uid;
>>
>> 	cg_test_a = cg_name(root, "cg_test_a");
>> 	cg_test_b = cg_name(root, "cg_test_b");
>>
>> 	if (!cg_test_a || !cg_test_b)
>> 		goto cleanup;
>>
>> 	cg_test_a_procs = cg_name(cg_test_a, "cgroup.procs");
>> 	cg_test_b_procs = cg_name(cg_test_b, "cgroup.procs");
>>
>> 	if (!cg_test_a_procs || !cg_test_b_procs)
>> 		goto cleanup;
>>
>> 	if (cg_create(cg_test_a) || cg_create(cg_test_b))
>> 		goto cleanup;
>>
>> So we create two subgroups this translates to
>>
>> cg_child_a = tst_cg_group_mk(tst_cg, 'a');
>> cg_child_b = tst_cg_group_mk(tst_cg, 'b');
>>
>>
>> 	if (cg_enter_current(cg_test_a))
>> 		goto cleanup;
>>
>> This writes "0" to the cgroup.procs file which I guess is a shortcut for
>> writing the current processes PID. We have to be careful here incase
>> this behaviour is different on V1. Probably this translates to
>>
>> SAFE_CG_PRINT(cg_child_a, "cgroup.procs", "0");
>>
>> It's not clear why the current PID is moved to this CG. It may be to
>> ensure we have permission to move to a sibling CGroup and to eliminate
>> other possible reasons for getting EACCES later on.
> 
> Yes, it's not clear.

Will follow kernel selftest style, use child_a and child_b two test cgroups.

> 
> But from kernel commit[1], it just check open fd's perms instead of
> current. So I don't use a and b two cgroup .  Also, it indeed fail on
> unfixed kernel  by using only one a cgrpup.
> 
> [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1756d7994
> 
>>
>> 	if (chown(cg_test_a_procs, test_euid, -1) ||
>> 	    chown(cg_test_b_procs, test_euid, -1))
>> 		goto cleanup;
>>
>> Then the procs files are chowned to nobody. I see no reason this
>> function can not be implemented in the same way safe_cg_printf is. We
>> don't need to check which controller the file belongs to, just chown all
>> of them.
>>
>> 	saved_uid = geteuid();
>> 	if (seteuid(test_euid))
>> 		goto cleanup;
> 
> Yes, we can chown all cgroup procs files ie SAFE_CG_PRINTF did.
> 
>>
>> Then the current procs effective uid is changed. We need to make sure to
>> set this back before doing cleanup (or fork like you did originally).
>>
>> 	cg_test_b_procs_fd = open(cg_test_b_procs, O_RDWR);
>>
>> 	if (seteuid(saved_uid))
>> 		goto cleanup;
>>
>> Then the file is opened and the euid set back
>>
>> 	if (cg_test_b_procs_fd < 0)
>> 		goto cleanup;
>>
>> 	if (write(cg_test_b_procs_fd, "0", 1) >= 0 || errno != EACCES)
>> 		goto cleanup;
>>
>> Then we try to move to CG B and expect to get EACCES (because of the
>> permissions present when opening the file).
>>
>> 	ret = KSFT_PASS;
>> ...
>>
>>
>> Probably we want to run this test on any controllers which are
>> available. Currently the API doesn't support that. We need some way of
>> specifying that the test will use any available controllers in
>> tst_cg_require and/or tst_test.
> 
> I will move this case into cgroup/cg_core01.c, kernel selftest seems use
> memory controller, I guess we can also use memory controller because it
> is most common.
>>
>> Then we need to handle setting the euid between open and writing which
>> stops us from using safe_cg_printf.
>>
>> Probably the API shouldn't try to handle stuff this wierd. Instead we
>> can create a function like
>>
>> int n = tst_cg_group_dirfds(int *dirfds)
>>
>> which copies tst_cgroup_group.dirs[i].dir_fd into dirfds (which can be
>> statically allocated in the test code if we export CTRLS_MAX).
>>
>> Then we can do
>>
>> seteuid(nobody);
>>
>> for (i = 0; i < n; i++) {
>>       procfds[i] = openat(dirfds[i], "cgroup.procs")
>> }
>>
>> seteuid(saved_euid);
>>
>> for (i = 0; i < n; i++) {
>>       ret = write(procfds[i], "0", 1);
>>       if (ret >= 0)
>>          tst_res(TFAIL...)
>>       ...
>>
>>       close(procfds[i]);
>> }
>>
>> and whatever else is required by tests which are doing something unusual
>> with the CG hierarchy.
> 
> OK. Will  try.
> 

After some try, I think may use the old way ie safe_cg_printf but store 
the fd array and return arry size is more clear. Please see my RFC-V3 patch.

Best Regards
Yang Xu
> Best Regards
> Yang Xu
>>
> 

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3 1/3] tst_safe_file_at: Add SAFE_FCHOWNAT macro

[Yang Xu]

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 include/tst_safe_file_at.h |  9 +++++++++
 lib/tst_safe_file_at.c     | 20 ++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/include/tst_safe_file_at.h b/include/tst_safe_file_at.h
index 8df34227f..e253198e6 100644
--- a/include/tst_safe_file_at.h
+++ b/include/tst_safe_file_at.h
@@ -25,6 +25,10 @@
 #define SAFE_UNLINKAT(dirfd, path, flags)				\
 	safe_unlinkat(__FILE__, __LINE__, (dirfd), (path), (flags))
 
+#define SAFE_FCHOWNAT(dirfd, path, owner, group, flags)			\
+	safe_fchownat(__FILE__, __LINE__,				\
+			(dirfd), (path), (owner), (group), (flags))
+
 const char *tst_decode_fd(const int fd)
 			  __attribute__((warn_unused_result));
 
@@ -58,4 +62,9 @@ int safe_unlinkat(const char *const file, const int lineno,
 		  const int dirfd, const char *const path, const int flags)
 		  __attribute__ ((nonnull));
 
+int safe_fchownat(const char *const file, const int lineno,
+		  const int dirfd, const char *const path, uid_t owner,
+		  gid_t group, int flags)
+		  __attribute__ ((nonnull));
+
 #endif
diff --git a/lib/tst_safe_file_at.c b/lib/tst_safe_file_at.c
index ca8ef2f68..6370a68e5 100644
--- a/lib/tst_safe_file_at.c
+++ b/lib/tst_safe_file_at.c
@@ -195,3 +195,23 @@ int safe_unlinkat(const char *const file, const int lineno,
 
 	return rval;
 }
+
+int safe_fchownat(const char *const file, const int lineno,
+		  const int dirfd, const char *const path, uid_t owner, gid_t group, int flags)
+{
+	int rval;
+
+	rval = fchownat(dirfd, path, owner, group, flags);
+
+	if (rval == -1) {
+		tst_brk_(file, lineno, TBROK | TERRNO,
+			 "fchownat(%d<%s>, '%s', %d, %d, %d) failed", dirfd,
+			 tst_decode_fd(dirfd), path, owner, group, flags);
+	} else if (rval) {
+		tst_brk_(file, lineno, TBROK | TERRNO,
+			 "Invalid fchownat(%d<%s>, '%s', %d, %d, %d) return value %d",
+			 dirfd, tst_decode_fd(dirfd), path, owner, group, flags, rval);
+	}
+
+	return rval;
+}
-- 
2.27.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3 2/3] tst_cgroup: Add safe_cg_open and safe_cg_fchown functions

[Yang Xu]

safe_cg_open is used to open the sub control's file ie cgroup.procs
and returns the opened fd number. The opened fd array is stored in
fds argument.

safe_cg_fchown is used to use fchownat to change file's uid and gid.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 include/tst_cgroup.h | 44 +++++++++++++++++++++++++++++
 lib/tst_cgroup.c     | 66 +++++++++++++++++++++++++++-----------------
 2 files changed, 85 insertions(+), 25 deletions(-)

diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
index d06847cc6..70c5b3fd4 100644
--- a/include/tst_cgroup.h
+++ b/include/tst_cgroup.h
@@ -89,6 +89,32 @@ enum tst_cg_ver {
 	TST_CG_V2 = 2,
 };
 
+/* Controller sub-systems */
+enum cgroup_ctrl_indx {
+	CTRL_MEMORY = 1,
+	CTRL_CPU,
+	CTRL_CPUSET,
+	CTRL_IO,
+	CTRL_PIDS,
+	CTRL_HUGETLB,
+	CTRL_CPUACCT,
+	CTRL_DEVICES,
+	CTRL_FREEZER,
+	CTRL_NETCLS,
+	CTRL_NETPRIO,
+	CTRL_BLKIO,
+	CTRL_MISC,
+	CTRL_PERFEVENT,
+	CTRL_DEBUG
+};
+
+#define CTRLS_MAX CTRL_DEBUG
+
+/* At most we can have one cgroup V1 tree for each controller and one
+ * (empty) v2 tree.
+ */
+#define ROOTS_MAX (CTRLS_MAX + 1)
+
 /* Used to specify CGroup hierarchy configuration options, allowing a
  * test to request a particular CGroup structure.
  */
@@ -201,6 +227,24 @@ void safe_cg_printf(const char *const file, const int lineno,
 			const char *const fmt, ...)
 			__attribute__ ((format (printf, 5, 6), nonnull));
 
+#define SAFE_CG_OPEN(cg, file_name, flags, fds)			\
+	safe_cg_open(__FILE__, __LINE__, (cg), (file_name), (flags), (fds))
+
+int safe_cg_open(const char *const file, const int lineno,
+			const struct tst_cg_group *const cg,
+			const char *const file_name,
+			int flags, int *fds)
+			__attribute__ ((nonnull));
+
+#define SAFE_CG_FCHOWN(cg, file_name, owner, group)		\
+	safe_cg_fchown(__FILE__, __LINE__, (cg), (file_name), (owner), (group))
+
+void safe_cg_fchown(const char *const file, const int lineno,
+			const struct tst_cg_group *const cg,
+			const char *const file_name,
+			uid_t owner, gid_t group)
+			__attribute__ ((nonnull));
+
 #define SAFE_CG_SCANF(cg, file_name, fmt, ...)			\
 	safe_cg_scanf(__FILE__, __LINE__,				\
 			  (cg), (file_name), (fmt), __VA_ARGS__)
diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index 1cfd79243..3d9568efe 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -77,31 +77,6 @@ struct cgroup_root {
 	int no_cpuset_prefix:1;
 };
 
-/* Controller sub-systems */
-enum cgroup_ctrl_indx {
-	CTRL_MEMORY = 1,
-	CTRL_CPU,
-	CTRL_CPUSET,
-	CTRL_IO,
-	CTRL_PIDS,
-	CTRL_HUGETLB,
-	CTRL_CPUACCT,
-	CTRL_DEVICES,
-	CTRL_FREEZER,
-	CTRL_NETCLS,
-	CTRL_NETPRIO,
-	CTRL_BLKIO,
-	CTRL_MISC,
-	CTRL_PERFEVENT,
-	CTRL_DEBUG
-};
-#define CTRLS_MAX CTRL_DEBUG
-
-/* At most we can have one cgroup V1 tree for each controller and one
- * (empty) v2 tree.
- */
-#define ROOTS_MAX (CTRLS_MAX + 1)
-
 /* Describes a controller file or knob
  *
  * The primary purpose of this is to map V2 names to V1
@@ -1320,6 +1295,47 @@ void safe_cg_printf(const char *const file, const int lineno,
 	}
 }
 
+int safe_cg_open(const char *const file, const int lineno,
+		      const struct tst_cg_group *cg,
+		      const char *const file_name, int flags, int *fds)
+{
+	const struct cgroup_file *const cfile =
+		cgroup_file_find(file, lineno, file_name);
+	struct cgroup_dir *const *dir;
+	const char *alias;
+	int i = 0;
+
+	for_each_dir(cg, cfile->ctrl_indx, dir) {
+		alias = cgroup_file_alias(cfile, *dir);
+		if (!alias)
+			continue;
+
+		fds[i++] = safe_openat(file, lineno, (*dir)->dir_fd, alias, flags);
+	}
+
+	return i;
+}
+
+void safe_cg_fchown(const char *const file, const int lineno,
+			const struct tst_cg_group *cg,
+			const char *const file_name,
+			uid_t owner, gid_t group)
+{
+	const struct cgroup_file *const cfile =
+		cgroup_file_find(file, lineno, file_name);
+	struct cgroup_dir *const *dir;
+	const char *alias;
+
+	for_each_dir(cg, cfile->ctrl_indx, dir) {
+		alias = cgroup_file_alias(cfile, *dir);
+		if (!alias)
+			continue;
+
+		safe_fchownat(file, lineno, (*dir)->dir_fd, alias, owner, group, 0);
+	}
+}
+
+
 void safe_cg_scanf(const char *const file, const int lineno,
 		       const struct tst_cg_group *const cg,
 		       const char *const file_name,
-- 
2.27.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3 3/3] core01: copy from kernel selftest test_cgcore_lesser_euid_open

[Yang Xu]

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 runtest/controllers                           |   1 +
 .../kernel/controllers/cgroup/.gitignore      |   1 +
 testcases/kernel/controllers/cgroup/core01.c  | 107 ++++++++++++++++++
 3 files changed, 109 insertions(+)
 create mode 100644 testcases/kernel/controllers/cgroup/core01.c

diff --git a/runtest/controllers b/runtest/controllers
index 22d482050..c0052713e 100644
--- a/runtest/controllers
+++ b/runtest/controllers
@@ -1,4 +1,5 @@
 #DESCRIPTION:Resource Management testing
+core01		core01
 cgroup		cgroup_regression_test.sh
 memcg_regression	memcg_regression_test.sh
 memcg_test_3	memcg_test_3
diff --git a/testcases/kernel/controllers/cgroup/.gitignore b/testcases/kernel/controllers/cgroup/.gitignore
index eb91cc4e1..f1c878d74 100644
--- a/testcases/kernel/controllers/cgroup/.gitignore
+++ b/testcases/kernel/controllers/cgroup/.gitignore
@@ -1,3 +1,4 @@
 /cgroup_regression_fork_processes
 /cgroup_regression_getdelays
 /cgroup_regression_6_2
+/core01
diff --git a/testcases/kernel/controllers/cgroup/core01.c b/testcases/kernel/controllers/cgroup/core01.c
new file mode 100644
index 000000000..80ffec85c
--- /dev/null
+++ b/testcases/kernel/controllers/cgroup/core01.c
@@ -0,0 +1,107 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved.
+ * Author: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * When a task is writing to an fd opened by a different task, the perm check
+ * should use the credentials of the latter task.
+ *
+ * It is copy from kernel selftests cgroup test_core test_cgcore_lesser_euid_open
+ * subcase. The difference is that kernel selftest only supports cgroup v2 but
+ * here we also support cgroup v1 and v2.
+ *
+ * It is a regression test for
+ *
+ * commit e57457641613fef0d147ede8bd6a3047df588b95
+ * Author: Tejun Heo <tj@kernel.org>
+ * Date:   Thu Jan 6 11:02:29 2022 -1000
+ *
+ * cgroup: Use open-time cgroup namespace for process migration perm checks
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <sys/types.h>
+#include <pwd.h>
+#include "tst_test.h"
+#include "tst_safe_file_at.h"
+
+static struct tst_cg_group *cg_child_a, *cg_child_b;
+static uid_t nobody_uid, save_uid;
+
+static void test_lesser_euid_open(void)
+{
+	int fds[ROOTS_MAX] = {-1};
+	int i, loops;
+
+	cg_child_a = tst_cg_group_mk(tst_cg, "child_a");
+	cg_child_b = tst_cg_group_mk(tst_cg, "child_b");
+
+	if (!SAFE_FORK()) {
+		SAFE_CG_PRINT(cg_child_a, "cgroup.procs", "0");
+		SAFE_CG_FCHOWN(cg_child_a, "cgroup.procs",  nobody_uid, -1);
+		SAFE_CG_FCHOWN(cg_child_b, "cgroup.procs",  nobody_uid, -1);
+		SAFE_SETEUID(nobody_uid);
+
+		loops = SAFE_CG_OPEN(cg_child_b, "cgroup.procs", O_RDWR, fds);
+		SAFE_SETEUID(save_uid);
+
+		for (i = 0; i < loops; i++) {
+			if (fds[i] < 1) {
+				tst_res(TFAIL, "unexpected negative fd %d", fds[i]);
+				exit(1);
+			}
+
+			TEST(write(fds[i], "0", 1));
+			if (TST_RET >= 0 || TST_ERR != EACCES)
+				tst_res(TFAIL, "%s failed", __func__);
+			else
+				tst_res(TPASS | TTERRNO, "%s passed", __func__);
+
+			SAFE_CLOSE(fds[i]);
+		}
+		exit(0);
+	}
+
+	tst_reap_children();
+	cg_child_b = tst_cg_group_rm(cg_child_b);
+	cg_child_a = tst_cg_group_rm(cg_child_a);
+}
+
+static void setup(void)
+{
+	struct passwd *pw;
+
+	pw = SAFE_GETPWNAM("nobody");
+	nobody_uid = pw->pw_uid;
+	save_uid = geteuid();
+}
+
+static void cleanup(void)
+{
+	if (cg_child_a) {
+		SAFE_CG_PRINTF(tst_cg_drain, "cgroup.procs", "%d", getpid());
+		cg_child_a = tst_cg_group_rm(cg_child_a);
+	}
+	if (cg_child_b) {
+		SAFE_CG_PRINTF(tst_cg_drain, "cgroup.procs", "%d", getpid());
+		cg_child_b = tst_cg_group_rm(cg_child_b);
+	}
+}
+
+static struct tst_test test = {
+	.setup = setup,
+	.cleanup = cleanup,
+	.test_all = test_lesser_euid_open,
+	.forks_child = 1,
+	.needs_root = 1,
+	.needs_cgroup_ctrls = (const char *const[]){"memory",  NULL},
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "e57457641613"},
+		{}
+	},
+};
-- 
2.27.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 2/3] tst_cgroup: Add safe_cg_open and safe_cg_fchown functions

[Richard Palethorpe]

Hello,

Yang Xu <xuyang2018.jy@fujitsu.com> writes:

> safe_cg_open is used to open the sub control's file ie cgroup.procs
> and returns the opened fd number. The opened fd array is stored in
> fds argument.
>
> safe_cg_fchown is used to use fchownat to change file's uid and gid.
>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  include/tst_cgroup.h | 44 +++++++++++++++++++++++++++++
>  lib/tst_cgroup.c     | 66 +++++++++++++++++++++++++++-----------------
>  2 files changed, 85 insertions(+), 25 deletions(-)
>
> diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
> index d06847cc6..70c5b3fd4 100644
> --- a/include/tst_cgroup.h
> +++ b/include/tst_cgroup.h
> @@ -89,6 +89,32 @@ enum tst_cg_ver {
>  	TST_CG_V2 = 2,
>  };
>  
> +/* Controller sub-systems */
> +enum cgroup_ctrl_indx {
> +	CTRL_MEMORY = 1,
> +	CTRL_CPU,
> +	CTRL_CPUSET,
> +	CTRL_IO,
> +	CTRL_PIDS,
> +	CTRL_HUGETLB,
> +	CTRL_CPUACCT,
> +	CTRL_DEVICES,
> +	CTRL_FREEZER,
> +	CTRL_NETCLS,
> +	CTRL_NETPRIO,
> +	CTRL_BLKIO,
> +	CTRL_MISC,
> +	CTRL_PERFEVENT,
> +	CTRL_DEBUG
> +};
> +
> +#define CTRLS_MAX CTRL_DEBUG
> +
> +/* At most we can have one cgroup V1 tree for each controller and one
> + * (empty) v2 tree.
> + */
> +#define ROOTS_MAX (CTRLS_MAX + 1)

These need TST_CG_ prepending to them to stop name collisions.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 2/3] tst_cgroup: Add safe_cg_open and safe_cg_fchown functions

[Richard Palethorpe]

Richard Palethorpe <rpalethorpe@suse.de> writes:

> Hello,
>
> Yang Xu <xuyang2018.jy@fujitsu.com> writes:
>
>> safe_cg_open is used to open the sub control's file ie cgroup.procs
>> and returns the opened fd number. The opened fd array is stored in
>> fds argument.
>>
>> safe_cg_fchown is used to use fchownat to change file's uid and gid.
>>
>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>> ---
>>  include/tst_cgroup.h | 44 +++++++++++++++++++++++++++++
>>  lib/tst_cgroup.c     | 66 +++++++++++++++++++++++++++-----------------
>>  2 files changed, 85 insertions(+), 25 deletions(-)
>>
>> diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
>> index d06847cc6..70c5b3fd4 100644
>> --- a/include/tst_cgroup.h
>> +++ b/include/tst_cgroup.h
>> @@ -89,6 +89,32 @@ enum tst_cg_ver {
>>  	TST_CG_V2 = 2,
>>  };
>>  
>> +/* Controller sub-systems */
>> +enum cgroup_ctrl_indx {
>> +	CTRL_MEMORY = 1,
>> +	CTRL_CPU,
>> +	CTRL_CPUSET,
>> +	CTRL_IO,
>> +	CTRL_PIDS,
>> +	CTRL_HUGETLB,
>> +	CTRL_CPUACCT,
>> +	CTRL_DEVICES,
>> +	CTRL_FREEZER,
>> +	CTRL_NETCLS,
>> +	CTRL_NETPRIO,
>> +	CTRL_BLKIO,
>> +	CTRL_MISC,
>> +	CTRL_PERFEVENT,
>> +	CTRL_DEBUG
>> +};
>> +
>> +#define CTRLS_MAX CTRL_DEBUG
>> +
>> +/* At most we can have one cgroup V1 tree for each controller and one
>> + * (empty) v2 tree.
>> + */
>> +#define ROOTS_MAX (CTRLS_MAX + 1)
>
> These need TST_CG_ prepending to them to stop name collisions.

Actually, I think the easiest thing to do is not export these. Instead
you could leave these alone and export "#define TST_CG_ROOTS_MAX 32".

The rest of the patch-set looks great.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 2/3] tst_cgroup: Add safe_cg_open and safe_cg_fchown functions

[xuyang2018.jy]

Hi Richard

> 
> Richard Palethorpe <rpalethorpe@suse.de> writes:
> 
>> Hello,
>>
>> Yang Xu <xuyang2018.jy@fujitsu.com> writes:
>>
>>> safe_cg_open is used to open the sub control's file ie cgroup.procs
>>> and returns the opened fd number. The opened fd array is stored in
>>> fds argument.
>>>
>>> safe_cg_fchown is used to use fchownat to change file's uid and gid.
>>>
>>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>>> ---
>>>   include/tst_cgroup.h | 44 +++++++++++++++++++++++++++++
>>>   lib/tst_cgroup.c     | 66 +++++++++++++++++++++++++++-----------------
>>>   2 files changed, 85 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
>>> index d06847cc6..70c5b3fd4 100644
>>> --- a/include/tst_cgroup.h
>>> +++ b/include/tst_cgroup.h
>>> @@ -89,6 +89,32 @@ enum tst_cg_ver {
>>>   	TST_CG_V2 = 2,
>>>   };
>>>   
>>> +/* Controller sub-systems */
>>> +enum cgroup_ctrl_indx {
>>> +	CTRL_MEMORY = 1,
>>> +	CTRL_CPU,
>>> +	CTRL_CPUSET,
>>> +	CTRL_IO,
>>> +	CTRL_PIDS,
>>> +	CTRL_HUGETLB,
>>> +	CTRL_CPUACCT,
>>> +	CTRL_DEVICES,
>>> +	CTRL_FREEZER,
>>> +	CTRL_NETCLS,
>>> +	CTRL_NETPRIO,
>>> +	CTRL_BLKIO,
>>> +	CTRL_MISC,
>>> +	CTRL_PERFEVENT,
>>> +	CTRL_DEBUG
>>> +};
>>> +
>>> +#define CTRLS_MAX CTRL_DEBUG
>>> +
>>> +/* At most we can have one cgroup V1 tree for each controller and one
>>> + * (empty) v2 tree.
>>> + */
>>> +#define ROOTS_MAX (CTRLS_MAX + 1)
>>
>> These need TST_CG_ prepending to them to stop name collisions.
> 
> Actually, I think the easiest thing to do is not export these. Instead
> you could leave these alone and export "#define TST_CG_ROOTS_MAX 32".

Yes, I forgot ltp name rules when export it.

Will use  TST_CG_ROOTS_MAX in v4  and add your reviewed-by.


Best Regards
Yang Xu
> 
> The rest of the patch-set looks great.
> 

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v4 1/3] tst_safe_file_at: Add SAFE_FCHOWNAT macro

[Yang Xu]

Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 include/tst_safe_file_at.h |  9 +++++++++
 lib/tst_safe_file_at.c     | 20 ++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/include/tst_safe_file_at.h b/include/tst_safe_file_at.h
index 8df34227f..e253198e6 100644
--- a/include/tst_safe_file_at.h
+++ b/include/tst_safe_file_at.h
@@ -25,6 +25,10 @@
 #define SAFE_UNLINKAT(dirfd, path, flags)				\
 	safe_unlinkat(__FILE__, __LINE__, (dirfd), (path), (flags))
 
+#define SAFE_FCHOWNAT(dirfd, path, owner, group, flags)			\
+	safe_fchownat(__FILE__, __LINE__,				\
+			(dirfd), (path), (owner), (group), (flags))
+
 const char *tst_decode_fd(const int fd)
 			  __attribute__((warn_unused_result));
 
@@ -58,4 +62,9 @@ int safe_unlinkat(const char *const file, const int lineno,
 		  const int dirfd, const char *const path, const int flags)
 		  __attribute__ ((nonnull));
 
+int safe_fchownat(const char *const file, const int lineno,
+		  const int dirfd, const char *const path, uid_t owner,
+		  gid_t group, int flags)
+		  __attribute__ ((nonnull));
+
 #endif
diff --git a/lib/tst_safe_file_at.c b/lib/tst_safe_file_at.c
index ca8ef2f68..6370a68e5 100644
--- a/lib/tst_safe_file_at.c
+++ b/lib/tst_safe_file_at.c
@@ -195,3 +195,23 @@ int safe_unlinkat(const char *const file, const int lineno,
 
 	return rval;
 }
+
+int safe_fchownat(const char *const file, const int lineno,
+		  const int dirfd, const char *const path, uid_t owner, gid_t group, int flags)
+{
+	int rval;
+
+	rval = fchownat(dirfd, path, owner, group, flags);
+
+	if (rval == -1) {
+		tst_brk_(file, lineno, TBROK | TERRNO,
+			 "fchownat(%d<%s>, '%s', %d, %d, %d) failed", dirfd,
+			 tst_decode_fd(dirfd), path, owner, group, flags);
+	} else if (rval) {
+		tst_brk_(file, lineno, TBROK | TERRNO,
+			 "Invalid fchownat(%d<%s>, '%s', %d, %d, %d) return value %d",
+			 dirfd, tst_decode_fd(dirfd), path, owner, group, flags, rval);
+	}
+
+	return rval;
+}
-- 
2.27.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v4 2/3] tst_cgroup: Add safe_cg_open and safe_cg_fchown functions

[Yang Xu]

safe_cg_open is used to open the sub controller's file ie cgroup.procs
and returns the opened fd number. The opened fd array is stored in
fds argument.

safe_cg_fchown is used to use fchownat to change file's uid and gid.

Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 include/tst_cgroup.h | 21 +++++++++++++++++++++
 lib/tst_cgroup.c     | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
index d06847cc6..db959380f 100644
--- a/include/tst_cgroup.h
+++ b/include/tst_cgroup.h
@@ -89,6 +89,9 @@ enum tst_cg_ver {
 	TST_CG_V2 = 2,
 };
 
+/* This value is equal to ROOTS_MAX in tst_cgroup.c. */
+#define TST_CG_ROOTS_MAX 32
+
 /* Used to specify CGroup hierarchy configuration options, allowing a
  * test to request a particular CGroup structure.
  */
@@ -201,6 +204,24 @@ void safe_cg_printf(const char *const file, const int lineno,
 			const char *const fmt, ...)
 			__attribute__ ((format (printf, 5, 6), nonnull));
 
+#define SAFE_CG_OPEN(cg, file_name, flags, fds)			\
+	safe_cg_open(__FILE__, __LINE__, (cg), (file_name), (flags), (fds))
+
+int safe_cg_open(const char *const file, const int lineno,
+			const struct tst_cg_group *const cg,
+			const char *const file_name,
+			int flags, int *fds)
+			__attribute__ ((nonnull));
+
+#define SAFE_CG_FCHOWN(cg, file_name, owner, group)		\
+	safe_cg_fchown(__FILE__, __LINE__, (cg), (file_name), (owner), (group))
+
+void safe_cg_fchown(const char *const file, const int lineno,
+			const struct tst_cg_group *const cg,
+			const char *const file_name,
+			uid_t owner, gid_t group)
+			__attribute__ ((nonnull));
+
 #define SAFE_CG_SCANF(cg, file_name, fmt, ...)			\
 	safe_cg_scanf(__FILE__, __LINE__,				\
 			  (cg), (file_name), (fmt), __VA_ARGS__)
diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index 1da3f0a5d..6c015e4f8 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -1326,6 +1326,47 @@ void safe_cg_printf(const char *const file, const int lineno,
 	}
 }
 
+int safe_cg_open(const char *const file, const int lineno,
+		      const struct tst_cg_group *cg,
+		      const char *const file_name, int flags, int *fds)
+{
+	const struct cgroup_file *const cfile =
+		cgroup_file_find(file, lineno, file_name);
+	struct cgroup_dir *const *dir;
+	const char *alias;
+	int i = 0;
+
+	for_each_dir(cg, cfile->ctrl_indx, dir) {
+		alias = cgroup_file_alias(cfile, *dir);
+		if (!alias)
+			continue;
+
+		fds[i++] = safe_openat(file, lineno, (*dir)->dir_fd, alias, flags);
+	}
+
+	return i;
+}
+
+void safe_cg_fchown(const char *const file, const int lineno,
+			const struct tst_cg_group *cg,
+			const char *const file_name,
+			uid_t owner, gid_t group)
+{
+	const struct cgroup_file *const cfile =
+		cgroup_file_find(file, lineno, file_name);
+	struct cgroup_dir *const *dir;
+	const char *alias;
+
+	for_each_dir(cg, cfile->ctrl_indx, dir) {
+		alias = cgroup_file_alias(cfile, *dir);
+		if (!alias)
+			continue;
+
+		safe_fchownat(file, lineno, (*dir)->dir_fd, alias, owner, group, 0);
+	}
+}
+
+
 void safe_cg_scanf(const char *const file, const int lineno,
 		       const struct tst_cg_group *const cg,
 		       const char *const file_name,
-- 
2.27.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v4 3/3] cgroup_core01: copy from kernel selftest test_cgcore_lesser_euid_open

[Yang Xu]

Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 runtest/controllers                           |   1 +
 .../kernel/controllers/cgroup/.gitignore      |   1 +
 .../kernel/controllers/cgroup/cgroup_core01.c | 107 ++++++++++++++++++
 3 files changed, 109 insertions(+)
 create mode 100644 testcases/kernel/controllers/cgroup/cgroup_core01.c

diff --git a/runtest/controllers b/runtest/controllers
index 22d482050..41f8367e4 100644
--- a/runtest/controllers
+++ b/runtest/controllers
@@ -1,4 +1,5 @@
 #DESCRIPTION:Resource Management testing
+cgroup_core01	cgroup_core01
 cgroup		cgroup_regression_test.sh
 memcg_regression	memcg_regression_test.sh
 memcg_test_3	memcg_test_3
diff --git a/testcases/kernel/controllers/cgroup/.gitignore b/testcases/kernel/controllers/cgroup/.gitignore
index eb91cc4e1..382f2d9f2 100644
--- a/testcases/kernel/controllers/cgroup/.gitignore
+++ b/testcases/kernel/controllers/cgroup/.gitignore
@@ -1,3 +1,4 @@
 /cgroup_regression_fork_processes
 /cgroup_regression_getdelays
 /cgroup_regression_6_2
+/cgroup_core01
diff --git a/testcases/kernel/controllers/cgroup/cgroup_core01.c b/testcases/kernel/controllers/cgroup/cgroup_core01.c
new file mode 100644
index 000000000..fc60ae5aa
--- /dev/null
+++ b/testcases/kernel/controllers/cgroup/cgroup_core01.c
@@ -0,0 +1,107 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved.
+ * Author: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * When a task is writing to an fd opened by a different task, the perm check
+ * should use the credentials of the latter task.
+ *
+ * It is copy from kernel selftests cgroup test_core test_cgcore_lesser_euid_open
+ * subcase. The difference is that kernel selftest only supports cgroup v2 but
+ * here we also support cgroup v1 and v2.
+ *
+ * It is a regression test for
+ *
+ * commit e57457641613fef0d147ede8bd6a3047df588b95
+ * Author: Tejun Heo <tj@kernel.org>
+ * Date:   Thu Jan 6 11:02:29 2022 -1000
+ *
+ * cgroup: Use open-time cgroup namespace for process migration perm checks
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <sys/types.h>
+#include <pwd.h>
+#include "tst_test.h"
+#include "tst_safe_file_at.h"
+
+static struct tst_cg_group *cg_child_a, *cg_child_b;
+static uid_t nobody_uid, save_uid;
+
+static void test_lesser_euid_open(void)
+{
+	int fds[TST_CG_ROOTS_MAX] = {-1};
+	int i, loops;
+
+	cg_child_a = tst_cg_group_mk(tst_cg, "child_a");
+	cg_child_b = tst_cg_group_mk(tst_cg, "child_b");
+
+	if (!SAFE_FORK()) {
+		SAFE_CG_PRINT(cg_child_a, "cgroup.procs", "0");
+		SAFE_CG_FCHOWN(cg_child_a, "cgroup.procs",  nobody_uid, -1);
+		SAFE_CG_FCHOWN(cg_child_b, "cgroup.procs",  nobody_uid, -1);
+		SAFE_SETEUID(nobody_uid);
+
+		loops = SAFE_CG_OPEN(cg_child_b, "cgroup.procs", O_RDWR, fds);
+		SAFE_SETEUID(save_uid);
+
+		for (i = 0; i < loops; i++) {
+			if (fds[i] < 1) {
+				tst_res(TFAIL, "unexpected negative fd %d", fds[i]);
+				exit(1);
+			}
+
+			TEST(write(fds[i], "0", 1));
+			if (TST_RET >= 0 || TST_ERR != EACCES)
+				tst_res(TFAIL, "%s failed", __func__);
+			else
+				tst_res(TPASS | TTERRNO, "%s passed", __func__);
+
+			SAFE_CLOSE(fds[i]);
+		}
+		exit(0);
+	}
+
+	tst_reap_children();
+	cg_child_b = tst_cg_group_rm(cg_child_b);
+	cg_child_a = tst_cg_group_rm(cg_child_a);
+}
+
+static void setup(void)
+{
+	struct passwd *pw;
+
+	pw = SAFE_GETPWNAM("nobody");
+	nobody_uid = pw->pw_uid;
+	save_uid = geteuid();
+}
+
+static void cleanup(void)
+{
+	if (cg_child_a) {
+		SAFE_CG_PRINTF(tst_cg_drain, "cgroup.procs", "%d", getpid());
+		cg_child_a = tst_cg_group_rm(cg_child_a);
+	}
+	if (cg_child_b) {
+		SAFE_CG_PRINTF(tst_cg_drain, "cgroup.procs", "%d", getpid());
+		cg_child_b = tst_cg_group_rm(cg_child_b);
+	}
+}
+
+static struct tst_test test = {
+	.setup = setup,
+	.cleanup = cleanup,
+	.test_all = test_lesser_euid_open,
+	.forks_child = 1,
+	.needs_root = 1,
+	.needs_cgroup_ctrls = (const char *const[]){"memory",  NULL},
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "e57457641613"},
+		{}
+	},
+};
-- 
2.27.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v4 2/3] tst_cgroup: Add safe_cg_open and safe_cg_fchown functions

[Richard Palethorpe]

Hello,

Yang Xu <xuyang2018.jy@fujitsu.com> writes:

> safe_cg_open is used to open the sub controller's file ie cgroup.procs
> and returns the opened fd number. The opened fd array is stored in
> fds argument.
>
> safe_cg_fchown is used to use fchownat to change file's uid and gid.
>
> Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  include/tst_cgroup.h | 21 +++++++++++++++++++++
>  lib/tst_cgroup.c     | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
>
> diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
> index d06847cc6..db959380f 100644
> --- a/include/tst_cgroup.h
> +++ b/include/tst_cgroup.h
> @@ -89,6 +89,9 @@ enum tst_cg_ver {
>  	TST_CG_V2 = 2,
>  };
>  
> +/* This value is equal to ROOTS_MAX in tst_cgroup.c. */
> +#define TST_CG_ROOTS_MAX 32

Thanks, pushed with small change to this comment. Because
TST_CG_ROOTS_MAX is actually greater than ROOTS_MAX.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v4 2/3] tst_cgroup: Add safe_cg_open and safe_cg_fchown functions

[xuyang2018.jy]

Hi Richard

> Hello,
> 
> Yang Xu <xuyang2018.jy@fujitsu.com> writes:
> 
>> safe_cg_open is used to open the sub controller's file ie cgroup.procs
>> and returns the opened fd number. The opened fd array is stored in
>> fds argument.
>>
>> safe_cg_fchown is used to use fchownat to change file's uid and gid.
>>
>> Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>
>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>> ---
>>   include/tst_cgroup.h | 21 +++++++++++++++++++++
>>   lib/tst_cgroup.c     | 41 +++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 62 insertions(+)
>>
>> diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
>> index d06847cc6..db959380f 100644
>> --- a/include/tst_cgroup.h
>> +++ b/include/tst_cgroup.h
>> @@ -89,6 +89,9 @@ enum tst_cg_ver {
>>   	TST_CG_V2 = 2,
>>   };
>>   
>> +/* This value is equal to ROOTS_MAX in tst_cgroup.c. */
>> +#define TST_CG_ROOTS_MAX 32
> 
> Thanks, pushed with small change to this comment. Because
> TST_CG_ROOTS_MAX is actually greater than ROOTS_MAX.

Oh, yes, good catch.

Best Regards
Yang Xu
> 

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp