| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20111221220239.GJ23916@ZenIV.linux.org.uk>
Date: Wed, 21 Dec 2011 22:02:39 +0000
From: Al Viro <viro@...IV.linux.org.uk>
To: "Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
Cc: mc@...ux.vnet.ibm.com, Stephen Boyd <sboyd@...eaurora.org>,
linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
Nick Piggin <npiggin@...nel.dk>, david@...morbit.com,
"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
Maciej Rutecki <maciej.rutecki@...il.com>
Subject: Re: [PATCH] VFS: br_write_lock locks on possible CPUs other than
online CPUs
On Thu, Dec 22, 2011 at 02:45:29AM +0530, Srivatsa S. Bhat wrote:
> The following patch is the same as the patch you posted, but with this small
> change (aimed merely at making the code a bit easier to understand) and a
> commit message added. Please point out if this change seems bad for
> any reason. And if it is fine, Viro, can you please sign-off on this patch?
> (since this patch has code contributions from both you and me)
I can live with that. I still think it's not particulary useful, but...
Anyway,
ACKed-by: Al Viro <viro@...iv.linux.org.uk>
Signed-off-by: Al Viro <viro@...iv.linux.org.uk>
I couldn't care less who ends up in the Author: fields. I can feed it to
Linus tonight, if you prefer it to go through the VFS tree. But IMO that's
properly a CPU-hotplug issue and VFS is only involved as it's the only
current user of br-locks. Should go into -stable as well, all way back to
2.6.36...
Sad that Nick is still MIA, BTW - it's his code and he'd been Cc'ed on the
thread all along... ;-/
> I tested this patch locally - the originally reported issue did not crop up
> (soft-lockups in VFS callpaths).
>
> ---
> From: Srivatsa S. Bhat <srivatsa.bhat@...ux.vnet.ibm.com>
> Subject: [PATCH] VFS: Fix race between CPU hotplug and lglocks
>
> Currently, the *_global_[un]lock_online() routines are not at all synchronized
> with CPU hotplug. Soft-lockups detected as a consequence of this race was
> reported earlier at https://lkml.org/lkml/2011/8/24/185. (Thanks to Cong Meng
> for finding out that the root-cause of this issue is the race condition
> between br_write_[un]lock() and CPU hotplug, which results in the lock states
> getting messed up).
>
> Fixing this race by just adding {get,put}_online_cpus() at appropriate places
> in *_global_[un]lock_online() is not a good option, because, then suddenly
> br_write_[un]lock() would become blocking, whereas they have been kept as
> non-blocking all this time, and we would want to keep them that way.
>
> So, overall, we want to ensure 3 things:
> 1. br_write_lock() and br_write_unlock() must remain as non-blocking.
> 2. The corresponding lock and unlock of the per-cpu spinlocks must not happen
> for different sets of CPUs.
> 3. Either prevent any new CPU online operation in between this lock-unlock, or
> ensure that the newly onlined CPU does not proceed with its corresponding
> per-cpu spinlock unlocked.
>
> To achieve all this:
> (a) We introduce a new spinlock that is taken by the *_global_lock_online()
> routine and released by the *_global_unlock_online() routine.
> (b) We register a callback for CPU hotplug notifications, and this callback
> takes the same spinlock as above.
> (c) We maintain a bitmap which is close to the cpu_online_mask, and once it is
> initialized in the lock_init() code, all future updates to it are done in
> the callback, under the above spinlock.
> (d) The above bitmap is used (instead of cpu_online_mask) while locking and
> unlocking the per-cpu locks.
>
> The callback takes the spinlock upon the CPU_UP_PREPARE event. So, if the
> br_write_lock-unlock sequence is in progress, the callback keeps spinning,
> thus preventing the CPU online operation till the lock-unlock sequence is
> complete. This takes care of requirement (3).
>
> The bitmap that we maintain remains unmodified throughout the lock-unlock
> sequence, since all updates to it are managed by the callback, which takes
> the same spinlock as the one taken by the lock code and released only by the
> unlock routine. Combining this with (d) above, satisfies requirement (2).
>
> Overall, since we use a spinlock (mentioned in (a)) to prevent CPU hotplug
> operations from racing with br_write_lock-unlock, requirement (1) is also
> taken care of.
>
> By the way, it is to be noted that a CPU offline operation can actually run
> in parallel with our lock-unlock sequence, because our callback doesn't react
> to notifications earlier than CPU_DEAD (in order to maintain our bitmap
> properly). And this means, since we use our own bitmap (which is stale, on
> purpose) during the lock-unlock sequence, we could end up unlocking the
> per-cpu lock of an offline CPU (because we had locked it earlier, when the
> CPU was online), in order to satisfy requirement (2). But this is harmless,
> though it looks a bit awkward.
>
> Debugged-by: Cong Meng <mc@...ux.vnet.ibm.com>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@...ux.vnet.ibm.com>
> ---
>
> include/linux/lglock.h | 36 ++++++++++++++++++++++++++++++++----
> 1 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/lglock.h b/include/linux/lglock.h
> index f549056..87f402c 100644
> --- a/include/linux/lglock.h
> +++ b/include/linux/lglock.h
> @@ -22,6 +22,7 @@
> #include <linux/spinlock.h>
> #include <linux/lockdep.h>
> #include <linux/percpu.h>
> +#include <linux/cpu.h>
>
> /* can make br locks by using local lock for read side, global lock for write */
> #define br_lock_init(name) name##_lock_init()
> @@ -72,9 +73,31 @@
>
> #define DEFINE_LGLOCK(name) \
> \
> + DEFINE_SPINLOCK(name##_cpu_lock); \
> + cpumask_t name##_cpus __read_mostly; \
> DEFINE_PER_CPU(arch_spinlock_t, name##_lock); \
> DEFINE_LGLOCK_LOCKDEP(name); \
> \
> + static int \
> + name##_lg_cpu_callback(struct notifier_block *nb, \
> + unsigned long action, void *hcpu) \
> + { \
> + switch (action & ~CPU_TASKS_FROZEN) { \
> + case CPU_UP_PREPARE: \
> + spin_lock(&name##_cpu_lock); \
> + cpu_set((unsigned long)hcpu, name##_cpus); \
> + spin_unlock(&name##_cpu_lock); \
> + break; \
> + case CPU_UP_CANCELED: case CPU_DEAD: \
> + spin_lock(&name##_cpu_lock); \
> + cpu_clear((unsigned long)hcpu, name##_cpus); \
> + spin_unlock(&name##_cpu_lock); \
> + } \
> + return NOTIFY_OK; \
> + } \
> + static struct notifier_block name##_lg_cpu_notifier = { \
> + .notifier_call = name##_lg_cpu_callback, \
> + }; \
> void name##_lock_init(void) { \
> int i; \
> LOCKDEP_INIT_MAP(&name##_lock_dep_map, #name, &name##_lock_key, 0); \
> @@ -83,6 +106,11 @@
> lock = &per_cpu(name##_lock, i); \
> *lock = (arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED; \
> } \
> + register_hotcpu_notifier(&name##_lg_cpu_notifier); \
> + get_online_cpus(); \
> + for_each_online_cpu(i) \
> + cpu_set(i, name##_cpus); \
> + put_online_cpus(); \
> } \
> EXPORT_SYMBOL(name##_lock_init); \
> \
> @@ -124,9 +152,9 @@
> \
> void name##_global_lock_online(void) { \
> int i; \
> - preempt_disable(); \
> + spin_lock(&name##_cpu_lock); \
> rwlock_acquire(&name##_lock_dep_map, 0, 0, _RET_IP_); \
> - for_each_online_cpu(i) { \
> + for_each_cpu(i, &name##_cpus) { \
> arch_spinlock_t *lock; \
> lock = &per_cpu(name##_lock, i); \
> arch_spin_lock(lock); \
> @@ -137,12 +165,12 @@
> void name##_global_unlock_online(void) { \
> int i; \
> rwlock_release(&name##_lock_dep_map, 1, _RET_IP_); \
> - for_each_online_cpu(i) { \
> + for_each_cpu(i, &name##_cpus) { \
> arch_spinlock_t *lock; \
> lock = &per_cpu(name##_lock, i); \
> arch_spin_unlock(lock); \
> } \
> - preempt_enable(); \
> + spin_unlock(&name##_cpu_lock); \
> } \
> EXPORT_SYMBOL(name##_global_unlock_online); \
> \
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists