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

[Yonghong Song <yhs@meta.com>]

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
>>>
>