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

[Jan Kara <jack@suse.cz>]

On Fri 22-03-24 06:33:46, Al Viro wrote:
> 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.

Well, there are a couple of places where we end up reading bd_openers
without ->open_mutex. Sure these places are racy wrt other opens / closes
so they need to be careful but we want to make sure we read back at least
some sane value which is not guaranteed with normal int and compiler
possily playing weird tricks when updating it. But yes, we could convert
the atomic_t to using READ_ONCE + WRITE_ONCE in appropriate places to avoid
these issues and make it more obvious bd_openers are not really handled in
an atomic way.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR