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@xxxxxxxx> wrote:
On 11/6/22 11:42 PM, Hou Tao wrote:
From: Hou Tao <houtao1@xxxxxxxxxx>
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@xxxxxxxxxx>
Acked-by: Yonghong Song <yhs@xxxxxx>
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@xxxxxxxxxxxxxxx/
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
