From patchwork Wed Apr 12 07:54:39 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Thomas Gleixner X-Patchwork-Id: 82296 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp155906vqo; Wed, 12 Apr 2023 01:20:54 -0700 (PDT) X-Google-Smtp-Source: AKy350aDjJ8rq14l+ViC+nlmFNREIHr6CR4KXBaYuPqESDb+aSnx4HKXXpTVACfBY+mtV8VZM9OI X-Received: by 2002:a17:906:a44f:b0:947:ebd5:c798 with SMTP id cb15-20020a170906a44f00b00947ebd5c798mr5337411ejb.54.1681287654113; Wed, 12 Apr 2023 01:20:54 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1681287654; cv=none; d=google.com; s=arc-20160816; b=GQ0Qtug/0I8hHy5qlwyWtGuYBQ9v2fc288XQaxwSosOVCfQKsdO0ghtV6X1x2mLvgI ON3cd68+4EviytPxuv/0c6C/6kbvDPGhFSFsYaGsZ6KA5GiHEaI+pCc67VeozJu/45v+ KX2kT3zSJJNUSuXf2IbAk8zjee4y848YcrcJUJ4OGXY2fkDEYlObRx6bbv7GnztNnumX //hl8KPo9Zx1uFtLeHzabQj0XvCA/2YTFZcN6Wu1oQ1uSQh28GwxFI4YG3KXulSw0GFw FbUCz7flQIU7ptoGJO2SFb8hJGSqVdnWk7c8Lb41dqHBrNZsXxGVQv+lgweWoFSj3s8D qhKw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:message-id:date:references :in-reply-to:subject:cc:to:dkim-signature:dkim-signature:from; bh=FOD+CmMFtFJl4+nHjymUPVJdIuhf/gLdHdu5oOnu/+M=; b=ZB+t8AWw4BEGaSdO3hg1Ek1RxpUbnIiorravWAsGAYdGLwD9S30UUQE6RCiG3A3unw YeSUBYyhkreOd0dFiNuioY0p4ObmrftPBi34WCHtKfTQjj6jb9+O2tKG+S0XfP2q8iZu /7rJ4h0wZuPRoEMY8jeJ7my0ZryRmNlFrX3YeRj2qL8P3gaiH44trXxgGado9ZX4Ja16 uc1jR6gwsl/WAZLj5GZ9QdQiXEL5t7guCH2c2x0eilWlMJ/AjswSK+rJ+6S1QvcPOBj4 LYnpxkNMojS8g3SiUgzixtRRLvTp5rvTbRwkG37OEJFKdzUaDt9oeBDNNGF1dKkjuWFz 5R5w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linutronix.de header.s=2020 header.b=Oat9kcFx; dkim=neutral (no key) header.i=@linutronix.de header.s=2020e header.b=mf1xcf3f; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=linutronix.de Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id w10-20020a50fa8a000000b00504994eb1bcsi6759029edr.626.2023.04.12.01.20.17; Wed, 12 Apr 2023 01:20:54 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@linutronix.de header.s=2020 header.b=Oat9kcFx; dkim=neutral (no key) header.i=@linutronix.de header.s=2020e header.b=mf1xcf3f; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=linutronix.de Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229541AbjDLHyp (ORCPT + 99 others); Wed, 12 Apr 2023 03:54:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42590 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229481AbjDLHyn (ORCPT ); Wed, 12 Apr 2023 03:54:43 -0400 Received: from galois.linutronix.de (Galois.linutronix.de [IPv6:2a0a:51c0:0:12e:550::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1A091C3 for ; Wed, 12 Apr 2023 00:54:42 -0700 (PDT) From: Thomas Gleixner DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1681286080; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=FOD+CmMFtFJl4+nHjymUPVJdIuhf/gLdHdu5oOnu/+M=; b=Oat9kcFx4KqMqC9bLqOAunYtzkDHn/6pI9D7pGv1TqXgiO6pQrdVdseY7yBuPdO9uGBf6V M0vlvaDvziQiLgl30KLAqopLGAeCspUmV8w4f9zxHcK9FtkzCpmSzljNM5Q0B5vDG6inxj mHTihfdMTvGvEO3RzciytIv4WRlUUCBKpRvHpREZAm6iTZw3KBwuheaH1gsizblL+/JUVv b+CZApBXxG8L5ilAVO4Og72SggBk+mmHs36Grt+9gejBt0hhiZb41SmcPSmK1MfZWfuqCZ UDy9cmexZbz3WaEbNrWzgr+0LfZynFl/owqJxdBmERDKi9ocXGK980B//u67fg== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1681286080; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=FOD+CmMFtFJl4+nHjymUPVJdIuhf/gLdHdu5oOnu/+M=; b=mf1xcf3fnKdbeMLa5iKp5Ow0A++KMhT4i3kyN/gLwewp+2p627lZN/Wf8FVFgg613fkkH8 kRhKzbFHdzI0jiBw== To: Schspa Shi Cc: longman@redhat.com, swboyd@chromium.org, linux@roeck-us.net, wuchi.zero@gmail.com, linux-kernel@vger.kernel.org, syzbot+5093ba19745994288b53@syzkaller.appspotmail.com Subject: [PATCH] debugobject: Prevent init race with static objects In-Reply-To: <87pm908xvu.ffs@tglx> References: <20230303161906.831686-1-schspa@gmail.com> <87bkl9jt3a.ffs@tglx> <87sfdw8yru.ffs@tglx> <87pm908xvu.ffs@tglx> Date: Wed, 12 Apr 2023 09:54:39 +0200 Message-ID: <87zg7dzgao.ffs@tglx> MIME-Version: 1.0 X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1762957883122428943?= X-GMAIL-MSGID: =?utf-8?q?1762957883122428943?= Statically initialized objects are usually not initialized via the init() function of the subsystem. They are special cased and the subsystem provides a function to validate whether an object which is not yet tracked by debugobjects is statically initialized. This means the object is started to be tracked on first use, e.g. activation. This works perfectly fine, unless there are two concurrent operations on that object. Schspa decoded the problem: T0 T1 debug_object_assert_init(addr) lock_hash_bucket() obj = lookup_object(addr); if (!obj) { unlock_hash_bucket(); - > preemption lock_subsytem_object(addr); activate_object(addr) lock_hash_bucket(); obj = lookup_object(addr); if (!obj) { unlock_hash_bucket(); if (is_static_object(addr)) init_and_track(addr); lock_hash_bucket(); obj = lookup_object(addr); obj->state = ACTIVATED; unlock_hash_bucket(); subsys function modifies content of addr, so static object detection does not longer work. unlock_subsytem_object(addr); if (is_static_object(addr)) <- Fails debugobject emits a warning and invokes the fixup function which reinitializes the already active object in the worst case. This race exists forever, but was never observed until mod_timer() got a debug_object_assert_init() added which is outside of the timer base lock held section right at the beginning of the function to cover the lockless early exit points too. Rework the code so that the lookup, the static object check and the tracking object association happens atomically under the hash bucket lock. This prevents the issue completely as all callers are serialized on the hash bucket lock and therefore cannot observe inconsistent state. Fixes: 3ac7fe5a4aab ("infrastructure to debug (dynamic) objects") Reported-by: syzbot+5093ba19745994288b53@syzkaller.appspotmail.com Debugged-by: Schspa Shi Signed-off-by: Thomas Gleixner Link: https://syzkaller.appspot.com/bug?id=22c8a5938eab640d1c6bcc0e3dc7be519d878462 Link: https://lore.kernel.org/lkml/20230303161906.831686-1-schspa@gmail.com Reviewed-by: Stephen Boyd Tested-by: Ido Schimmel --- lib/debugobjects.c | 125 +++++++++++++++++++++++++++-------------------------- 1 file changed, 66 insertions(+), 59 deletions(-) --- a/lib/debugobjects.c +++ b/lib/debugobjects.c @@ -216,10 +216,6 @@ static struct debug_obj *__alloc_object( return obj; } -/* - * Allocate a new object. If the pool is empty, switch off the debugger. - * Must be called with interrupts disabled. - */ static struct debug_obj * alloc_object(void *addr, struct debug_bucket *b, const struct debug_obj_descr *descr) { @@ -552,11 +548,49 @@ static void debug_object_is_on_stack(voi WARN_ON(1); } +static struct debug_obj *lookup_object_or_alloc(void *addr, struct debug_bucket *b, + const struct debug_obj_descr *descr, + bool onstack, bool alloc_ifstatic) +{ + struct debug_obj *obj = lookup_object(addr, b); + enum debug_obj_state state = ODEBUG_STATE_NONE; + + if (likely(obj)) + return obj; + + /* + * debug_object_init() unconditionally allocates untracked + * objects. It does not matter whether it is a static object or + * not. + * + * debug_object_assert_init() and debug_object_activate() allow + * allocation only if the descriptor callback confirms that the + * object is static and considered initialized. For non-static + * objects the allocation needs to be done from the fixup callback. + */ + if (unlikely(alloc_ifstatic)) { + if (!descr->is_static_object || !descr->is_static_object(addr)) + return ERR_PTR(-ENOENT); + /* Statically allocated objects are considered initialized */ + state = ODEBUG_STATE_INIT; + } + + obj = alloc_object(addr, b, descr); + if (likely(obj)) { + obj->state = state; + debug_object_is_on_stack(addr, onstack); + return obj; + } + + /* Out of memory. Do the cleanup outside of the locked region */ + debug_objects_enabled = 0; + return NULL; +} + static void __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack) { enum debug_obj_state state; - bool check_stack = false; struct debug_bucket *db; struct debug_obj *obj; unsigned long flags; @@ -572,16 +606,11 @@ static void raw_spin_lock_irqsave(&db->lock, flags); - obj = lookup_object(addr, db); - if (!obj) { - obj = alloc_object(addr, db, descr); - if (!obj) { - debug_objects_enabled = 0; - raw_spin_unlock_irqrestore(&db->lock, flags); - debug_objects_oom(); - return; - } - check_stack = true; + obj = lookup_object_or_alloc(addr, db, descr, onstack, false); + if (unlikely(!obj)) { + raw_spin_unlock_irqrestore(&db->lock, flags); + debug_objects_oom(); + return; } switch (obj->state) { @@ -607,8 +636,6 @@ static void } raw_spin_unlock_irqrestore(&db->lock, flags); - if (check_stack) - debug_object_is_on_stack(addr, onstack); } /** @@ -648,14 +675,12 @@ EXPORT_SYMBOL_GPL(debug_object_init_on_s */ int debug_object_activate(void *addr, const struct debug_obj_descr *descr) { + struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr }; enum debug_obj_state state; struct debug_bucket *db; struct debug_obj *obj; unsigned long flags; int ret; - struct debug_obj o = { .object = addr, - .state = ODEBUG_STATE_NOTAVAILABLE, - .descr = descr }; if (!debug_objects_enabled) return 0; @@ -664,8 +689,8 @@ int debug_object_activate(void *addr, co raw_spin_lock_irqsave(&db->lock, flags); - obj = lookup_object(addr, db); - if (obj) { + obj = lookup_object_or_alloc(addr, db, descr, false, true); + if (likely(!IS_ERR_OR_NULL(obj))) { bool print_object = false; switch (obj->state) { @@ -698,24 +723,16 @@ int debug_object_activate(void *addr, co raw_spin_unlock_irqrestore(&db->lock, flags); - /* - * We are here when a static object is activated. We - * let the type specific code confirm whether this is - * true or not. if true, we just make sure that the - * static object is tracked in the object tracker. If - * not, this must be a bug, so we try to fix it up. - */ - if (descr->is_static_object && descr->is_static_object(addr)) { - /* track this static object */ - debug_object_init(addr, descr); - debug_object_activate(addr, descr); - } else { - debug_print_object(&o, "activate"); - ret = debug_object_fixup(descr->fixup_activate, addr, - ODEBUG_STATE_NOTAVAILABLE); - return ret ? 0 : -EINVAL; + /* If NULL the allocaction has hit OOM */ + if (!obj) { + debug_objects_oom(); + return 0; } - return 0; + + /* Object is neither static nor tracked. It's not initialized */ + debug_print_object(&o, "activate"); + ret = debug_object_fixup(descr->fixup_activate, addr, ODEBUG_STATE_NOTAVAILABLE); + return ret ? 0 : -EINVAL; } EXPORT_SYMBOL_GPL(debug_object_activate); @@ -869,6 +886,7 @@ EXPORT_SYMBOL_GPL(debug_object_free); */ void debug_object_assert_init(void *addr, const struct debug_obj_descr *descr) { + struct debug_obj o = { .object = addr, .state = ODEBUG_STATE_NOTAVAILABLE, .descr = descr }; struct debug_bucket *db; struct debug_obj *obj; unsigned long flags; @@ -879,31 +897,20 @@ void debug_object_assert_init(void *addr db = get_bucket((unsigned long) addr); raw_spin_lock_irqsave(&db->lock, flags); + obj = lookup_object_or_alloc(addr, db, descr, false, true); + raw_spin_unlock_irqrestore(&db->lock, flags); + if (likely(!IS_ERR_OR_NULL(obj))) + return; - obj = lookup_object(addr, db); + /* If NULL the allocaction has hit OOM */ if (!obj) { - struct debug_obj o = { .object = addr, - .state = ODEBUG_STATE_NOTAVAILABLE, - .descr = descr }; - - raw_spin_unlock_irqrestore(&db->lock, flags); - /* - * Maybe the object is static, and we let the type specific - * code confirm. Track this static object if true, else invoke - * fixup. - */ - if (descr->is_static_object && descr->is_static_object(addr)) { - /* Track this static object */ - debug_object_init(addr, descr); - } else { - debug_print_object(&o, "assert_init"); - debug_object_fixup(descr->fixup_assert_init, addr, - ODEBUG_STATE_NOTAVAILABLE); - } + debug_objects_oom(); return; } - raw_spin_unlock_irqrestore(&db->lock, flags); + /* Object is neither tracked nor static. It's not initialized. */ + debug_print_object(&o, "assert_init"); + debug_object_fixup(descr->fixup_assert_init, addr, ODEBUG_STATE_NOTAVAILABLE); } EXPORT_SYMBOL_GPL(debug_object_assert_init);