linux-kernel - Re: [PATCH] blkdev: Fix blkdev_open to release the bdev on error

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+55aFwSpKo_v4w_TfQRcR2CzvDG6W_woGV60bwxshpWfwGtjw@mail.gmail.com>
Date:	Mon, 7 Dec 2015 10:49:05 -0800
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	"Suzuki K. Poulose" <suzuki.poulose@....com>
Cc:	Al Viro <viro@...iv.linux.org.uk>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	Marc Zyngier <marc.zyngier@....com>, Tejun Heo <tj@...nel.org>,
	stable <stable@...r.kernel.org>
Subject: Re: [PATCH] blkdev: Fix blkdev_open to release the bdev on error

On Mon, Dec 7, 2015 at 10:05 AM, Suzuki K. Poulose
<suzuki.poulose@....com> wrote:
> blkdev_open() doesn't release the bdev, it attached to a given
> inode, if blkdev_get() fails (e.g, due to absence of a device).
> This can cause kernel crashes when the original filesystem
> tries to flush the data during evict_inode.

Ugh. This code is a mess. Al, can you please comment?

So what happens is that when "blkdev_get()" fails, it will do a
bdput() on the bdev.

But blkdev_open() hasn't done a bdget(). It's done a bd_acquire().
Which will do the whole "add inodes to bd_inodes". And yes,
bd_forget() will undo that.

HOWEVER.

bd_forget() will undo that unconditionally, but bd_acquire() has *not*
unconditionally done that bd_inodes list operation. It might already
have been there.

So as far as I can tell, the patch here undoes things potentially too much.

Shouldn't the last bdput() already end up doing a bd_forget()? We'd have

 bdput -> iput -> iput_final -> evict -> bd_forget.

but the fact that Suzuki shows an oops clearly shows that something is
badly wrong.

IOW, the path looks simple and apparently fixes an oops, but I'd like
much more of an explanation for what happens, because it all feels
wrong to me. Why doesn't the bdput() end up undoing the bd_acquire()
properly?

                  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/