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@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.
OK. I will keep it as is.
>
>>>
>>>>>
>>>>> Hao
>>>>
>>
