pstore: change mutex locking to spin_locks

From: Don Zickus <[email protected]>

pstore was using mutex locking to protect read/write access to the backend plug-ins. This causes problems when pstore is executed in an NMI context through panic() -> kmsg_dump().

This patch changes the mutex to a spin_lock_irqsave then also checks to see if we are in an NMI context. If we are in an NMI and can't get the lock, just print a message stating that and blow by the locking.

All this is probably a hack around the bigger locking problem but it solves my current situation of trying to sleep in an NMI context.

Tested by loading the lkdtm module and executing a HARDLOCKUP which will cause the machine to panic inside the nmi handler.

Signed-off-by Don Zickus <[email protected]>

Signed-off-by Tony Luck <[email protected]>

From: Don Zickus <[email protected]>

pstore was using mutex locking to protect read/write access to the backend plug-ins. This causes problems when pstore is executed in an NMI context through panic() -> kmsg_dump().

This patch changes the mutex to a spin_lock_irqsave then also checks to see if we are in an NMI context. If we are in an NMI and can't get the lock, just print a message stating that and blow by the locking.

All this is probably a hack around the bigger locking problem but it solves my current situation of trying to sleep in an NMI context.

Tested by loading the lkdtm module and executing a HARDLOCKUP which will cause the machine to panic inside the nmi handler.

...

fs/pstore/platform.c +6 -0

+	if (in_nmi()) {
+		is_locked = spin_trylock(&psinfo->buf_lock);
+		if (!is_locked)
+			pr_err("pstore dump routine blocked in NMI, may corrupt error record\n");
+	} else
+		spin_lock_irqsave(&psinfo->buf_lock, flags);
 	oopscount++;
 	while (total < kmsg_bytes) {
 		dst = psinfo->buf;

It's still bad if lockdep is enabled. See kernel/lockdep.c:lock_acquire() and lock_release(). They aren't NMI-safe.

Quoted 3 lines · Andrew Morton

It's still bad if lockdep is enabled. See kernel/lockdep.c:lock_acquire() and lock_release(). They aren't NMI-safe.

Good thing nmi_enter() does lockdep_disable() and makes lock_acquire() bail on if (current->lockdep_recursion).

Still the fact that pstore needs locks from NMI context (let alone tried to use a mutex) makes one think one should avoid it like the plague.

Unfortunately, this plague is called ACPI4 (well APEI)

Quoted 28 lines · Tony

From: Don Zickus <[email protected]>

pstore was using mutex locking to protect read/write access to the backend plug-ins. This causes problems when pstore is executed in an NMI context through panic() -> kmsg_dump().

This patch changes the mutex to a spin_lock_irqsave then also checks to see if we are in an NMI context. If we are in an NMI and can't get the lock, just print a message stating that and blow by the locking.

All this is probably a hack around the bigger locking problem but it solves my current situation of trying to sleep in an NMI context.

Tested by loading the lkdtm module and executing a HARDLOCKUP which will cause the machine to panic inside the nmi handler.

...

fs/pstore/platform.c +6 -0

+	if (in_nmi()) {
+		is_locked = spin_trylock(&psinfo->buf_lock);
+		if (!is_locked)
+			pr_err("pstore dump routine blocked in NMI, may corrupt error record\n");
+	} else
+		spin_lock_irqsave(&psinfo->buf_lock, flags);
 	oopscount++;
 	while (total < kmsg_bytes) {
 		dst = psinfo->buf;

fs/pstore/platform.c static void pstore_dump(struct kmsg_dumper *dumper, · line 139

Quoted 10 lines

fs/pstore/platform.c +5 -1

@@ -123,7 +131,11 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 		total += l1_cpy + l2_cpy;
 		part++;
 	}
-	mutex_unlock(&psinfo->buf_mutex);
+	if (in_nmi()) {
+		if (is_locked)
+			spin_unlock(&psinfo->buf_lock);
+	} else
+		spin_unlock_irqrestore(&psinfo->buf_lock, flags);
 }

It's still bad if lockdep is enabled. See kernel/lockdep.c:lock_acquire() and lock_release(). They aren't NMI-safe.

One approach would be to switch to bit_spin_lock(). Which will break if/when bit spinlocks get lockdep-enabled, so don't do that.

A better approach would be to use the underlying spinlock functions which bypass lockdep, but I cannot immediately locate those amongst the misama of spinlock interface mess.

This problem of locking-vs-NMIs has been "solved" several times before but I don't recall any standardized approach being developed. Does anyone have a favorite implementation to look at?

Probably the raw_spin_* stuff. I think those purposely avoid the lockdep mechanisms.

But in this case we have a filesystem that can be read/written to from a normal context and also written to from an NMI context

Quoted 2 lines

But in this case we have a filesystem that can be read/written to from a normal context and also written to from an NMI context

I fixed (avoided) the problem of writing to the pstore filesystem in NMI context in my other pstore patch in linux-next ("defer inserting OOPS entries in pstore").

So the remaining locking requirement in pstore is just to protect the non-volatile backing store from simultaneous access.