| Message ID | 20221107074222.1323017-2-houtao@huaweicloud.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Pin the start cgroup for cgroup iterator | expand |
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>
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
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
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 >
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 >>
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 >>> >
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.
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 >>>> >>
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), };