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.
