[PATCH bpf v2 0/3] Pin iterator link when opening iterator

[PATCH bpf v2 0/3] Pin iterator link when opening 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, however to prevent such UAF problem from happening again for
bpf iterator, just pinning iterator link when opening iterator, so after
doing the necessary reference acquisitions in .attach_target() there will
be unnecessary to pin iteration target again in .init_seq_private() for
each iterator type. Also adding a selftests to demonstrate the UAF
problem when iterating a dead cgroup.

Comments are always welcome.

Change Log:
v2:
 * Patch 1: Pinning iterator link when opening iterator, instead of
   acquiring the reference of start cgroup in cgroup_iter_seq_init().
 * Patch 2 & 3: Address comments from Yonghong Song and add Acked-by tag

v1: https://lore.kernel.org/bpf/20221107074222.1323017-1-houtao@huaweicloud.com/T/#t

Hou Tao (3):
  bpf: Pin iterator link when opening iterator
  selftests/bpf: Add cgroup helper remove_cgroup()
  selftests/bpf: Add test for cgroup iterator on a dead cgroup

 kernel/bpf/bpf_iter.c                         | 21 +++--
 tools/testing/selftests/bpf/cgroup_helpers.c  | 19 +++++
 tools/testing/selftests/bpf/cgroup_helpers.h  |  1 +
 .../selftests/bpf/prog_tests/cgroup_iter.c    | 76 +++++++++++++++++++
 4 files changed, 110 insertions(+), 7 deletions(-)

-- 
2.29.2

[PATCH bpf v2 1/3] bpf: Pin iterator link when opening iterator

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

For many bpf iterator (e.g., cgroup iterator), iterator link acquires
the reference of iteration target in .attach_target(), but iterator link
may be closed before or in the middle of iteration, so iterator will
need to acquire the reference of iteration target as well to prevent
potential use-after-free. To avoid doing the acquisition in
.init_seq_private() for each iterator type, just pin iterator link in
iterator.

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

diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index 5dc307bdeaeb..67d899011cb2 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -20,7 +20,7 @@ struct bpf_iter_link {
 };
 
 struct bpf_iter_priv_data {
-	struct bpf_iter_target_info *tinfo;
+	struct bpf_iter_link *link;
 	const struct bpf_iter_seq_info *seq_info;
 	struct bpf_prog *prog;
 	u64 session_id;
@@ -79,7 +79,7 @@ static bool bpf_iter_support_resched(struct seq_file *seq)
 
 	iter_priv = container_of(seq->private, struct bpf_iter_priv_data,
 				 target_private);
-	return bpf_iter_target_support_resched(iter_priv->tinfo);
+	return bpf_iter_target_support_resched(iter_priv->link->tinfo);
 }
 
 /* maximum visited objects before bailing out */
@@ -276,6 +276,7 @@ static int iter_release(struct inode *inode, struct file *file)
 		iter_priv->seq_info->fini_seq_private(seq->private);
 
 	bpf_prog_put(iter_priv->prog);
+	bpf_link_put(&iter_priv->link->link);
 	seq->private = iter_priv;
 
 	return seq_release_private(inode, file);
@@ -576,11 +577,19 @@ int bpf_iter_link_attach(const union bpf_attr *attr, bpfptr_t uattr,
 }
 
 static void init_seq_meta(struct bpf_iter_priv_data *priv_data,
-			  struct bpf_iter_target_info *tinfo,
+			  struct bpf_iter_link *link,
 			  const struct bpf_iter_seq_info *seq_info,
 			  struct bpf_prog *prog)
 {
-	priv_data->tinfo = tinfo;
+	/* For many bpf iterator, iterator link acquires the reference of
+	 * iteration target in .attach_target(), but iterator link may be
+	 * closed before or in the middle of iteration, so iterator will
+	 * need to acquire the reference of iteration target as well. To
+	 * avoid doing the acquisition in .init_seq_private() for each
+	 * iterator type, just pin iterator link in iterator.
+	 */
+	bpf_link_inc(&link->link);
+	priv_data->link = link;
 	priv_data->seq_info = seq_info;
 	priv_data->prog = prog;
 	priv_data->session_id = atomic64_inc_return(&session_id);
@@ -592,7 +601,6 @@ static int prepare_seq_file(struct file *file, struct bpf_iter_link *link,
 			    const struct bpf_iter_seq_info *seq_info)
 {
 	struct bpf_iter_priv_data *priv_data;
-	struct bpf_iter_target_info *tinfo;
 	struct bpf_prog *prog;
 	u32 total_priv_dsize;
 	struct seq_file *seq;
@@ -603,7 +611,6 @@ static int prepare_seq_file(struct file *file, struct bpf_iter_link *link,
 	bpf_prog_inc(prog);
 	mutex_unlock(&link_mutex);
 
-	tinfo = link->tinfo;
 	total_priv_dsize = offsetof(struct bpf_iter_priv_data, target_private) +
 			   seq_info->seq_priv_size;
 	priv_data = __seq_open_private(file, seq_info->seq_ops,
@@ -619,7 +626,7 @@ static int prepare_seq_file(struct file *file, struct bpf_iter_link *link,
 			goto release_seq_file;
 	}
 
-	init_seq_meta(priv_data, tinfo, seq_info, prog);
+	init_seq_meta(priv_data, link, seq_info, prog);
 	seq = file->private_data;
 	seq->private = priv_data->target_private;
 
-- 
2.29.2

Re: [PATCH bpf v2 1/3] bpf: Pin iterator link when opening iterator

[Yonghong Song]

On 11/10/22 10:34 PM, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> For many bpf iterator (e.g., cgroup iterator), iterator link acquires
> the reference of iteration target in .attach_target(), but iterator link
> may be closed before or in the middle of iteration, so iterator will
> need to acquire the reference of iteration target as well to prevent
> potential use-after-free. To avoid doing the acquisition in
> .init_seq_private() for each iterator type, just pin iterator link in
> iterator.
> 
> Fixes: d4ccaf58a847 ("bpf: Introduce cgroup iter")
> Signed-off-by: Hou Tao <houtao1@huawei.com>

Thanks. LGTM. Once this patch went through bpf and circulated back to
bpf-next, you can revert the change for the following patches:
    f0d2b2716d71  bpf: Acquire map uref in .init_seq_private for 
sock{map,hash} iterator
    3c5f6e698b5c  bpf: Acquire map uref in .init_seq_private for sock 
local storage map iterator
    ef1e93d2eeb5  bpf: Acquire map uref in .init_seq_private for hash 
map iterator
    f76fa6b33805  bpf: Acquire map uref in .init_seq_private for array 
map iterator
in bpf-next.

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

Re: [PATCH bpf v2 1/3] bpf: Pin iterator link when opening iterator

[Hao Luo]

On Fri, Nov 11, 2022 at 8:31 AM Yonghong Song <yhs@meta.com> wrote:
>
>
>
> On 11/10/22 10:34 PM, Hou Tao wrote:
> > From: Hou Tao <houtao1@huawei.com>
> >
> > For many bpf iterator (e.g., cgroup iterator), iterator link acquires
> > the reference of iteration target in .attach_target(), but iterator link
> > may be closed before or in the middle of iteration, so iterator will
> > need to acquire the reference of iteration target as well to prevent
> > potential use-after-free. To avoid doing the acquisition in
> > .init_seq_private() for each iterator type, just pin iterator link in
> > iterator.
> >
> > Fixes: d4ccaf58a847 ("bpf: Introduce cgroup iter")
> > Signed-off-by: Hou Tao <houtao1@huawei.com>
>
> Thanks. LGTM. Once this patch went through bpf and circulated back to
> bpf-next, you can revert the change for the following patches:
>     f0d2b2716d71  bpf: Acquire map uref in .init_seq_private for
> sock{map,hash} iterator
>     3c5f6e698b5c  bpf: Acquire map uref in .init_seq_private for sock
> local storage map iterator
>     ef1e93d2eeb5  bpf: Acquire map uref in .init_seq_private for hash
> map iterator
>     f76fa6b33805  bpf: Acquire map uref in .init_seq_private for array
> map iterator
> in bpf-next.
>
> Acked-by: Yonghong Song <yhs@fb.com>

Acked-by: Hao Luo <haoluo@google.com>

Thanks!

Re: [PATCH bpf v2 1/3] bpf: Pin iterator link when opening iterator

[Martin KaFai Lau]

On 11/10/22 10:34 PM, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> For many bpf iterator (e.g., cgroup iterator), iterator link acquires
> the reference of iteration target in .attach_target(), but iterator link
> may be closed before or in the middle of iteration, so iterator will
> need to acquire the reference of iteration target as well to prevent
> potential use-after-free. To avoid doing the acquisition in
> .init_seq_private() for each iterator type, just pin iterator link in
> iterator.

iiuc, a link currently will go away when all its fds closed and pinned file 
removed.  After this change, the link will stay until the last iter is closed(). 
  Before then, the user space can still "bpftool link show" and even get the 
link back by bpf_link_get_fd_by_id().  If this is the case, it would be useful 
to explain it in the commit message.

and does this new behavior make sense when comparing with other link types?

> 
> Fixes: d4ccaf58a847 ("bpf: Introduce cgroup iter")
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>   kernel/bpf/bpf_iter.c | 21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
> index 5dc307bdeaeb..67d899011cb2 100644
> --- a/kernel/bpf/bpf_iter.c
> +++ b/kernel/bpf/bpf_iter.c
> @@ -20,7 +20,7 @@ struct bpf_iter_link {
>   };
>   
>   struct bpf_iter_priv_data {
> -	struct bpf_iter_target_info *tinfo;
> +	struct bpf_iter_link *link;
>   	const struct bpf_iter_seq_info *seq_info;
>   	struct bpf_prog *prog;
>   	u64 session_id;
> @@ -79,7 +79,7 @@ static bool bpf_iter_support_resched(struct seq_file *seq)
>   
>   	iter_priv = container_of(seq->private, struct bpf_iter_priv_data,
>   				 target_private);
> -	return bpf_iter_target_support_resched(iter_priv->tinfo);
> +	return bpf_iter_target_support_resched(iter_priv->link->tinfo);
>   }
>   
>   /* maximum visited objects before bailing out */
> @@ -276,6 +276,7 @@ static int iter_release(struct inode *inode, struct file *file)
>   		iter_priv->seq_info->fini_seq_private(seq->private);
>   
>   	bpf_prog_put(iter_priv->prog);
> +	bpf_link_put(&iter_priv->link->link);
>   	seq->private = iter_priv;
>   
>   	return seq_release_private(inode, file);
> @@ -576,11 +577,19 @@ int bpf_iter_link_attach(const union bpf_attr *attr, bpfptr_t uattr,
>   }
>   
>   static void init_seq_meta(struct bpf_iter_priv_data *priv_data,
> -			  struct bpf_iter_target_info *tinfo,
> +			  struct bpf_iter_link *link,
>   			  const struct bpf_iter_seq_info *seq_info,
>   			  struct bpf_prog *prog)
>   {
> -	priv_data->tinfo = tinfo;
> +	/* For many bpf iterator, iterator link acquires the reference of
> +	 * iteration target in .attach_target(), but iterator link may be
> +	 * closed before or in the middle of iteration, so iterator will
> +	 * need to acquire the reference of iteration target as well. To
> +	 * avoid doing the acquisition in .init_seq_private() for each
> +	 * iterator type, just pin iterator link in iterator.
> +	 */
> +	bpf_link_inc(&link->link);
> +	priv_data->link = link;
>   	priv_data->seq_info = seq_info;
>   	priv_data->prog = prog;
>   	priv_data->session_id = atomic64_inc_return(&session_id);
> @@ -592,7 +601,6 @@ static int prepare_seq_file(struct file *file, struct bpf_iter_link *link,
>   			    const struct bpf_iter_seq_info *seq_info)
>   {
>   	struct bpf_iter_priv_data *priv_data;
> -	struct bpf_iter_target_info *tinfo;
>   	struct bpf_prog *prog;
>   	u32 total_priv_dsize;
>   	struct seq_file *seq;
> @@ -603,7 +611,6 @@ static int prepare_seq_file(struct file *file, struct bpf_iter_link *link,
>   	bpf_prog_inc(prog);
>   	mutex_unlock(&link_mutex);
>   
> -	tinfo = link->tinfo;
>   	total_priv_dsize = offsetof(struct bpf_iter_priv_data, target_private) +
>   			   seq_info->seq_priv_size;
>   	priv_data = __seq_open_private(file, seq_info->seq_ops,
> @@ -619,7 +626,7 @@ static int prepare_seq_file(struct file *file, struct bpf_iter_link *link,
>   			goto release_seq_file;
>   	}
>   
> -	init_seq_meta(priv_data, tinfo, seq_info, prog);
> +	init_seq_meta(priv_data, link, seq_info, prog);
>   	seq = file->private_data;
>   	seq->private = priv_data->target_private;
>   

Re: [PATCH bpf v2 1/3] bpf: Pin iterator link when opening iterator

[Alexei Starovoitov]

On Tue, Nov 15, 2022 at 11:16 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 11/10/22 10:34 PM, Hou Tao wrote:
> > From: Hou Tao <houtao1@huawei.com>
> >
> > For many bpf iterator (e.g., cgroup iterator), iterator link acquires
> > the reference of iteration target in .attach_target(), but iterator link
> > may be closed before or in the middle of iteration, so iterator will
> > need to acquire the reference of iteration target as well to prevent
> > potential use-after-free. To avoid doing the acquisition in
> > .init_seq_private() for each iterator type, just pin iterator link in
> > iterator.
>
> iiuc, a link currently will go away when all its fds closed and pinned file
> removed.  After this change, the link will stay until the last iter is closed().
>   Before then, the user space can still "bpftool link show" and even get the
> link back by bpf_link_get_fd_by_id().  If this is the case, it would be useful
> to explain it in the commit message.
>
> and does this new behavior make sense when comparing with other link types?

One more question to the above...

Does this change mean that pinned cgroup iterator in bpffs
would prevent cgroup removal?
So that cgroup cannot even become a dying cgroup ?

If so we can do that and an approach similar to init_seq_private
taken for map iterators is necessary here as well.

Also pls target this kind of change to bpf-next especially
when there is a consideration to revert other fixes.
This kind of questionable fixes are not suitable for bpf tree
regardless of how long the "bug" was present.

Re: [PATCH bpf v2 1/3] bpf: Pin iterator link when opening iterator

[Hou Tao]

Hi,

On 11/16/2022 9:37 AM, Alexei Starovoitov wrote:
> On Tue, Nov 15, 2022 at 11:16 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>> On 11/10/22 10:34 PM, Hou Tao wrote:
>>> From: Hou Tao <houtao1@huawei.com>
>>>
>>> For many bpf iterator (e.g., cgroup iterator), iterator link acquires
>>> the reference of iteration target in .attach_target(), but iterator link
>>> may be closed before or in the middle of iteration, so iterator will
>>> need to acquire the reference of iteration target as well to prevent
>>> potential use-after-free. To avoid doing the acquisition in
>>> .init_seq_private() for each iterator type, just pin iterator link in
>>> iterator.
>> iiuc, a link currently will go away when all its fds closed and pinned file
>> removed.  After this change, the link will stay until the last iter is closed().
>>   Before then, the user space can still "bpftool link show" and even get the
>> link back by bpf_link_get_fd_by_id().  If this is the case, it would be useful
>> to explain it in the commit message.
>>
>> and does this new behavior make sense when comparing with other link types?
> One more question to the above...
>
> Does this change mean that pinned cgroup iterator in bpffs
> would prevent cgroup removal?
> So that cgroup cannot even become a dying cgroup ?
No. The pinned cgroup still can be removed by rmdir and it is demonstrated in
the added test case. Getting an extra reference of cgroup just delays the
freeing of cgroup. And the change doesn't effect the pinned iterator in bpffs,
because when pinning the iterator in bpffs, it has already pinned the iterator link.
> If so we can do that and an approach similar to init_seq_private
> taken for map iterators is necessary here as well.
Do you mean pinning the cgroup in .init_seq_private() instead of pinning the
iterator link just like v1 does ?
>
> Also pls target this kind of change to bpf-next especially
> when there is a consideration to revert other fixes.
> This kind of questionable fixes are not suitable for bpf tree
> regardless of how long the "bug" was present.
The reason to post the fix to bpf tree instead bpf-next is that cgroup iterator
is merged in v6.1 and I think it is better to merge the fix into v6.1 instead of
v6.2. And patchset v1 is not a questionable fixes, because iterator link has
already acquired the reference of the start cgroup.

Re: [PATCH bpf v2 1/3] bpf: Pin iterator link when opening iterator

[Alexei Starovoitov]

On Tue, Nov 15, 2022 at 6:40 PM Hou Tao <houtao@huaweicloud.com> wrote:
> >
> > Also pls target this kind of change to bpf-next especially
> > when there is a consideration to revert other fixes.
> > This kind of questionable fixes are not suitable for bpf tree
> > regardless of how long the "bug" was present.
> The reason to post the fix to bpf tree instead bpf-next is that cgroup iterator
> is merged in v6.1 and I think it is better to merge the fix into v6.1 instead of
> v6.2. And patchset v1 is not a questionable fixes, because iterator link has
> already acquired the reference of the start cgroup.

These "fixes are not suitable for bpf tree regardless of how long the
"bug" was present".

Re: [PATCH bpf v2 1/3] bpf: Pin iterator link when opening iterator

[Hou Tao]

Hi,

On 11/16/2022 1:43 PM, Alexei Starovoitov wrote:
> On Tue, Nov 15, 2022 at 6:40 PM Hou Tao <houtao@huaweicloud.com> wrote:
>>> Also pls target this kind of change to bpf-next especially
>>> when there is a consideration to revert other fixes.
>>> This kind of questionable fixes are not suitable for bpf tree
>>> regardless of how long the "bug" was present.
>> The reason to post the fix to bpf tree instead bpf-next is that cgroup iterator
>> is merged in v6.1 and I think it is better to merge the fix into v6.1 instead of
>> v6.2. And patchset v1 is not a questionable fixes, because iterator link has
>> already acquired the reference of the start cgroup.
> These "fixes are not suitable for bpf tree regardless of how long the
> "bug" was present".
I see. Should I include the revert patches in v3 and resend it to bpf-next tree
? Or the patchset will be merged to bpf-next and I just need to send a follow-up
revert patchset to bpf-next ?
> .

Re: [PATCH bpf v2 1/3] bpf: Pin iterator link when opening iterator

[Hao Luo]

On Tue, Nov 15, 2022 at 5:37 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Nov 15, 2022 at 11:16 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
> >
> > On 11/10/22 10:34 PM, Hou Tao wrote:
> > > From: Hou Tao <houtao1@huawei.com>
> > >
> > > For many bpf iterator (e.g., cgroup iterator), iterator link acquires
> > > the reference of iteration target in .attach_target(), but iterator link
> > > may be closed before or in the middle of iteration, so iterator will
> > > need to acquire the reference of iteration target as well to prevent
> > > potential use-after-free. To avoid doing the acquisition in
> > > .init_seq_private() for each iterator type, just pin iterator link in
> > > iterator.
> >
> > iiuc, a link currently will go away when all its fds closed and pinned file
> > removed.  After this change, the link will stay until the last iter is closed().
> >   Before then, the user space can still "bpftool link show" and even get the
> > link back by bpf_link_get_fd_by_id().  If this is the case, it would be useful
> > to explain it in the commit message.
> >
> > and does this new behavior make sense when comparing with other link types?

I think this is a unique problem in iter link. Because iter link is
the only link type that can generate another object.

>
> One more question to the above...
>
> Does this change mean that pinned cgroup iterator in bpffs
> would prevent cgroup removal?

Yes, when attaching the program to cgroup, the cgroup iter link gets
an extra ref of the cgroup. It puts that ref when detach.

> So that cgroup cannot even become a dying cgroup ?
>

No. The cgroup will become offline and its corresponding kernfs node
will be removed. The cgroup object is still accessible.

> If so we can do that and an approach similar to init_seq_private
> taken for map iterators is necessary here as well.
>
> Also pls target this kind of change to bpf-next especially
> when there is a consideration to revert other fixes.
> This kind of questionable fixes are not suitable for bpf tree
> regardless of how long the "bug" was present.

Re: [PATCH bpf v2 1/3] bpf: Pin iterator link when opening iterator

[Hou Tao]

Hi,

On 11/16/2022 10:48 AM, Hao Luo wrote:
> On Tue, Nov 15, 2022 at 5:37 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> On Tue, Nov 15, 2022 at 11:16 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>> On 11/10/22 10:34 PM, Hou Tao wrote:
>>>> From: Hou Tao <houtao1@huawei.com>
>>>>
>>>> For many bpf iterator (e.g., cgroup iterator), iterator link acquires
>>>> the reference of iteration target in .attach_target(), but iterator link
>>>> may be closed before or in the middle of iteration, so iterator will
>>>> need to acquire the reference of iteration target as well to prevent
>>>> potential use-after-free. To avoid doing the acquisition in
>>>> .init_seq_private() for each iterator type, just pin iterator link in
>>>> iterator.
>>> iiuc, a link currently will go away when all its fds closed and pinned file
>>> removed.  After this change, the link will stay until the last iter is closed().
>>>   Before then, the user space can still "bpftool link show" and even get the
>>> link back by bpf_link_get_fd_by_id().  If this is the case, it would be useful
>>> to explain it in the commit message.
>>>
>>> and does this new behavior make sense when comparing with other link types?
> I think this is a unique problem in iter link. Because iter link is
> the only link type that can generate another object.
>
>> One more question to the above...
>>
>> Does this change mean that pinned cgroup iterator in bpffs
>> would prevent cgroup removal?
> Yes, when attaching the program to cgroup, the cgroup iter link gets
> an extra ref of the cgroup. It puts that ref when detach.
It seems we can not move the pinning of cgroup into cgroup_iter_seq_init(),
because the representation of the cgroup could be a fd and the fd will not be
accessible when iterator link is pinned in bpffs.
>
>> So that cgroup cannot even become a dying cgroup ?
>>
> No. The cgroup will become offline and its corresponding kernfs node
> will be removed. The cgroup object is still accessible.
>
>> If so we can do that and an approach similar to init_seq_private
>> taken for map iterators is necessary here as well.
>>
>> Also pls target this kind of change to bpf-next especially
>> when there is a consideration to revert other fixes.
>> This kind of questionable fixes are not suitable for bpf tree
>> regardless of how long the "bug" was present.
> .

Re: [PATCH bpf v2 1/3] bpf: Pin iterator link when opening iterator

[Martin KaFai Lau]

On 11/15/22 6:48 PM, Hao Luo wrote:
> On Tue, Nov 15, 2022 at 5:37 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> On Tue, Nov 15, 2022 at 11:16 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>>
>>> On 11/10/22 10:34 PM, Hou Tao wrote:
>>>> From: Hou Tao <houtao1@huawei.com>
>>>>
>>>> For many bpf iterator (e.g., cgroup iterator), iterator link acquires
>>>> the reference of iteration target in .attach_target(), but iterator link
>>>> may be closed before or in the middle of iteration, so iterator will
>>>> need to acquire the reference of iteration target as well to prevent
>>>> potential use-after-free. To avoid doing the acquisition in
>>>> .init_seq_private() for each iterator type, just pin iterator link in
>>>> iterator.
>>>
>>> iiuc, a link currently will go away when all its fds closed and pinned file
>>> removed.  After this change, the link will stay until the last iter is closed().
>>>    Before then, the user space can still "bpftool link show" and even get the
>>> link back by bpf_link_get_fd_by_id().  If this is the case, it would be useful
>>> to explain it in the commit message.
>>>
>>> and does this new behavior make sense when comparing with other link types?
> 
> I think this is a unique problem in iter link. Because iter link is
> the only link type that can generate another object.

Should a similar solution as in the current map iter be used then?

I am thinking, after all link fds are closed and its pinned files are removed, 
if it cannot stop the already created iter, it should at least stop new iter 
from being created?

Re: [PATCH bpf v2 1/3] bpf: Pin iterator link when opening iterator

[Hou Tao]

Hi,

On 11/17/2022 2:48 PM, Martin KaFai Lau wrote:
> On 11/15/22 6:48 PM, Hao Luo wrote:
>> On Tue, Nov 15, 2022 at 5:37 PM Alexei Starovoitov
>> <alexei.starovoitov@gmail.com> wrote:
>>>
>>> On Tue, Nov 15, 2022 at 11:16 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>>>
>>>> On 11/10/22 10:34 PM, Hou Tao wrote:
>>>>> From: Hou Tao <houtao1@huawei.com>
>>>>>
>>>>> For many bpf iterator (e.g., cgroup iterator), iterator link acquires
>>>>> the reference of iteration target in .attach_target(), but iterator link
>>>>> may be closed before or in the middle of iteration, so iterator will
>>>>> need to acquire the reference of iteration target as well to prevent
>>>>> potential use-after-free. To avoid doing the acquisition in
>>>>> .init_seq_private() for each iterator type, just pin iterator link in
>>>>> iterator.
>>>>
>>>> iiuc, a link currently will go away when all its fds closed and pinned file
>>>> removed.  After this change, the link will stay until the last iter is
>>>> closed().
>>>>    Before then, the user space can still "bpftool link show" and even get the
>>>> link back by bpf_link_get_fd_by_id().  If this is the case, it would be useful
>>>> to explain it in the commit message.
>>>>
>>>> and does this new behavior make sense when comparing with other link types?
>>
>> I think this is a unique problem in iter link. Because iter link is
>> the only link type that can generate another object.
>
> Should a similar solution as in the current map iter be used then?
>
> I am thinking, after all link fds are closed and its pinned files are removed,
> if it cannot stop the already created iter, it should at least stop new iter
> from being created?
Rather than adding the above logic for iterator link, just pinning the start
cgroup in .init_seq_private() will be much simpler.

And Hao worried about the close of iterator link fd will release the attached
program and cause use-after-free problem, but it is not true, because
prepare_seq_file() has already acquired an extra reference of the currently
attached program, so it is OK to read the iterator after the close of iterator
link fd. So what do you think ?
>
>
>
> .

Re: [PATCH bpf v2 1/3] bpf: Pin iterator link when opening iterator

[Martin KaFai Lau]

On 11/17/22 5:52 PM, Hou Tao wrote:
> Hi,
> 
> On 11/17/2022 2:48 PM, Martin KaFai Lau wrote:
>> On 11/15/22 6:48 PM, Hao Luo wrote:
>>> On Tue, Nov 15, 2022 at 5:37 PM Alexei Starovoitov
>>> <alexei.starovoitov@gmail.com> wrote:
>>>>
>>>> On Tue, Nov 15, 2022 at 11:16 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>>>>
>>>>> On 11/10/22 10:34 PM, Hou Tao wrote:
>>>>>> From: Hou Tao <houtao1@huawei.com>
>>>>>>
>>>>>> For many bpf iterator (e.g., cgroup iterator), iterator link acquires
>>>>>> the reference of iteration target in .attach_target(), but iterator link
>>>>>> may be closed before or in the middle of iteration, so iterator will
>>>>>> need to acquire the reference of iteration target as well to prevent
>>>>>> potential use-after-free. To avoid doing the acquisition in
>>>>>> .init_seq_private() for each iterator type, just pin iterator link in
>>>>>> iterator.
>>>>>
>>>>> iiuc, a link currently will go away when all its fds closed and pinned file
>>>>> removed.  After this change, the link will stay until the last iter is
>>>>> closed().
>>>>>     Before then, the user space can still "bpftool link show" and even get the
>>>>> link back by bpf_link_get_fd_by_id().  If this is the case, it would be useful
>>>>> to explain it in the commit message.
>>>>>
>>>>> and does this new behavior make sense when comparing with other link types?
>>>
>>> I think this is a unique problem in iter link. Because iter link is
>>> the only link type that can generate another object.
>>
>> Should a similar solution as in the current map iter be used then?
>>
>> I am thinking, after all link fds are closed and its pinned files are removed,
>> if it cannot stop the already created iter, it should at least stop new iter
>> from being created?
> Rather than adding the above logic for iterator link, just pinning the start
> cgroup in .init_seq_private() will be much simpler.

Yeah, it is better to fix the bug without changing the behavior when all the 
link fds are closed and pinned files are removed.  In particular this will make 
the link iter works differently from other link types.

I can see pinning a link inside an iter is a generic solution for all iter types 
but lets continue to explore other ways to refactor this within the kernel if it 
is really needed instead of leaking it to the user space.  (not saying this 
refactoring belongs to the same patch set.  lets fix the current bug first.)

> prepare_seq_file() has already acquired an extra reference of the currently
> attached program, so it is OK to read the iterator after the close of iterator
> link fd. So what do you think ?

Right, it is my understanding also that the prog refcnt has been acquired during 
the iter creation.

Re: [PATCH bpf v2 1/3] bpf: Pin iterator link when opening iterator

[Hao Luo]

On Thu, Nov 17, 2022 at 11:34 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 11/17/22 5:52 PM, Hou Tao wrote:
> > Hi,
> >
> > On 11/17/2022 2:48 PM, Martin KaFai Lau wrote:
<...>
> > Rather than adding the above logic for iterator link, just pinning the start
> > cgroup in .init_seq_private() will be much simpler.
>
> Yeah, it is better to fix the bug without changing the behavior when all the
> link fds are closed and pinned files are removed.  In particular this will make
> the link iter works differently from other link types.
>
> I can see pinning a link inside an iter is a generic solution for all iter types
> but lets continue to explore other ways to refactor this within the kernel if it
> is really needed instead of leaking it to the user space.  (not saying this
> refactoring belongs to the same patch set.  lets fix the current bug first.)
>
> > prepare_seq_file() has already acquired an extra reference of the currently
> > attached program, so it is OK to read the iterator after the close of iterator
> > link fd. So what do you think ?
>
> Right, it is my understanding also that the prog refcnt has been acquired during
> the iter creation.

Sounds good to me. The fact that the iter holds a reference to the
program is what I missed in my reply. Both solutions are correct.
Because of that, I don't have a strong opinion on either of them now.
:)

Re: [PATCH bpf v2 1/3] bpf: Pin iterator link when opening iterator

[Hou Tao]

Hi,

On 11/16/2022 3:16 AM, Martin KaFai Lau wrote:
> On 11/10/22 10:34 PM, Hou Tao wrote:
>> From: Hou Tao <houtao1@huawei.com>
>>
>> For many bpf iterator (e.g., cgroup iterator), iterator link acquires
>> the reference of iteration target in .attach_target(), but iterator link
>> may be closed before or in the middle of iteration, so iterator will
>> need to acquire the reference of iteration target as well to prevent
>> potential use-after-free. To avoid doing the acquisition in
>> .init_seq_private() for each iterator type, just pin iterator link in
>> iterator.
>
> iiuc, a link currently will go away when all its fds closed and pinned file
> removed.  After this change, the link will stay until the last iter is
> closed().  Before then, the user space can still "bpftool link show" and even
> get the link back by bpf_link_get_fd_by_id().  If this is the case, it would
> be useful to explain it in the commit message.
Yes, the iterator link is still reachable from "bpftool link show" and
bpf_link_get_fd_by_id().
>
> and does this new behavior make sense when comparing with other link types?
Have not considered about that. It seems that other link type will not be used
as an input for the creation of a bpf object (e.g., iterator). But if the fd of
other link type is closed, the link will be released. So after the fix in v2,
the behavior of iterator link will be different with other link types.
>
>>
>> Fixes: d4ccaf58a847 ("bpf: Introduce cgroup iter")
>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>> ---
>>   kernel/bpf/bpf_iter.c | 21 ++++++++++++++-------
>>   1 file changed, 14 insertions(+), 7 deletions(-)
>>
.

[PATCH bpf v2 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.

Acked-by: Yonghong Song <yhs@fb.com>
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 v2 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 from cgroup iterator.
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 from iterator fd will trigger use-after-free.

Acked-by: Yonghong Song <yhs@fb.com>
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 .../selftests/bpf/prog_tests/cgroup_iter.c    | 76 +++++++++++++++++++
 1 file changed, 76 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..e02feb5fae97 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c
@@ -189,6 +189,80 @@ 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 will be dead during read() 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_cgrp;
+
+	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);
+	close(cgrp_fd);
+
+	/* Remove cgroup to mark it as dead */
+	remove_cgroup(cgrp_name);
+
+	/* Two kern_sync_rcu() and usleep() pairs 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);
+	return;
+free_link:
+	bpf_link__destroy(link);
+close_cgrp:
+	close(cgrp_fd);
+}
+
 void test_cgroup_iter(void)
 {
 	struct cgroup_iter *skel = NULL;
@@ -217,6 +291,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 v2 3/3] selftests/bpf: Add test for cgroup iterator on a dead cgroup

[Hao Luo]

On Thu, Nov 10, 2022 at 10:08 PM Hou Tao <houtao@huaweicloud.com> 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 from cgroup iterator.
> 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 from iterator fd will trigger use-after-free.
>
> Acked-by: Yonghong Song <yhs@fb.com>
> Signed-off-by: Hou Tao <houtao1@huawei.com>

Acked-by: Hao Luo <haoluo@google.com>