linux-kernel - Re: [patch] module: potential deadlock in error path

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]

Date:	Sun, 20 Jan 2013 20:37:36 -0800
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	Dan Carpenter <dan.carpenter@...cle.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	kernel-janitors <kernel-janitors@...r.kernel.org>,
	Alexander Graf <agraf@...e.de>,
	Prarit Bhargava <prarit@...hat.com>,
	Sasha Levin <sasha.levin@...cle.com>
Subject: Re: [patch] module: potential deadlock in error path

On Sun, Jan 20, 2013 at 7:52 PM, Rusty Russell <rusty@...tcorp.com.au> wrote:
>
> You've now conflated two completely different lock paths into a single
> unlock.

We have that elsewhere too. And it's what we used to have before too.

So the simple fact is that commit 1fb9341ac348 just introduced this
bug, and moving the goto target around is the obvious fix for it, and
makes it match the old code that was simply incorrectly modified.

The suggested patch instead has *some* cleanup inside the
if-statement, and some at the goto target. That makes no sense to
humans, and just makes it harder for the compiler to generate better
code.

> mutex_bug_cleanup() should really lock internally, but doesn't
> so we wrap it.  And that mutex_unlock of yours has nothing to do with
> cleaning up ddebug, so the labels misnamed, at best.

Bah, humbug. It's called "ddebug_cleanup" because it's called after
the  debug setup, so it needs to clean up the state set up by that.
The fact that it needs to unlock is secondary, and is simply because
the lock is taken at that point, so needs to be released. The naming
is not wonderful, but it's not hugely illogical, and again, that's
what it used to (except "ddebug" has been renamed to
"ddebug_cleanup"). You could rename it if you want to (we used to have
a target called "unlock" at that point), but that's *still* no excuse
for just creating code that does cleanup in two totally unrelated
places.

> Not that it matters much: this is going to change for next merge window.

Now, agreed, that looks better, although I suspect you could have
taken the "split that ugly function up" further still.

                   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/