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

[Linus Torvalds]

On Mon, Dec 7, 2015 at 10:05 AM, Suzuki K. Poulose
<suzuki.poulose@xxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/