| Message ID | 20241016093633.670555-1-chenridong@huaweicloud.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | cgroup/bpf: fix NULL pointer dereference at cgroup_bpf_offline | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
Hello. On Wed, Oct 16, 2024 at 09:36:33AM GMT, Chen Ridong <chenridong@huaweicloud.com> wrote: > As mentioned above, when cgroup_bpf_inherit returns an error in > cgroup_setup_root, cgrp->bpf.refcnt has been exited. If cgrp->bpf.refcnt is > killed again in the cgroup_kill_sb function, the data of cgrp->bpf.refcnt > may have become NULL, leading to NULL pointer dereference. > > To fix this issue, goto err when cgroup_bpf_inherit returns an error. > Additionally, if cgroup_bpf_inherit returns an error after rebinding > subsystems, the root_cgrp->self.refcnt is exited, which leads to > cgroup1_root_to_use return 1 (restart) when subsystems is mounted next. > This is due to a failure trying to get the refcnt(the root is root_cgrp, > without rebinding back to cgrp_dfl_root). So move the call to > cgroup_bpf_inherit above rebind_subsystems in the cgroup_setup_root. > > Fixes: 04f8ef5643bc ("cgroup: Fix memory leak caused by missing cgroup_bpf_offline") > Signed-off-by: Chen Ridong <chenridong@huawei.com> Hm, I always thought that BPF progs can only be attached to the default hierarchy (cgroup_bpf_prog_attach/cgroup_get_from_fd should prevent that). Thus I wonder whether cgroup_bpf_inherit (which is more like cgroup_bpf_init in this case) needs to be called no v1 roots at all (and with such a change, 04f8ef5643bc could be effectively reverted too). Or can bpf data be used on v1 hierarchies somehow? Thanks, Michal
On Wed, Oct 16, 2024 at 03:13:52PM +0200, Michal Koutný wrote: > Hello. > > On Wed, Oct 16, 2024 at 09:36:33AM GMT, Chen Ridong <chenridong@huaweicloud.com> wrote: > > As mentioned above, when cgroup_bpf_inherit returns an error in > > cgroup_setup_root, cgrp->bpf.refcnt has been exited. If cgrp->bpf.refcnt is > > killed again in the cgroup_kill_sb function, the data of cgrp->bpf.refcnt > > may have become NULL, leading to NULL pointer dereference. > > > > To fix this issue, goto err when cgroup_bpf_inherit returns an error. > > Additionally, if cgroup_bpf_inherit returns an error after rebinding > > subsystems, the root_cgrp->self.refcnt is exited, which leads to > > cgroup1_root_to_use return 1 (restart) when subsystems is mounted next. > > This is due to a failure trying to get the refcnt(the root is root_cgrp, > > without rebinding back to cgrp_dfl_root). So move the call to > > cgroup_bpf_inherit above rebind_subsystems in the cgroup_setup_root. > > > > Fixes: 04f8ef5643bc ("cgroup: Fix memory leak caused by missing cgroup_bpf_offline") > > Signed-off-by: Chen Ridong <chenridong@huawei.com> > > Hm, I always thought that BPF progs can only be attached to the default > hierarchy (cgroup_bpf_prog_attach/cgroup_get_from_fd should prevent > that). > > Thus I wonder whether cgroup_bpf_inherit (which is more like > cgroup_bpf_init in this case) needs to be called no v1 roots at all (and > with such a change, 04f8ef5643bc could be effectively reverted too). > > Or can bpf data be used on v1 hierarchies somehow? We relaxed some of the usages (see cgroup_v1v2_get_from_fd()) but cgroup BPF progs can only be attached to v2. Thanks.
On 2024/10/17 1:04, Tejun Heo wrote: > On Wed, Oct 16, 2024 at 03:13:52PM +0200, Michal Koutný wrote: >> Hello. >> >> On Wed, Oct 16, 2024 at 09:36:33AM GMT, Chen Ridong <chenridong@huaweicloud.com> wrote: >>> As mentioned above, when cgroup_bpf_inherit returns an error in >>> cgroup_setup_root, cgrp->bpf.refcnt has been exited. If cgrp->bpf.refcnt is >>> killed again in the cgroup_kill_sb function, the data of cgrp->bpf.refcnt >>> may have become NULL, leading to NULL pointer dereference. >>> >>> To fix this issue, goto err when cgroup_bpf_inherit returns an error. >>> Additionally, if cgroup_bpf_inherit returns an error after rebinding >>> subsystems, the root_cgrp->self.refcnt is exited, which leads to >>> cgroup1_root_to_use return 1 (restart) when subsystems is mounted next. >>> This is due to a failure trying to get the refcnt(the root is root_cgrp, >>> without rebinding back to cgrp_dfl_root). So move the call to >>> cgroup_bpf_inherit above rebind_subsystems in the cgroup_setup_root. >>> >>> Fixes: 04f8ef5643bc ("cgroup: Fix memory leak caused by missing cgroup_bpf_offline") >>> Signed-off-by: Chen Ridong <chenridong@huawei.com> >> >> Hm, I always thought that BPF progs can only be attached to the default >> hierarchy (cgroup_bpf_prog_attach/cgroup_get_from_fd should prevent >> that). >> >> Thus I wonder whether cgroup_bpf_inherit (which is more like >> cgroup_bpf_init in this case) needs to be called no v1 roots at all (and >> with such a change, 04f8ef5643bc could be effectively reverted too). >> >> Or can bpf data be used on v1 hierarchies somehow? > > We relaxed some of the usages (see cgroup_v1v2_get_from_fd()) but cgroup BPF > progs can only be attached to v2. > > Thanks. > So, should commit 04f8ef5643bc ("cgroup: Fix memory leak caused by missing cgroup_bpf_offline") be reverted, and should cgroup_bpf_inherit be only called in v2? Have I understood this correctly? Best regards, Ridong
On Thu, Oct 17, 2024 at 08:17:58PM GMT, Chen Ridong <chenridong@huaweicloud.com> wrote: > So, should commit 04f8ef5643bc ("cgroup: Fix memory leak caused by missing > cgroup_bpf_offline") be reverted, and should cgroup_bpf_inherit be only > called in v2? Yes, that should resolve both the original leak and the current NULL ptr dereference without extra rebinding complications. I'd like to keep only cgroup v2 be attachable by BPF programs. cgroup_v1v2_get_from_fd serves the needs of traversing v1 hierarchies, not program attchments. I hope this proposal is workable also from the CC'ed BPF people perspective. Michal
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c index 5886b95c6eae..8a0cbf95cc57 100644 --- a/kernel/cgroup/cgroup.c +++ b/kernel/cgroup/cgroup.c @@ -2136,12 +2136,13 @@ int cgroup_setup_root(struct cgroup_root *root, u16 ss_mask) if (ret) goto destroy_root; - ret = rebind_subsystems(root, ss_mask); + ret = cgroup_bpf_inherit(root_cgrp); if (ret) goto exit_stats; - ret = cgroup_bpf_inherit(root_cgrp); - WARN_ON_ONCE(ret); + ret = rebind_subsystems(root, ss_mask); + if (ret) + goto exit_stats; trace_cgroup_setup_root(root);