Re: [PATCH] fs: open the block device after allocation the super_block

[Christian Brauner <brauner@kernel.org>]

On Tue, Aug 15, 2023 at 04:43:12PM +0200, Christian Brauner wrote:
> >  		up_write(&s->s_umount);
> > -		blkdev_put(bdev, fs_type);
> > +		error = setup_bdev_super(s, flags, NULL);
> >  		down_write(&s->s_umount);
> 
> So I've been looking through the branches to see what's ready for v6.6
> and what needs some more time. While doing so I went over this again and
> realized that we have an issue here.
> 
> While it looks like dropping s_umount here and calling
> setup_bdev_super() is fine I think it isn't. Consider two processes
> racing to create the same mount:
> 
> P1                                                                    P2
> vfs_get_tree()                                                        vfs_get_tree()
> -> get_tree() == get_tree_bdev()                                      -> get_tree() == get_tree_bdev()
>    -> sget_fc()                                                          -> sget_fc()
>         // allocate new sb; no matching sb found
>       -> sb_p1 = alloc_super()
>       -> hlist_add_head(&s->s_instances, &s->s_type->fs_supers)
>       -> spin_unlock(&sb_lock)                                              
>       // yield s_umount to avoid deadlocks
>    -> up_write(&sb->s_umount)
>                                                                             -> spin_lock(&sb_lock)
>                                                                                // find sb_p1
>                                                                                if (test(old, fc))
>                                                                                        goto share_extant_sb;
>       // Assume P1 sleeps on bdev_lock or open_mutex
>       // in blkdev_get_by_dev().
>    -> setup_bdev_super()
>    -> down_write(&sb->s_umount)
> 
> Now P2 jumps to the share_extant_sb label and calls:
> 
> grab_super(sb_p1)
> -> spin_unlock(&sb_lock)
> -> down_write(&s->s_umount)
> 
> Since s_umount is unlocked P2 doesn't go to sleep and instead immediately
> goes to retry by jumping to the "retry" label. If P1 is still sleeping
> on a a bdev mutex the same thing happens again.
> 
> So if you have a range of processes P{1,n} that all try to mount the
> same device you're hammering endlessly on sb_lock without ever going to
> sleep like we used to. The same problem exists for all iterate_supers()

That part is wrong. If you have P{1,n} and P1 takes s_umount exclusively
then P{2,n} will sleep on s_umount until P1 is done. But there's still
at least on process spinning through sget_fc() for no good reason.