linux-kernel - Re: Is spin_is_locked() safe to use with BUG_ON()/WARN

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+55aFww-UYfLA4cN6MSkeXzmAnAruShddAPegCpn+5yd1HGmQ@mail.gmail.com>
Date:	Thu, 23 May 2013 07:59:38 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	David Howells <dhowells@...hat.com>
Cc:	Ingo Molnar <mingo@...nel.org>, milosz@...in.com,
	"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: Is spin_is_locked() safe to use with BUG_ON()/WARN_ON()?

On Thu, May 23, 2013 at 7:50 AM, David Howells <dhowells@...hat.com> wrote:
>
> We are using spin_is_locked() in a few places to give a warning or an oops if
> either a spinlock is not held or if it is held.  I'm not sure all of these are
> safe.

No, they're not. On SMP, you can get spurious "it's locked" (because
somebody *else* took the lock on another CPU) and on UP you'll always
get "it's unlocked".

So it's never safe to check the state, at least not without checking
for SMP or UP (and realizing that in the SMP case you can only assert
that it's held).

I guess we could change the UP case to always return "it's locked".
But since you'd better know what you're doing with "spin_is_locked()",
I don't think it's worth it making it easier to use.

> Take uas_try_complete() in drivers/usb/storage/uas.c which does:
>
>         WARN_ON(!spin_is_locked(&devinfo->lock));

Pure garbage. That's a debug thing that should not exist.

> or fscache_start_operations() which does:
>
>         ASSERT(spin_is_locked(&object->lock));

Same thing.

We do *not* want to add some crazy "spin_is_nt_locked". We just want
to get rid of these idiotic debug tests.

Note that even on SMP, spin_is_locked() can end up being bad. If this
whole memory transaction thing takes off, testing the lock is possibly
going to abort the transaction.

So I'd suggest removing it entirely. Drivers have absolutely no place
doing crap like this. We could add some particular
"assert_spin_lock_held()" that only ends up existing if spinlock
debugging is enabled or something, and make it clear that it is purely
a debug feature (and it verifies that *this* process holds the lock,
using the debug fields), not a "test if something is locked" or not.

             Linus
--
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/