Re: [PATCH 6/6] fs: Make bind mounts work with bdev_allow_write_mounted=n

From: Christian Brauner <brauner@kernel.org>
To: Jan Kara <jack@suse.cz>
Cc: linux-fsdevel@vger.kernel.org, linux-block@vger.kernel.org,
	Christoph Hellwig <hch@infradead.org>,
	Jens Axboe <axboe@kernel.dk>, Kees Cook <keescook@google.com>,
	Ted Tso <tytso@mit.edu>, syzkaller <syzkaller@googlegroups.com>,
	Alexander Popov <alex.popov@linux.com>,
	Eric Biggers <ebiggers@google.com>,
	linux-xfs@vger.kernel.org, linux-btrfs@vger.kernel.org,
	Dmitry Vyukov <dvyukov@google.com>
Subject: Re: [PATCH 6/6] fs: Make bind mounts work with bdev_allow_write_mounted=n
Date: Wed, 5 Jul 2023 15:46:19 +0200	[thread overview]
Message-ID: <20230705-pumpwerk-vielversprechend-a4b1fd947b65@brauner> (raw)
In-Reply-To: <20230705130033.ttv6rdywj5bnxlzx@quack3>

On Wed, Jul 05, 2023 at 03:00:33PM +0200, Jan Kara wrote:
> On Tue 04-07-23 15:59:41, Christian Brauner wrote:
> > On Tue, Jul 04, 2023 at 02:56:54PM +0200, Jan Kara wrote:
> > > When we don't allow opening of mounted block devices for writing, bind
> > > mounting is broken because the bind mount tries to open the block device
> > 
> > Sorry, I'm going to be annoying now...
> > 
> > Afaict, the analysis is misleading but I'm happy to be corrected ofc.
> 
> I'm not sure what your objection exactly is. Probably I was imprecise in my
> changelog description. What gets broken by not allowing RW open of a
> mounted block device is:
> 
> mount -t ext4 /dev/sda1 /mnt1
> mount -t ext4 /dev/sda1 /mnt2
> 
> The second mount should create another mount of the superblock created by
> the first mount but before that is done, get_tree_bdev() tries to open the
> block device and fails when only patches 1 & 2 are applied. This patch
> fixes that.

My objection is that this has nothing to do with mounts but with
superblocks. :) No mount need exist for this issue to appear. And I would
prefer if we keep superblock and mount separate as this leads to unclear
analysis and changelogs.

> 
> > Finding an existing superblock is independent of mounts. get_tree_bdev()
> > and mount_bdev() are really only interested in finding a matching
> > superblock independent of whether or not a mount for it already exists.
> > IOW, if you had two filesystem contexts for the same block device with
> > different mount options:
> > 
> > T1								T2
> > fd_fs = fsopen("ext4");						fd_fs = fsopen("ext4");
> > fsconfig(fd_fs, FSCONFIG_SET_STRING, "source", "/dev/sda");	fsconfig(fd_fs, FSCONFIG_SET_STRING, "source", "/dev/sda");
> > 
> > // create superblock
> > fsconfig(fd_fs, FSCONFIG_CMD_CREATE, ...)
> > 								// finds superblock of T1 if opts are compatible
> > 								fsconfig(fd_fs, FSCONFIG_CMD_CREATE, ...)
> > 
> > you should have the issue that you're describing.
> 
> Correct, this will get broken when not allowing RW open for mounted block
> devices as well because the second fsconfig(fd_fs, FSCONFIG_CMD_CREATE,
> ...) will fail to open the block device in get_tree_bdev(). But again this
> patch should fix that.
> 
> > But for neither of them does a mount already exist as the first mount
> > here would only be created when:
> > 
> > T1								T2
> > fsmount(fd_fs);							fsmount(fd_fs);
> > 
> > is called at which point the whole superblock issue is already settled.
> > Afterwards, both mounts of both T1 and T2 refer to the same superblock -
> > as long as the fs and the mount options support this ofc.
> 
> I guess the confusion comes from me calling "mount" an operation as
> performed by the mount(8) command but which is in fact multiple operations
> with the new mount API. Anyway, is the motivation of this patch clearer
> now?

I'm clear about what you're doing here. I would just like to not have
mounts brought into the changelog. Even before the new mount api what
you were describing was technically a superblock only issue. If someone
reads the changelog I want them to be able to clearly see that this is a
fix for superblocks, not mounts.

Especially, since the code you touch really only has to to with
superblocks.
Let me - non ironically - return the question: Is my own request clearer
now?