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@xxxxxxxxx> wrote: >> On 11/10/22 10:34 PM, Hou Tao wrote: >>> From: Hou Tao <houtao1@xxxxxxxxxx> >>> >>> 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.
