Re: [RFC v4 linux-next 19/19] fs & block: remove bdev->bd_inode

[Al Viro <viro@zeniv.linux.org.uk>]

On Tue, Mar 19, 2024 at 04:26:19PM +0800, Yu Kuai wrote:

> +void put_bdev_file(struct block_device *bdev)
> +{
> +       struct file *file = NULL;
> +       struct inode *bd_inode = bdev_inode(bdev);
> +
> +       mutex_lock(&bdev->bd_disk->open_mutex);
> +       file = bd_inode->i_private;
> +
> +       if (!atomic_read(&bdev->bd_openers))
> +               bd_inode->i_private = NULL;
> +
> +       mutex_unlock(&bdev->bd_disk->open_mutex);
> +
> +       fput(file);
> +}

Locking is completely wrong here.  The only thing that protects
->bd_openers is ->open_mutex.  atomic_read() is obviously a red
herring.

Suppose another thread has already opened the same sucker
with bdev_file_open_by_dev().

Now you are doing the same thing, just as the other guy is
getting to bdev_release() call.

The thing is, between your get_bdev_file() and increment of ->bd_openers
(in bdev_open()) there's a window when bdev_release() of the old file
could've gotten all the way through the decrement of ->bd_openers
(to 0, since our increment has not happened yet) and through the
call of put_bdev_file(), which ends up clearing ->i_private.

End result:

* old ->i_private leaked (already grabbed by your get_bdev_file())
* ->bd_openers at 1 (after your bdev_open() gets through)
* ->i_private left NULL.

Christoph, could we please get rid of that atomic_t nonsense?
It only confuses people into brainos like that.  It really
needs ->open_mutex for any kind of atomicity.