| Message ID | 20220412192459.227740-1-tadeusz.struk@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | cgroup: don't queue css_release_work if one already pending | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
Hello Tadeusz. Thanks for analyzing this syzbot report. Let me provide my understanding of the test case and explanation why I think your patch fixes it but is not fully correct. On Tue, Apr 12, 2022 at 12:24:59PM -0700, Tadeusz Struk <tadeusz.struk@linaro.org> wrote: > Syzbot found a corrupted list bug scenario that can be triggered from > cgroup css_create(). The reproduces writes to cgroup.subtree_control > file, which invokes cgroup_apply_control_enable(), css_create(), and > css_populate_dir(), which then randomly fails with a fault injected -ENOMEM. The reproducer code makes it hard for me to understand which function fails with ENOMEM. But I can see your patch fixes the reproducer and your additional debug patch which proves that css->destroy_work is re-queued. > In such scenario the css_create() error path rcu enqueues css_free_rwork_fn > work for an css->refcnt initialized with css_release() destructor, Note that css_free_rwork_fn() utilizes css->destroy_*r*work. The error path in css_create() open codes relevant parts of css_release_work_fn() so that css_release() can be skipped and the refcnt is eventually just percpu_ref_exit()'d. > and there is a chance that the css_release() function will be invoked > for a cgroup_subsys_state, for which a destroy_work has already been > queued via css_create() error path. But I think the problem is css_populate_dir() failing in cgroup_apply_control_enable(). (Is this what you actually meant? css_create() error path is then irrelevant, no?) The already created csses should then be rolled back via cgroup_restore_control(cgrp); cgroup_apply_control_disable(cgrp); ... kill_css(css) I suspect the double-queuing is a result of the fact that there exists only the single reference to the css->refcnt. I.e. it's percpu_ref_kill_and_confirm()'d and released both at the same time. (Normally (when not killing the last reference), css->destroy_work reuse is not a problem because of the sequenced chain css_killed_work_fn()->css_put()->css_release().) > This can be avoided by adding a check to css_release() that checks > if it has already been enqueued. If that's what's happening, then your patch omits the final css_release_work_fn() in favor of css_killed_work_fn() but both should be run during the rollback upon css_populate_dir() failure. So an alternative approach to tackle this situation would be to split css->destroy_work into two work work_structs (one for killing, one for releasing) at the cost of inflating cgroup_subsys_state. Take my hypothesis with a grain of salt maybe the assumption (last reference == initial reference) is not different from normal operation. Regards, Michal
Hi Michal, Thanks for your analysis. On 4/14/22 09:44, Michal Koutný wrote: > Hello Tadeusz. > > Thanks for analyzing this syzbot report. Let me provide my understanding > of the test case and explanation why I think your patch fixes it but is > not fully correct. > > On Tue, Apr 12, 2022 at 12:24:59PM -0700, Tadeusz Struk <tadeusz.struk@linaro.org> wrote: >> Syzbot found a corrupted list bug scenario that can be triggered from >> cgroup css_create(). The reproduces writes to cgroup.subtree_control >> file, which invokes cgroup_apply_control_enable(), css_create(), and >> css_populate_dir(), which then randomly fails with a fault injected -ENOMEM. > > The reproducer code makes it hard for me to understand which function > fails with ENOMEM. > But I can see your patch fixes the reproducer and your additional debug > patch which proves that css->destroy_work is re-queued. Yes, it is hard to see the actual failing point because, I think it is randomly failing in different places. I think in the actual case that causes the list corruption is in fact in css_create(). It is the css_create() error path that does fist rcu enqueue in: https://elixir.bootlin.com/linux/v5.10.109/source/kernel/cgroup/cgroup.c#L5228 and the second is triggered by the css->refcnt calling css_release() The reason why we don't see it actually failing in css_create() in the trace dump is that the fail_dump() is rate-limited, see: https://elixir.bootlin.com/linux/v5.18-rc2/source/lib/fault-inject.c#L44 I was confused as well, so I put additional debug prints in every place where css_release() can fail, and it was actually in css_create()->cgroup_idr_alloc() that failed in my case. What happened was, the write triggered: cgroup_subtree_control_write()->cgroup_apply_control()->cgroup_apply_control_enable()->css_create() which, allocates and initializes the css, then fails in cgroup_idr_alloc(), bails out and calls queue_rcu_work(cgroup_destroy_wq, &css->destroy_rwork); then cgroup_subtree_control_write() bails out to out_unlock:, which then goes: cgroup_kn_unlock()->cgroup_put()->css_put()->percpu_ref_put(&css->refcnt)->percpu_ref_put_many(ref) which then calls ref->data->release(ref) and enqueues the same &css->destroy_rwork on cgroup_destroy_wq causing list corruption in insert_work. >> In such scenario the css_create() error path rcu enqueues css_free_rwork_fn >> work for an css->refcnt initialized with css_release() destructor, > > Note that css_free_rwork_fn() utilizes css->destroy_*r*work. > The error path in css_create() open codes relevant parts of > css_release_work_fn() so that css_release() can be skipped and the > refcnt is eventually just percpu_ref_exit()'d. > >> and there is a chance that the css_release() function will be invoked >> for a cgroup_subsys_state, for which a destroy_work has already been >> queued via css_create() error path. > > But I think the problem is css_populate_dir() failing in > cgroup_apply_control_enable(). (Is this what you actually meant? > css_create() error path is then irrelevant, no?) I thought so too at first as the the crushdump shows that this is failing in css_populate_dir(), but this is not the fail that causes the list corruption. The code can recover from the fail in css_populate_dir(). The fail that causes trouble is in css_create(), that makes it go to its error path. I can dig out the patch with my debug prints and request syzbot to run it if you want. > > The already created csses should then be rolled back via > cgroup_restore_control(cgrp); > cgroup_apply_control_disable(cgrp); > ... > kill_css(css) > > I suspect the double-queuing is a result of the fact that there exists > only the single reference to the css->refcnt. I.e. it's > percpu_ref_kill_and_confirm()'d and released both at the same time. > > (Normally (when not killing the last reference), css->destroy_work reuse > is not a problem because of the sequenced chain > css_killed_work_fn()->css_put()->css_release().) > >> This can be avoided by adding a check to css_release() that checks >> if it has already been enqueued. > > If that's what's happening, then your patch omits the final > css_release_work_fn() in favor of css_killed_work_fn() but both should > be run during the rollback upon css_populate_dir() failure. This change only prevents from double queue: queue_[rcu]_work(cgroup_destroy_wq, &css->destroy_rwork); I don't see how it affects the css_killed_work_fn() clean path. I didn't look at it, since I thought it is irrelevant in this case.
Hello, On Thu, Apr 14, 2022 at 10:51:18AM -0700, Tadeusz Struk wrote: > What happened was, the write triggered: > cgroup_subtree_control_write()->cgroup_apply_control()->cgroup_apply_control_enable()->css_create() > > which, allocates and initializes the css, then fails in cgroup_idr_alloc(), > bails out and calls queue_rcu_work(cgroup_destroy_wq, &css->destroy_rwork); Yes, but this css hasn't been installed yet. > then cgroup_subtree_control_write() bails out to out_unlock:, which then goes: > > cgroup_kn_unlock()->cgroup_put()->css_put()->percpu_ref_put(&css->refcnt)->percpu_ref_put_many(ref) And this is a different css. cgroup->self which isn't connected to the half built css which got destroyed in css_create(). So, I have a bit of difficulty following this scenario. The way that the current code uses destroy_work is definitely nasty and it'd probably be a good idea to separate out the different use cases, but let's first understand what's failing. Thanks.
On Thu, Apr 14, 2022 at 06:44:09PM +0200, Michal Koutný wrote: > I suspect the double-queuing is a result of the fact that there exists > only the single reference to the css->refcnt. I.e. it's > percpu_ref_kill_and_confirm()'d and released both at the same time. > > (Normally (when not killing the last reference), css->destroy_work reuse > is not a problem because of the sequenced chain > css_killed_work_fn()->css_put()->css_release().) If this is the case, we need to hold an extra reference to be put by the css_killed_work_fn(), right? Thanks.
On Thu, Apr 21, 2022 at 02:00:56PM -1000, Tejun Heo <tj@kernel.org> wrote: > If this is the case, we need to hold an extra reference to be put by the > css_killed_work_fn(), right? I looked into it a bit more lately and found that there already is such a fuse in kill_css() [1]. At the same type syzbots stack trace demonstrates the fuse is ineffective > css_release+0xae/0xc0 kernel/cgroup/cgroup.c:5146 (**) > percpu_ref_put_many include/linux/percpu-refcount.h:322 [inline] > percpu_ref_put include/linux/percpu-refcount.h:338 [inline] > percpu_ref_call_confirm_rcu lib/percpu-refcount.c:162 [inline] (*) > percpu_ref_switch_to_atomic_rcu+0x5a2/0x5b0 lib/percpu-refcount.c:199 > rcu_do_batch+0x4f8/0xbc0 kernel/rcu/tree.c:2485 > rcu_core+0x59b/0xe30 kernel/rcu/tree.c:2722 > rcu_core_si+0x9/0x10 kernel/rcu/tree.c:2735 > __do_softirq+0x27e/0x596 kernel/softirq.c:305 (*) this calls css_killed_ref_fn confirm_switch (**) zero references after confirmed kill? So, I was also looking at the possible race with css_free_rwork_fn() (from failed css_create()) but that would likely emit a warning from __percpu_ref_exit(). So, I still think there's something fishy (so far possible only via artificial ENOMEM injection) that needs an explanation... Michal [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/cgroup/cgroup.c#n5608
On 4/22/22 04:05, Michal Koutný wrote: > On Thu, Apr 21, 2022 at 02:00:56PM -1000, Tejun Heo <tj@kernel.org> wrote: >> If this is the case, we need to hold an extra reference to be put by the >> css_killed_work_fn(), right? > > I looked into it a bit more lately and found that there already is such > a fuse in kill_css() [1]. > > At the same type syzbots stack trace demonstrates the fuse is > ineffective > >> css_release+0xae/0xc0 kernel/cgroup/cgroup.c:5146 (**) >> percpu_ref_put_many include/linux/percpu-refcount.h:322 [inline] >> percpu_ref_put include/linux/percpu-refcount.h:338 [inline] >> percpu_ref_call_confirm_rcu lib/percpu-refcount.c:162 [inline] (*) >> percpu_ref_switch_to_atomic_rcu+0x5a2/0x5b0 lib/percpu-refcount.c:199 >> rcu_do_batch+0x4f8/0xbc0 kernel/rcu/tree.c:2485 >> rcu_core+0x59b/0xe30 kernel/rcu/tree.c:2722 >> rcu_core_si+0x9/0x10 kernel/rcu/tree.c:2735 >> __do_softirq+0x27e/0x596 kernel/softirq.c:305 > > (*) this calls css_killed_ref_fn confirm_switch > (**) zero references after confirmed kill? > > So, I was also looking at the possible race with css_free_rwork_fn() > (from failed css_create()) but that would likely emit a warning from > __percpu_ref_exit(). > > So, I still think there's something fishy (so far possible only via > artificial ENOMEM injection) that needs an explanation... I can't reliably reproduce this issue on neither mainline nor v5.10, where syzbot originally found it. It still triggers for syzbot though.
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c index adb820e98f24..9ae2de29f8c9 100644 --- a/kernel/cgroup/cgroup.c +++ b/kernel/cgroup/cgroup.c @@ -5210,8 +5210,11 @@ static void css_release(struct percpu_ref *ref) struct cgroup_subsys_state *css = container_of(ref, struct cgroup_subsys_state, refcnt); - INIT_WORK(&css->destroy_work, css_release_work_fn); - queue_work(cgroup_destroy_wq, &css->destroy_work); + if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, + work_data_bits(&css->destroy_work))) { + INIT_WORK(&css->destroy_work, css_release_work_fn); + queue_work(cgroup_destroy_wq, &css->destroy_work); + } } static void init_and_link_css(struct cgroup_subsys_state *css,
Syzbot found a corrupted list bug scenario that can be triggered from cgroup css_create(). The reproduces writes to cgroup.subtree_control file, which invokes cgroup_apply_control_enable(), css_create(), and css_populate_dir(), which then randomly fails with a fault injected -ENOMEM. In such scenario the css_create() error path rcu enqueues css_free_rwork_fn work for an css->refcnt initialized with css_release() destructor, and there is a chance that the css_release() function will be invoked for a cgroup_subsys_state, for which a destroy_work has already been queued via css_create() error path. This causes a list_add corruption as can be seen in the syzkaller report [1]. This can be avoided by adding a check to css_release() that checks if it has already been enqueued. [1] https://syzkaller.appspot.com/bug?id=e26e54d6eac9d9fb50b221ec3e4627b327465dbd Cc: Tejun Heo <tj@kernel.org> Cc: Zefan Li <lizefan.x@bytedance.com> Cc: Johannes Weiner <hannes@cmpxchg.org> Cc: Christian Brauner <brauner@kernel.org> Cc: Alexei Starovoitov <ast@kernel.org> Cc: Daniel Borkmann <daniel@iogearbox.net> Cc: Andrii Nakryiko <andrii@kernel.org> Cc: Martin KaFai Lau <kafai@fb.com> Cc: Song Liu <songliubraving@fb.com> Cc: Yonghong Song <yhs@fb.com> Cc: John Fastabend <john.fastabend@gmail.com> Cc: KP Singh <kpsingh@kernel.org> Cc: <cgroups@vger.kernel.org> Cc: <netdev@vger.kernel.org> Cc: <bpf@vger.kernel.org> Cc: <stable@vger.kernel.org> Cc: <linux-kernel@vger.kernel.org> Reported-by: syzbot+e42ae441c3b10acf9e9d@syzkaller.appspotmail.com Fixes: 8f36aaec9c92 ("cgroup: Use rcu_work instead of explicit rcu and work item") Signed-off-by: Tadeusz Struk <tadeusz.struk@linaro.org> --- kernel/cgroup/cgroup.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)