[PATCH bpf 0/3] Pin the start cgroup for cgroup iterator

[PATCH bpf 0/3] Pin the start cgroup for cgroup iterator

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

Hi,

The patchset tries to fix the potential use-after-free problem in cgroup
iterator. The problem is similar with the UAF problem fixed in map
iterator and the fixes is also similar: pinning the iterated resource
in .init_seq_private() and unpinning in .fini_seq_private(). Also adding
a test to demonstrate the problem.

Not sure whether or not it will be helpful to add some comments for
.init_seq_private() to state that the implementation of
.init_seq_private() should not depend on iterator link to guarantee
the liveness of iterated object. Comments are always welcome.

Hou Tao (3):
  bpf: Pin the start cgroup in cgroup_iter_seq_init()
  selftests/bpf: Add cgroup helper remove_cgroup()
  selftests/bpf: Add test for cgroup iterator on a dead cgroup

 kernel/bpf/cgroup_iter.c                      | 14 ++++
 tools/testing/selftests/bpf/cgroup_helpers.c  | 19 +++++
 tools/testing/selftests/bpf/cgroup_helpers.h  |  1 +
 .../selftests/bpf/prog_tests/cgroup_iter.c    | 78 +++++++++++++++++++
 4 files changed, 112 insertions(+)

-- 
2.29.2

[PATCH bpf 1/3] bpf: Pin the start cgroup in cgroup_iter_seq_init()

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

bpf_iter_attach_cgroup() has already acquired an extra reference for the
start cgroup, but the reference will be released if the iterator link fd
is closed after the creation of iterator fd, and it may lead to
User-After-Free when reading the iterator fd.

So fixing it by acquiring another reference for the start cgroup.

Fixes: d4ccaf58a847 ("bpf: Introduce cgroup iter")
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 kernel/bpf/cgroup_iter.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/kernel/bpf/cgroup_iter.c b/kernel/bpf/cgroup_iter.c
index 9fcf09f2ef00..c187a9e62bdb 100644
--- a/kernel/bpf/cgroup_iter.c
+++ b/kernel/bpf/cgroup_iter.c
@@ -164,16 +164,30 @@ static int cgroup_iter_seq_init(void *priv, struct bpf_iter_aux_info *aux)
 	struct cgroup_iter_priv *p = (struct cgroup_iter_priv *)priv;
 	struct cgroup *cgrp = aux->cgroup.start;
 
+	/* bpf_iter_attach_cgroup() has already acquired an extra reference
+	 * for the start cgroup, but the reference may be released after
+	 * cgroup_iter_seq_init(), so acquire another reference for the
+	 * start cgroup.
+	 */
 	p->start_css = &cgrp->self;
+	css_get(p->start_css);
 	p->terminate = false;
 	p->visited_all = false;
 	p->order = aux->cgroup.order;
 	return 0;
 }
 
+static void cgroup_iter_seq_fini(void *priv)
+{
+	struct cgroup_iter_priv *p = (struct cgroup_iter_priv *)priv;
+
+	css_put(p->start_css);
+}
+
 static const struct bpf_iter_seq_info cgroup_iter_seq_info = {
 	.seq_ops		= &cgroup_iter_seq_ops,
 	.init_seq_private	= cgroup_iter_seq_init,
+	.fini_seq_private	= cgroup_iter_seq_fini,
 	.seq_priv_size		= sizeof(struct cgroup_iter_priv),
 };
 
-- 
2.29.2

[PATCH bpf 2/3] selftests/bpf: Add cgroup helper remove_cgroup()

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

Add remove_cgroup() to remove a cgroup which doesn't have any children
or live processes. It will be used by the following patch to test cgroup
iterator on a dead cgroup.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 tools/testing/selftests/bpf/cgroup_helpers.c | 19 +++++++++++++++++++
 tools/testing/selftests/bpf/cgroup_helpers.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/tools/testing/selftests/bpf/cgroup_helpers.c b/tools/testing/selftests/bpf/cgroup_helpers.c
index e914cc45b766..d1fef58b41c7 100644
--- a/tools/testing/selftests/bpf/cgroup_helpers.c
+++ b/tools/testing/selftests/bpf/cgroup_helpers.c
@@ -332,6 +332,25 @@ int get_root_cgroup(void)
 	return fd;
 }
 
+/*
+ * remove_cgroup() - Remove a cgroup
+ * @relative_path: The cgroup path, relative to the workdir, to remove
+ *
+ * This function expects a cgroup to already be created, relative to the cgroup
+ * work dir. It also expects the cgroup doesn't have any children or live
+ * processes and it removes the cgroup.
+ *
+ * On failure, it will print an error to stderr.
+ */
+void remove_cgroup(const char *relative_path)
+{
+	char cgroup_path[PATH_MAX + 1];
+
+	format_cgroup_path(cgroup_path, relative_path);
+	if (rmdir(cgroup_path))
+		log_err("rmdiring cgroup %s .. %s", relative_path, cgroup_path);
+}
+
 /**
  * create_and_get_cgroup() - Create a cgroup, relative to workdir, and get the FD
  * @relative_path: The cgroup path, relative to the workdir, to join
diff --git a/tools/testing/selftests/bpf/cgroup_helpers.h b/tools/testing/selftests/bpf/cgroup_helpers.h
index 3358734356ab..f099a166c94d 100644
--- a/tools/testing/selftests/bpf/cgroup_helpers.h
+++ b/tools/testing/selftests/bpf/cgroup_helpers.h
@@ -18,6 +18,7 @@ int write_cgroup_file_parent(const char *relative_path, const char *file,
 int cgroup_setup_and_join(const char *relative_path);
 int get_root_cgroup(void);
 int create_and_get_cgroup(const char *relative_path);
+void remove_cgroup(const char *relative_path);
 unsigned long long get_cgroup_id(const char *relative_path);
 
 int join_cgroup(const char *relative_path);
-- 
2.29.2

[PATCH bpf 3/3] selftests/bpf: Add test for cgroup iterator on a dead cgroup

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

The test closes both iterator link fd and cgroup fd, and removes the
cgroup file to make a dead cgroup before reading cgroup iterator fd. It
also uses kern_sync_rcu() and usleep() to wait for the release of
start cgroup. If the start cgroup is not pinned by cgroup iterator,
reading iterator fd will trigger use-after-free.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 .../selftests/bpf/prog_tests/cgroup_iter.c    | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c b/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c
index c4a2adb38da1..d64ed1cf1554 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c
@@ -189,6 +189,82 @@ static void test_walk_self_only(struct cgroup_iter *skel)
 			      BPF_CGROUP_ITER_SELF_ONLY, "self_only");
 }
 
+static void test_walk_dead_self_only(struct cgroup_iter *skel)
+{
+	DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts);
+	char expected_output[128], buf[128];
+	const char *cgrp_name = "/dead";
+	union bpf_iter_link_info linfo;
+	int len, cgrp_fd, iter_fd;
+	struct bpf_link *link;
+	size_t left;
+	char *p;
+
+	cgrp_fd = create_and_get_cgroup(cgrp_name);
+	if (!ASSERT_GE(cgrp_fd, 0, "create cgrp"))
+		return;
+
+	/* The cgroup is already dead during iteration, so it only has epilogue
+	 * in the output.
+	 */
+	snprintf(expected_output, sizeof(expected_output), EPILOGUE);
+
+	memset(&linfo, 0, sizeof(linfo));
+	linfo.cgroup.cgroup_fd = cgrp_fd;
+	linfo.cgroup.order = BPF_CGROUP_ITER_SELF_ONLY;
+	opts.link_info = &linfo;
+	opts.link_info_len = sizeof(linfo);
+
+	link = bpf_program__attach_iter(skel->progs.cgroup_id_printer, &opts);
+	if (!ASSERT_OK_PTR(link, "attach_iter"))
+		goto close_cg;
+
+	iter_fd = bpf_iter_create(bpf_link__fd(link));
+	if (!ASSERT_GE(iter_fd, 0, "iter_create"))
+		goto free_link;
+
+	/* Close link fd and cgroup fd */
+	bpf_link__destroy(link);
+	link = NULL;
+	close(cgrp_fd);
+	cgrp_fd = -1;
+
+	/* Remove cgroup to mark it as dead */
+	remove_cgroup(cgrp_name);
+
+	/* Two kern_sync_rcu() and usleep() pair are used to wait for the
+	 * releases of cgroup css, and the last kern_sync_rcu() and usleep()
+	 * pair is used to wait for the free of cgroup itself.
+	 */
+	kern_sync_rcu();
+	usleep(8000);
+	kern_sync_rcu();
+	usleep(8000);
+	kern_sync_rcu();
+	usleep(1000);
+
+	memset(buf, 0, sizeof(buf));
+	left = ARRAY_SIZE(buf);
+	p = buf;
+	while ((len = read(iter_fd, p, left)) > 0) {
+		p += len;
+		left -= len;
+	}
+
+	ASSERT_STREQ(buf, expected_output, "dead cgroup output");
+
+	/* read() after iter finishes should be ok. */
+	if (len == 0)
+		ASSERT_OK(read(iter_fd, buf, sizeof(buf)), "second_read");
+
+	close(iter_fd);
+free_link:
+	bpf_link__destroy(link);
+close_cg:
+	if (cgrp_fd >= 0)
+		close(cgrp_fd);
+}
+
 void test_cgroup_iter(void)
 {
 	struct cgroup_iter *skel = NULL;
@@ -217,6 +293,8 @@ void test_cgroup_iter(void)
 		test_early_termination(skel);
 	if (test__start_subtest("cgroup_iter__self_only"))
 		test_walk_self_only(skel);
+	if (test__start_subtest("cgroup_iter_dead_self_only"))
+		test_walk_dead_self_only(skel);
 out:
 	cgroup_iter__destroy(skel);
 	cleanup_cgroups();
-- 
2.29.2

Re: [PATCH bpf 1/3] bpf: Pin the start cgroup in cgroup_iter_seq_init()

[Yonghong Song]

On 11/6/22 11:42 PM, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> bpf_iter_attach_cgroup() has already acquired an extra reference for the
> start cgroup, but the reference will be released if the iterator link fd
> is closed after the creation of iterator fd, and it may lead to
> User-After-Free when reading the iterator fd.
> 
> So fixing it by acquiring another reference for the start cgroup.
> 
> Fixes: d4ccaf58a847 ("bpf: Introduce cgroup iter")
> Signed-off-by: Hou Tao <houtao1@huawei.com>

Acked-by: Yonghong Song <yhs@fb.com>

Re: [PATCH bpf 2/3] selftests/bpf: Add cgroup helper remove_cgroup()

[Yonghong Song]

On 11/6/22 11:42 PM, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> Add remove_cgroup() to remove a cgroup which doesn't have any children
> or live processes. It will be used by the following patch to test cgroup
> iterator on a dead cgroup.
> 
> Signed-off-by: Hou Tao <houtao1@huawei.com>

Acked-by: Yonghong Song <yhs@fb.com>

Re: [PATCH bpf 3/3] selftests/bpf: Add test for cgroup iterator on a dead cgroup

[Yonghong Song]

On 11/6/22 11:42 PM, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> The test closes both iterator link fd and cgroup fd, and removes the
> cgroup file to make a dead cgroup before reading cgroup iterator fd. It
> also uses kern_sync_rcu() and usleep() to wait for the release of
> start cgroup. If the start cgroup is not pinned by cgroup iterator,
> reading iterator fd will trigger use-after-free.
> 
> Signed-off-by: Hou Tao <houtao1@huawei.com>

LGTM with a few nits below.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   .../selftests/bpf/prog_tests/cgroup_iter.c    | 78 +++++++++++++++++++
>   1 file changed, 78 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c b/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c
> index c4a2adb38da1..d64ed1cf1554 100644
> --- a/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c
> @@ -189,6 +189,82 @@ static void test_walk_self_only(struct cgroup_iter *skel)
>   			      BPF_CGROUP_ITER_SELF_ONLY, "self_only");
>   }
>   
> +static void test_walk_dead_self_only(struct cgroup_iter *skel)
> +{
> +	DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts);
> +	char expected_output[128], buf[128];
> +	const char *cgrp_name = "/dead";
> +	union bpf_iter_link_info linfo;
> +	int len, cgrp_fd, iter_fd;
> +	struct bpf_link *link;
> +	size_t left;
> +	char *p;
> +
> +	cgrp_fd = create_and_get_cgroup(cgrp_name);
> +	if (!ASSERT_GE(cgrp_fd, 0, "create cgrp"))
> +		return;
> +
> +	/* The cgroup is already dead during iteration, so it only has epilogue
> +	 * in the output.
> +	 */

Let us reword the comment like
	The cgroup will be dead during read() iteration, and it only has
	epilogue in the output.

> +	snprintf(expected_output, sizeof(expected_output), EPILOGUE);
> +
> +	memset(&linfo, 0, sizeof(linfo));
> +	linfo.cgroup.cgroup_fd = cgrp_fd;
> +	linfo.cgroup.order = BPF_CGROUP_ITER_SELF_ONLY;
> +	opts.link_info = &linfo;
> +	opts.link_info_len = sizeof(linfo);
> +
> +	link = bpf_program__attach_iter(skel->progs.cgroup_id_printer, &opts);
> +	if (!ASSERT_OK_PTR(link, "attach_iter"))
> +		goto close_cg;
> +
> +	iter_fd = bpf_iter_create(bpf_link__fd(link));
> +	if (!ASSERT_GE(iter_fd, 0, "iter_create"))
> +		goto free_link;
> +
> +	/* Close link fd and cgroup fd */
> +	bpf_link__destroy(link);
> +	link = NULL;
> +	close(cgrp_fd);
> +	cgrp_fd = -1;

We can remove 'link = NULL' and 'cgroup_fd = -1' and
add 'return' after 'close(iter_fd)', which seems better
to understand the code.

> +
> +	/* Remove cgroup to mark it as dead */
> +	remove_cgroup(cgrp_name);
> +
> +	/* Two kern_sync_rcu() and usleep() pair are used to wait for the
> +	 * releases of cgroup css, and the last kern_sync_rcu() and usleep()
> +	 * pair is used to wait for the free of cgroup itself.
> +	 */
> +	kern_sync_rcu();
> +	usleep(8000);
> +	kern_sync_rcu();
> +	usleep(8000);
> +	kern_sync_rcu();
> +	usleep(1000);
> +
> +	memset(buf, 0, sizeof(buf));
> +	left = ARRAY_SIZE(buf);
> +	p = buf;
> +	while ((len = read(iter_fd, p, left)) > 0) {
> +		p += len;
> +		left -= len;
> +	}
> +
> +	ASSERT_STREQ(buf, expected_output, "dead cgroup output");
> +
> +	/* read() after iter finishes should be ok. */
> +	if (len == 0)
> +		ASSERT_OK(read(iter_fd, buf, sizeof(buf)), "second_read");
> +
> +	close(iter_fd);
> +free_link:
> +	bpf_link__destroy(link);
> +close_cg:
> +	if (cgrp_fd >= 0)
> +		close(cgrp_fd);
> +}
> +
>   void test_cgroup_iter(void)
>   {
>   	struct cgroup_iter *skel = NULL;
> @@ -217,6 +293,8 @@ void test_cgroup_iter(void)
>   		test_early_termination(skel);
>   	if (test__start_subtest("cgroup_iter__self_only"))
>   		test_walk_self_only(skel);
> +	if (test__start_subtest("cgroup_iter_dead_self_only"))

Let us follow the convention in this file with
	cgroup_iter__dead_self_only

> +		test_walk_dead_self_only(skel);
>   out:
>   	cgroup_iter__destroy(skel);
>   	cleanup_cgroups();

Re: [PATCH bpf 0/3] Pin the start cgroup for cgroup iterator

[Yonghong Song]

On 11/6/22 11:42 PM, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> Hi,
> 
> The patchset tries to fix the potential use-after-free problem in cgroup
> iterator. The problem is similar with the UAF problem fixed in map
> iterator and the fixes is also similar: pinning the iterated resource
> in .init_seq_private() and unpinning in .fini_seq_private(). Also adding
> a test to demonstrate the problem.
> 
> Not sure whether or not it will be helpful to add some comments for
> .init_seq_private() to state that the implementation of
> .init_seq_private() should not depend on iterator link to guarantee
> the liveness of iterated object. Comments are always welcome.

You added some comments in cgroup_iter init_seq_private(). Hopefully
that can serve as an example so for future iterators we can search
the code and remember to hold necessary references in init_seq_private()
function....

> 
> Hou Tao (3):
>    bpf: Pin the start cgroup in cgroup_iter_seq_init()
>    selftests/bpf: Add cgroup helper remove_cgroup()
>    selftests/bpf: Add test for cgroup iterator on a dead cgroup
> 
>   kernel/bpf/cgroup_iter.c                      | 14 ++++
>   tools/testing/selftests/bpf/cgroup_helpers.c  | 19 +++++
>   tools/testing/selftests/bpf/cgroup_helpers.h  |  1 +
>   .../selftests/bpf/prog_tests/cgroup_iter.c    | 78 +++++++++++++++++++
>   4 files changed, 112 insertions(+)
> 

Re: [PATCH bpf 3/3] selftests/bpf: Add test for cgroup iterator on a dead cgroup

[Hou Tao]

Hi,

On 11/8/2022 6:44 AM, Yonghong Song wrote:
>
>
> On 11/6/22 11:42 PM, Hou Tao wrote:
>> From: Hou Tao <houtao1@huawei.com>
>>
>> The test closes both iterator link fd and cgroup fd, and removes the
>> cgroup file to make a dead cgroup before reading cgroup iterator fd. It
>> also uses kern_sync_rcu() and usleep() to wait for the release of
>> start cgroup. If the start cgroup is not pinned by cgroup iterator,
>> reading iterator fd will trigger use-after-free.
>>
>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>
> LGTM with a few nits below.
>
> Acked-by: Yonghong Song <yhs@fb.com>
SNIP
>
>> +    cgrp_fd = create_and_get_cgroup(cgrp_name);
>> +    if (!ASSERT_GE(cgrp_fd, 0, "create cgrp"))
>> +        return;
>> +
>> +    /* The cgroup is already dead during iteration, so it only has epilogue
>> +     * in the output.
>> +     */
>
> Let us reword the comment like
>     The cgroup will be dead during read() iteration, and it only has
>     epilogue in the output.
Will do in v2.
>
>> +    snprintf(expected_output, sizeof(expected_output), EPILOGUE);
>> +
>> +    memset(&linfo, 0, sizeof(linfo));
>> +    linfo.cgroup.cgroup_fd = cgrp_fd;
>> +    linfo.cgroup.order = BPF_CGROUP_ITER_SELF_ONLY;
>> +    opts.link_info = &linfo;
>> +    opts.link_info_len = sizeof(linfo);
>> +
SNIP
>>   void test_cgroup_iter(void)
>>   {
>>       struct cgroup_iter *skel = NULL;
>> @@ -217,6 +293,8 @@ void test_cgroup_iter(void)
>>           test_early_termination(skel);
>>       if (test__start_subtest("cgroup_iter__self_only"))
>>           test_walk_self_only(skel);
>> +    if (test__start_subtest("cgroup_iter_dead_self_only"))
>
> Let us follow the convention in this file with
>     cgroup_iter__dead_self_only
My bad. Will fixes in v2.
>
>> +        test_walk_dead_self_only(skel);
>>   out:
>>       cgroup_iter__destroy(skel);
>>       cleanup_cgroups();

Re: [PATCH bpf 0/3] Pin the start cgroup for cgroup iterator

[Hou Tao]

Hi,

On 11/8/2022 6:51 AM, Yonghong Song wrote:
>
>
> On 11/6/22 11:42 PM, Hou Tao wrote:
>> From: Hou Tao <houtao1@huawei.com>
>>
>> Hi,
>>
>> The patchset tries to fix the potential use-after-free problem in cgroup
>> iterator. The problem is similar with the UAF problem fixed in map
>> iterator and the fixes is also similar: pinning the iterated resource
>> in .init_seq_private() and unpinning in .fini_seq_private(). Also adding
>> a test to demonstrate the problem.
>>
>> Not sure whether or not it will be helpful to add some comments for
>> .init_seq_private() to state that the implementation of
>> .init_seq_private() should not depend on iterator link to guarantee
>> the liveness of iterated object. Comments are always welcome.
>
> You added some comments in cgroup_iter init_seq_private(). Hopefully
> that can serve as an example so for future iterators we can search
> the code and remember to hold necessary references in init_seq_private()
> function....
Another way to prevent such problem is to pin iterator link in iterator fd, but
it introduce unnecessary dependency as said before, so hope the comments will be
helpful.
>
>>
>> Hou Tao (3):
>>    bpf: Pin the start cgroup in cgroup_iter_seq_init()
>>    selftests/bpf: Add cgroup helper remove_cgroup()
>>    selftests/bpf: Add test for cgroup iterator on a dead cgroup
>>
>>   kernel/bpf/cgroup_iter.c                      | 14 ++++
>>   tools/testing/selftests/bpf/cgroup_helpers.c  | 19 +++++
>>   tools/testing/selftests/bpf/cgroup_helpers.h  |  1 +
>>   .../selftests/bpf/prog_tests/cgroup_iter.c    | 78 +++++++++++++++++++
>>   4 files changed, 112 insertions(+)
>>
>
> .

Re: [PATCH bpf 1/3] bpf: Pin the start cgroup in cgroup_iter_seq_init()

[Hao Luo]

On Mon, Nov 7, 2022 at 1:59 PM Yonghong Song <yhs@meta.com> wrote:
>
>
>
> On 11/6/22 11:42 PM, Hou Tao wrote:
> > From: Hou Tao <houtao1@huawei.com>
> >
> > bpf_iter_attach_cgroup() has already acquired an extra reference for the
> > start cgroup, but the reference will be released if the iterator link fd
> > is closed after the creation of iterator fd, and it may lead to
> > User-After-Free when reading the iterator fd.
> >
> > So fixing it by acquiring another reference for the start cgroup.
> >
> > Fixes: d4ccaf58a847 ("bpf: Introduce cgroup iter")
> > Signed-off-by: Hou Tao <houtao1@huawei.com>
>
> Acked-by: Yonghong Song <yhs@fb.com>

There is an alternative: does it make sense to have the iterator hold
a ref of the link? When the link is closed, my assumption is that the
program is already detached from the cgroup. After that, it makes no
sense to still allow iterating the cgroup. IIUC, holding a ref to the
link in the iterator also fixes for other types of objects.

Hao

Re: [PATCH bpf 1/3] bpf: Pin the start cgroup in cgroup_iter_seq_init()

[Hou Tao]

Hi,

On 11/8/2022 10:11 AM, Hao Luo wrote:
> On Mon, Nov 7, 2022 at 1:59 PM Yonghong Song <yhs@meta.com> wrote:
>>
>>
>> On 11/6/22 11:42 PM, Hou Tao wrote:
>>> From: Hou Tao <houtao1@huawei.com>
>>>
>>> bpf_iter_attach_cgroup() has already acquired an extra reference for the
>>> start cgroup, but the reference will be released if the iterator link fd
>>> is closed after the creation of iterator fd, and it may lead to
>>> User-After-Free when reading the iterator fd.
>>>
>>> So fixing it by acquiring another reference for the start cgroup.
>>>
>>> Fixes: d4ccaf58a847 ("bpf: Introduce cgroup iter")
>>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>> Acked-by: Yonghong Song <yhs@fb.com>
> There is an alternative: does it make sense to have the iterator hold
> a ref of the link? When the link is closed, my assumption is that the
> program is already detached from the cgroup. After that, it makes no
> sense to still allow iterating the cgroup. IIUC, holding a ref to the
> link in the iterator also fixes for other types of objects.
Also considered the alternative solution when fixing the similar problem in bpf
map element iterator [0]. The problem is not all of bpf iterators need the
pinning (e.g., bpf map iterator). Because bpf prog is also pinned by iterator fd
in iter_open(), so closing the fd of iterator link doesn't release the bpf program.

[0]: https://lore.kernel.org/bpf/20220810080538.1845898-2-houtao@huaweicloud.com/
>
> Hao

Re: [PATCH bpf 1/3] bpf: Pin the start cgroup in cgroup_iter_seq_init()

[Yonghong Song]

On 11/7/22 8:08 PM, Hou Tao wrote:
> Hi,
> 
> On 11/8/2022 10:11 AM, Hao Luo wrote:
>> On Mon, Nov 7, 2022 at 1:59 PM Yonghong Song <yhs@meta.com> wrote:
>>>
>>>
>>> On 11/6/22 11:42 PM, Hou Tao wrote:
>>>> From: Hou Tao <houtao1@huawei.com>
>>>>
>>>> bpf_iter_attach_cgroup() has already acquired an extra reference for the
>>>> start cgroup, but the reference will be released if the iterator link fd
>>>> is closed after the creation of iterator fd, and it may lead to
>>>> User-After-Free when reading the iterator fd.
>>>>
>>>> So fixing it by acquiring another reference for the start cgroup.
>>>>
>>>> Fixes: d4ccaf58a847 ("bpf: Introduce cgroup iter")
>>>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>>> Acked-by: Yonghong Song <yhs@fb.com>
>> There is an alternative: does it make sense to have the iterator hold
>> a ref of the link? When the link is closed, my assumption is that the
>> program is already detached from the cgroup. After that, it makes no
>> sense to still allow iterating the cgroup. IIUC, holding a ref to the
>> link in the iterator also fixes for other types of objects.
> Also considered the alternative solution when fixing the similar problem in bpf
> map element iterator [0]. The problem is not all of bpf iterators need the
> pinning (e.g., bpf map iterator). Because bpf prog is also pinned by iterator fd
> in iter_open(), so closing the fd of iterator link doesn't release the bpf program.
> 
> [0]: https://lore.kernel.org/bpf/20220810080538.1845898-2-houtao@huaweicloud.com/

Okay, let us do the solution to hold a reference to the link for the 
iterator. For cgroup_iter, that means, both prog and cgroup will be 
present so we should be okay then.

>>
>> Hao
> 

Re: [PATCH bpf 1/3] bpf: Pin the start cgroup in cgroup_iter_seq_init()

[Hou Tao]

Hi,

On 11/8/2022 3:03 PM, Yonghong Song wrote:
>
>
> On 11/7/22 8:08 PM, Hou Tao wrote:
>> Hi,
>>
>> On 11/8/2022 10:11 AM, Hao Luo wrote:
>>> On Mon, Nov 7, 2022 at 1:59 PM Yonghong Song <yhs@meta.com> wrote:
>>>>
>>>>
>>>> On 11/6/22 11:42 PM, Hou Tao wrote:
>>>>> From: Hou Tao <houtao1@huawei.com>
>>>>>
>>>>> bpf_iter_attach_cgroup() has already acquired an extra reference for the
>>>>> start cgroup, but the reference will be released if the iterator link fd
>>>>> is closed after the creation of iterator fd, and it may lead to
>>>>> User-After-Free when reading the iterator fd.
>>>>>
>>>>> So fixing it by acquiring another reference for the start cgroup.
>>>>>
>>>>> Fixes: d4ccaf58a847 ("bpf: Introduce cgroup iter")
>>>>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>>>> Acked-by: Yonghong Song <yhs@fb.com>
>>> There is an alternative: does it make sense to have the iterator hold
>>> a ref of the link? When the link is closed, my assumption is that the
>>> program is already detached from the cgroup. After that, it makes no
>>> sense to still allow iterating the cgroup. IIUC, holding a ref to the
>>> link in the iterator also fixes for other types of objects.
>> Also considered the alternative solution when fixing the similar problem in bpf
>> map element iterator [0]. The problem is not all of bpf iterators need the
>> pinning (e.g., bpf map iterator). Because bpf prog is also pinned by iterator fd
>> in iter_open(), so closing the fd of iterator link doesn't release the bpf
>> program.
>>
>> [0]: https://lore.kernel.org/bpf/20220810080538.1845898-2-houtao@huaweicloud.com/
>
> Okay, let us do the solution to hold a reference to the link for the iterator.
> For cgroup_iter, that means, both prog and cgroup will be present so we should
> be okay then.
The reason I did not use the solution is that it will create unnecessary
dependency between iterator fd and iterator link and many bpf iterators also
don't need that. If we use the solution, should I revert the fixes to bpf map
iterator done before or keep it as-is ?
>
>>>
>>> Hao
>>

Re: [PATCH bpf 1/3] bpf: Pin the start cgroup in cgroup_iter_seq_init()

[Yonghong Song]

On 11/8/22 5:28 AM, Hou Tao wrote:
> Hi,
> 
> On 11/8/2022 3:03 PM, Yonghong Song wrote:
>>
>>
>> On 11/7/22 8:08 PM, Hou Tao wrote:
>>> Hi,
>>>
>>> On 11/8/2022 10:11 AM, Hao Luo wrote:
>>>> On Mon, Nov 7, 2022 at 1:59 PM Yonghong Song <yhs@meta.com> wrote:
>>>>>
>>>>>
>>>>> On 11/6/22 11:42 PM, Hou Tao wrote:
>>>>>> From: Hou Tao <houtao1@huawei.com>
>>>>>>
>>>>>> bpf_iter_attach_cgroup() has already acquired an extra reference for the
>>>>>> start cgroup, but the reference will be released if the iterator link fd
>>>>>> is closed after the creation of iterator fd, and it may lead to
>>>>>> User-After-Free when reading the iterator fd.
>>>>>>
>>>>>> So fixing it by acquiring another reference for the start cgroup.
>>>>>>
>>>>>> Fixes: d4ccaf58a847 ("bpf: Introduce cgroup iter")
>>>>>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>>>>> Acked-by: Yonghong Song <yhs@fb.com>
>>>> There is an alternative: does it make sense to have the iterator hold
>>>> a ref of the link? When the link is closed, my assumption is that the
>>>> program is already detached from the cgroup. After that, it makes no
>>>> sense to still allow iterating the cgroup. IIUC, holding a ref to the
>>>> link in the iterator also fixes for other types of objects.
>>> Also considered the alternative solution when fixing the similar problem in bpf
>>> map element iterator [0]. The problem is not all of bpf iterators need the
>>> pinning (e.g., bpf map iterator). Because bpf prog is also pinned by iterator fd
>>> in iter_open(), so closing the fd of iterator link doesn't release the bpf
>>> program.
>>>
>>> [0]: https://lore.kernel.org/bpf/20220810080538.1845898-2-houtao@huaweicloud.com/
>>
>> Okay, let us do the solution to hold a reference to the link for the iterator.
>> For cgroup_iter, that means, both prog and cgroup will be present so we should
>> be okay then.
> The reason I did not use the solution is that it will create unnecessary
> dependency between iterator fd and iterator link and many bpf iterators also
> don't need that. If we use the solution, should I revert the fixes to bpf map
> iterator done before or keep it as-is ?

Let us do it separately. This is a bug fix (targeting bpf tree). map 
iterator issue has been fixed and we can leave it there or change to 
hold a reference to the link. Personally I think we can leave it as is
since it is working.

>>
>>>>
>>>> Hao
>>>
> 

Re: [PATCH bpf 1/3] bpf: Pin the start cgroup in cgroup_iter_seq_init()

[Hao Luo]

On Tue, Nov 8, 2022 at 5:28 AM Hou Tao <houtao@huaweicloud.com> wrote:
> On 11/8/2022 3:03 PM, Yonghong Song wrote:
> > On 11/7/22 8:08 PM, Hou Tao wrote:
> >> On 11/8/2022 10:11 AM, Hao Luo wrote:
<...>
> >>> There is an alternative: does it make sense to have the iterator hold
> >>> a ref of the link? When the link is closed, my assumption is that the
> >>> program is already detached from the cgroup. After that, it makes no
> >>> sense to still allow iterating the cgroup. IIUC, holding a ref to the
> >>> link in the iterator also fixes for other types of objects.
> >>
> >> Also considered the alternative solution when fixing the similar problem in bpf
> >> map element iterator [0]. The problem is not all of bpf iterators need the
> >> pinning (e.g., bpf map iterator). Because bpf prog is also pinned by iterator fd
> >> in iter_open(), so closing the fd of iterator link doesn't release the bpf
> >> program.
> >>
> >> [0]: https://lore.kernel.org/bpf/20220810080538.1845898-2-houtao@huaweicloud.com/
> >
> > Okay, let us do the solution to hold a reference to the link for the iterator.
> > For cgroup_iter, that means, both prog and cgroup will be present so we should
> > be okay then.
> >
> The reason I did not use the solution is that it will create unnecessary
> dependency between iterator fd and iterator link and many bpf iterators also
> don't need that. If we use the solution, should I revert the fixes to bpf map
> iterator done before or keep it as-is ?
> >

Hou Tao, on the contrary, I do think the dependency is necessary. My
understanding is, the lifetime of an iterator should not go beyond the
lifetime of the link who generates the iterator.

You mention that many bpf iterators don't need that. I suspect there
are bugs due to lack of such dependencies. Hypothetically, if the link
is released, it may cause the program to be released as well. Then,
how could we still iterate the objects and call the program on the
objects? Please correct me if there is anything I missed.

Re: [PATCH bpf 1/3] bpf: Pin the start cgroup in cgroup_iter_seq_init()

[Hou Tao]

Hi,

On 11/9/2022 12:19 AM, Yonghong Song wrote:
>
>
> On 11/8/22 5:28 AM, Hou Tao wrote:
>> Hi,
>>
>> On 11/8/2022 3:03 PM, Yonghong Song wrote:
>>>
>>>
>>> On 11/7/22 8:08 PM, Hou Tao wrote:
>>>> Hi,
>>>>
>>>> On 11/8/2022 10:11 AM, Hao Luo wrote:
>>>>> On Mon, Nov 7, 2022 at 1:59 PM Yonghong Song <yhs@meta.com> wrote:
>>>>>>
>>>>>>
>>>>>> On 11/6/22 11:42 PM, Hou Tao wrote:
>>>>>>> From: Hou Tao <houtao1@huawei.com>
>>>>>>>
>>>>>>> bpf_iter_attach_cgroup() has already acquired an extra reference for the
>>>>>>> start cgroup, but the reference will be released if the iterator link fd
>>>>>>> is closed after the creation of iterator fd, and it may lead to
>>>>>>> User-After-Free when reading the iterator fd.
>>>>>>>
>>>>>>> So fixing it by acquiring another reference for the start cgroup.
>>>>>>>
>>>>>>> Fixes: d4ccaf58a847 ("bpf: Introduce cgroup iter")
>>>>>>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>>>>>> Acked-by: Yonghong Song <yhs@fb.com>
>>>>> There is an alternative: does it make sense to have the iterator hold
>>>>> a ref of the link? When the link is closed, my assumption is that the
>>>>> program is already detached from the cgroup. After that, it makes no
>>>>> sense to still allow iterating the cgroup. IIUC, holding a ref to the
>>>>> link in the iterator also fixes for other types of objects.
>>>> Also considered the alternative solution when fixing the similar problem in
>>>> bpf
>>>> map element iterator [0]. The problem is not all of bpf iterators need the
>>>> pinning (e.g., bpf map iterator). Because bpf prog is also pinned by
>>>> iterator fd
>>>> in iter_open(), so closing the fd of iterator link doesn't release the bpf
>>>> program.
>>>>
>>>> [0]:
>>>> https://lore.kernel.org/bpf/20220810080538.1845898-2-houtao@huaweicloud.com/
>>>
>>> Okay, let us do the solution to hold a reference to the link for the iterator.
>>> For cgroup_iter, that means, both prog and cgroup will be present so we should
>>> be okay then.
>> The reason I did not use the solution is that it will create unnecessary
>> dependency between iterator fd and iterator link and many bpf iterators also
>> don't need that. If we use the solution, should I revert the fixes to bpf map
>> iterator done before or keep it as-is ?
>
> Let us do it separately. This is a bug fix (targeting bpf tree). map iterator
> issue has been fixed and we can leave it there or change to hold a reference
> to the link. Personally I think we can leave it as is
> since it is working.
OK. I will keep it as is.
>
>>>
>>>>>
>>>>> Hao
>>>>
>>