From patchwork Mon Oct 23 12:29:10 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Rutland X-Patchwork-Id: 116736 Delivered-To: patch@linaro.org Received: by 10.140.22.164 with SMTP id 33csp4588812qgn; Mon, 23 Oct 2017 05:29:23 -0700 (PDT) X-Google-Smtp-Source: ABhQp+RqJNCDd+xx8CcQxBPqwFpxkxecPBWGFhlM2bh8NU0Clx2yJk2BtBD/GZHugCaZr2Z0Yv4S X-Received: by 10.99.124.27 with SMTP id x27mr11667114pgc.304.1508761763667; Mon, 23 Oct 2017 05:29:23 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1508761763; cv=none; d=google.com; s=arc-20160816; b=dYrlJs3fiulq5WF7rxCC2zFPg67f+04DIiAi6RE/dMOUm7IASQ2sOYp8S4Cp9LoXOH DKnC8jz2BJno1eq1gkKYzGd6ltxTMpmPiJJnbw+LFQpU7GQsItV5RlNT0K2B4nJ8g4vm E20PdOo4K9oFheeHAq0AV8U0WN3vQprDXTmeIflbnMOGYj4Du/o3fsiSJZBm3S3YTKWJ 3srAQEnpcxFEIp0Jwceiqx27hbiYJBi0b+NO9VtOgTWqQfwGSM8SnuYNE1iOn2hKYxl/ fcpZCbL9G8mzq9nfvvQVoqEsvbwdZh5Vo2Ch+dAOSnpzNEXn3Dq3lDSxrNWC8RbGx6gr zxrg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:message-id:date:subject:cc:to:from :arc-authentication-results; bh=JhdMEVTkYzIk3YpXeok31vNN8EJwX1puNy94tsPmFUA=; b=H7Yb/jATHWKtG+PA7QzDEy0mzGdHQy/bbK4iOYJjSSe1Lh8EYIioBCvL9jji2BvE5h YfufsVW6GITrYAnUmBqbEzzE1DTuQEXl4N4zKSjGwfL/C+IkkfUOCBHsPYDc/EVDm8lK 1GfrCD7Sn/mJv5Dbapub6VEO5MXvI3lBkV4Em9B/VR3MM9Qv8MdWbgT9tAkiX2BmsyhA 6xJ/D0W1+mPeRRWdIKwYC/IebaU6sDe484ElcfJHuf8TWklQoAj7X+ChxDeT6hRDpl8A 1vm2JMsNvIJyhXV364wSck1yZaJL5phhdE8Cddx9kif8KxsO+jFr1S14g7PvgXDSz2Yl Tw+A== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 205si5318577pfy.38.2017.10.23.05.29.23; Mon, 23 Oct 2017 05:29:23 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932205AbdJWM3V (ORCPT + 27 others); Mon, 23 Oct 2017 08:29:21 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:38574 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932093AbdJWM3T (ORCPT ); Mon, 23 Oct 2017 08:29:19 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6149B80D; Mon, 23 Oct 2017 05:29:19 -0700 (PDT) Received: from localhost.localdomain (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 48D8B3F58F; Mon, 23 Oct 2017 05:29:18 -0700 (PDT) From: Mark Rutland To: linux-kernel@vger.kernel.org Cc: Mark Rutland , Peter Zijlstra , Ingo Molnar Subject: [PATCH] locking/spinlock/debug: snapshot lock fields Date: Mon, 23 Oct 2017 13:29:10 +0100 Message-Id: <20171023122910.28376-1-mark.rutland@arm.com> X-Mailer: git-send-email 2.11.0 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Currently, the lock debug code doesn't use {READ,WRITE}_ONCE() to access lock fields, and thus may observe torn values given a suitably unhelpful compiler. These could result in false positives and/or false negatives for some sanity checks. Further, as we don't snapshot the values of various fields, these might change between the time they are sanity checked and the time they are logged, making debugging difficult. This patch ensures that lock fields are accessed with {READ,WRITE}_ONCE(), and uses a snapshot of the lock to ensure that logged values are the same as those used for the sanity checks. Signed-off-by: Mark Rutland Cc: Peter Zijlstra Cc: Ingo Molnar --- kernel/locking/spinlock_debug.c | 73 ++++++++++++++++++++++++++++------------- 1 file changed, 50 insertions(+), 23 deletions(-) Hi Peter, As mentioned at ELCE, I put this together while trying to track down an (as yet) inexplicable spinlock recursion bug. Luckily, it seems that my compiler wasn't actively out to get me, and this didn't help, but it might save someone a few hours of painful debugging in future. Mark. -- 2.11.0 diff --git a/kernel/locking/spinlock_debug.c b/kernel/locking/spinlock_debug.c index 9aa0fccd5d43..be0f0e5a7097 100644 --- a/kernel/locking/spinlock_debug.c +++ b/kernel/locking/spinlock_debug.c @@ -49,58 +49,85 @@ void __rwlock_init(rwlock_t *lock, const char *name, EXPORT_SYMBOL(__rwlock_init); -static void spin_dump(raw_spinlock_t *lock, const char *msg) +static void spin_dump(raw_spinlock_t *lockp, raw_spinlock_t lock, + const char *msg) { struct task_struct *owner = NULL; - if (lock->owner && lock->owner != SPINLOCK_OWNER_INIT) - owner = lock->owner; + if (lock.owner && lock.owner != SPINLOCK_OWNER_INIT) + owner = lock.owner; printk(KERN_EMERG "BUG: spinlock %s on CPU#%d, %s/%d\n", msg, raw_smp_processor_id(), current->comm, task_pid_nr(current)); printk(KERN_EMERG " lock: %pS, .magic: %08x, .owner: %s/%d, " ".owner_cpu: %d\n", - lock, lock->magic, + lockp, lock.magic, owner ? owner->comm : "", owner ? task_pid_nr(owner) : -1, - lock->owner_cpu); + lock.owner_cpu); dump_stack(); } -static void spin_bug(raw_spinlock_t *lock, const char *msg) +static void spin_bug(raw_spinlock_t *lockp, raw_spinlock_t lock, + const char *msg) { if (!debug_locks_off()) return; - spin_dump(lock, msg); + spin_dump(lockp, lock, msg); } -#define SPIN_BUG_ON(cond, lock, msg) if (unlikely(cond)) spin_bug(lock, msg) +#define SPIN_BUG_ON(cond, lockp, lock, msg) \ + if (unlikely(cond)) spin_bug(lockp, lock, msg) + +/* + * Copy fields from a lock, ensuring that each field is read without tearing. + * We cannot read the whole lock atomically, so this isn't a snapshot of the + * whole lock at an instant in time. However, it is a stable snapshot of each + * field that won't change under our feet. + */ +static inline raw_spinlock_t +debug_spin_lock_snapshot(raw_spinlock_t *lockp) +{ + raw_spinlock_t lock; + + lock.raw_lock = READ_ONCE(lockp->raw_lock); + lock.magic = READ_ONCE(lockp->magic); + lock.owner_cpu = READ_ONCE(lockp->owner_cpu); + lock.owner = READ_ONCE(lockp->owner); + + return lock; +} static inline void -debug_spin_lock_before(raw_spinlock_t *lock) +debug_spin_lock_before(raw_spinlock_t *lockp) { - SPIN_BUG_ON(lock->magic != SPINLOCK_MAGIC, lock, "bad magic"); - SPIN_BUG_ON(lock->owner == current, lock, "recursion"); - SPIN_BUG_ON(lock->owner_cpu == raw_smp_processor_id(), - lock, "cpu recursion"); + raw_spinlock_t lock = debug_spin_lock_snapshot(lockp); + + SPIN_BUG_ON(lock.magic != SPINLOCK_MAGIC, lockp, lock, "bad magic"); + SPIN_BUG_ON(lock.owner == current, lockp, lock, "recursion"); + SPIN_BUG_ON(lock.owner_cpu == raw_smp_processor_id(), lockp, lock, + "cpu recursion"); } static inline void debug_spin_lock_after(raw_spinlock_t *lock) { - lock->owner_cpu = raw_smp_processor_id(); - lock->owner = current; + WRITE_ONCE(lock->owner_cpu, raw_smp_processor_id()); + WRITE_ONCE(lock->owner, current); } -static inline void debug_spin_unlock(raw_spinlock_t *lock) +static inline void debug_spin_unlock(raw_spinlock_t *lockp) { - SPIN_BUG_ON(lock->magic != SPINLOCK_MAGIC, lock, "bad magic"); - SPIN_BUG_ON(!raw_spin_is_locked(lock), lock, "already unlocked"); - SPIN_BUG_ON(lock->owner != current, lock, "wrong owner"); - SPIN_BUG_ON(lock->owner_cpu != raw_smp_processor_id(), - lock, "wrong CPU"); - lock->owner = SPINLOCK_OWNER_INIT; - lock->owner_cpu = -1; + raw_spinlock_t lock = debug_spin_lock_snapshot(lockp); + + SPIN_BUG_ON(lock.magic != SPINLOCK_MAGIC, lockp, lock, "bad magic"); + SPIN_BUG_ON(arch_spin_value_unlocked(lock.raw_lock), lockp, lock, + "already unlocked"); + SPIN_BUG_ON(lock.owner != current, lockp, lock, "wrong owner"); + SPIN_BUG_ON(lock.owner_cpu != raw_smp_processor_id(), lockp, lock, + "wrong CPU"); + WRITE_ONCE(lockp->owner, SPINLOCK_OWNER_INIT); + WRITE_ONCE(lockp->owner_cpu, -1); } /*