| Message ID | 1404207001-7510-1-git-send-email-quwenruo@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
(adding Harald to CC) On Tue, Jul 01, 2014 at 05:30:01PM +0800, Qu Wenruo wrote: > This reverts commit 0723a0473fb48a1c93b113a28665b64ce5faf35a. > This commit has the following problem: > 1) Break the ro mount rule. > When users mount the whole btrfs ro, it is still possible to mount > subvol rw and change the contents. Which make the whole fs ro mount > non-sense. The proposed usecase was to allow mounting subvolumes with different ro/rw flags, and this makes sense to me (provided that the whole filesystem is mounted rw). Anything else seems to lead to all the internal problems you point below. I'm not even sure if mounting the first subvolume 'ro' should imply that the whole filesystem is ro or not. > 2) Cause whole btrfs ro/rw mount change fails. > When mount a subvol ro first, when you can't mount the whole fs mounted > rw. This is due to the check in btrfs_mount() which returns -EBUSY, > which is OK for single fs to prevent mount fs ro in one mount point and > mount the same fs rw in other mount point. > Step to reproduce: > mount -o subvol=subv,ro /dev/sda6 /mnt/btrfs > mount -o rw /dev/sda6 /mnt/btrfs <-this will fail Yeah, so first ro means whole filesystem is ro. > 3) Kernel warn in vfs. > When mount the whole fs ro, and mount a subvol ro, kernel warning will > show in fs/sync.c complaining s_umount rwsem is not locked. > Since this remount is not called by VFS, so s_mounts rwsem is not > correctly locked. That's serious. > 4) Lacks ro/rw conflicint check. > The commit uses a trick to 'cheat' vfs about ro/rw flags to allow > different ro/rw mount options, however this breaks the original ro/rw > conflicting rule: one fs(even subvolume) can't be mounted rw in one place > and mounted ro in someplace else. > This can be triggered quite easily: > mount -o subvol=subv,ro /dev/sda6 /mnt/btrfs > mount -o subvol=subv,rw /dev/sda6 /mnt/other Hm, and that's actually what the bind mount is for, and this was mentioned in the patch changelog as a workaround. > Also the check logical about checking the ro/rw needed to be investigated > further. e.g. When the subvolume is mounted ro, the whole btrfs fs is mounted > rw, so one can change the ro mounted subvolume from the mounted whole btrfs. I agree, the semantics is not clear and the code does not cover all cases. > Due to the above reasons, the per-subvolume ro/rw mount options should be > investigated further to ensure the correctness and better reverting it > before correct implement > (It may takes a lone time to find the right logic about ro/rw option in > subvolume) The different ro/rw feature is convenient, but unless the implementation covers all the points above, I'm afraid that the revert is the best option right now. Ack for the revert. (Yeah I was pushing the patch upstream but ...) -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/01/2014 11:32 AM, David Sterba wrote: > (adding Harald to CC) > > On Tue, Jul 01, 2014 at 05:30:01PM +0800, Qu Wenruo wrote: >> This reverts commit 0723a0473fb48a1c93b113a28665b64ce5faf35a. >> This commit has the following problem: >> 1) Break the ro mount rule. >> When users mount the whole btrfs ro, it is still possible to mount >> subvol rw and change the contents. Which make the whole fs ro mount >> non-sense. > > The proposed usecase was to allow mounting subvolumes with different > ro/rw flags, and this makes sense to me (provided that the whole > filesystem is mounted rw). > > Anything else seems to lead to all the internal problems you point > below. I'm not even sure if mounting the first subvolume 'ro' should > imply that the whole filesystem is ro or not. > >> 2) Cause whole btrfs ro/rw mount change fails. >> When mount a subvol ro first, when you can't mount the whole fs mounted >> rw. This is due to the check in btrfs_mount() which returns -EBUSY, >> which is OK for single fs to prevent mount fs ro in one mount point and >> mount the same fs rw in other mount point. >> Step to reproduce: >> mount -o subvol=subv,ro /dev/sda6 /mnt/btrfs >> mount -o rw /dev/sda6 /mnt/btrfs <-this will fail > > Yeah, so first ro means whole filesystem is ro. > >> 3) Kernel warn in vfs. >> When mount the whole fs ro, and mount a subvol ro, kernel warning will >> show in fs/sync.c complaining s_umount rwsem is not locked. >> Since this remount is not called by VFS, so s_mounts rwsem is not >> correctly locked. > > That's serious. Agreed, we'll pull this out until we get a better handle on things. Thanks for spending time on it. -chris -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01.07.2014 18:36, Chris Mason wrote: > On 07/01/2014 11:32 AM, David Sterba wrote: >> (adding Harald to CC) >> >> On Tue, Jul 01, 2014 at 05:30:01PM +0800, Qu Wenruo wrote: >>> This reverts commit 0723a0473fb48a1c93b113a28665b64ce5faf35a. >>> This commit has the following problem: >>> 1) Break the ro mount rule. >>> When users mount the whole btrfs ro, it is still possible to mount >>> subvol rw and change the contents. Which make the whole fs ro mount >>> non-sense. >> >> The proposed usecase was to allow mounting subvolumes with different >> ro/rw flags, and this makes sense to me (provided that the whole >> filesystem is mounted rw). >> >> Anything else seems to lead to all the internal problems you point >> below. I'm not even sure if mounting the first subvolume 'ro' should >> imply that the whole filesystem is ro or not. >> >>> 2) Cause whole btrfs ro/rw mount change fails. >>> When mount a subvol ro first, when you can't mount the whole fs mounted >>> rw. This is due to the check in btrfs_mount() which returns -EBUSY, >>> which is OK for single fs to prevent mount fs ro in one mount point and >>> mount the same fs rw in other mount point. >>> Step to reproduce: >>> mount -o subvol=subv,ro /dev/sda6 /mnt/btrfs >>> mount -o rw /dev/sda6 /mnt/btrfs <-this will fail >> >> Yeah, so first ro means whole filesystem is ro. >> >>> 3) Kernel warn in vfs. >>> When mount the whole fs ro, and mount a subvol ro, kernel warning will >>> show in fs/sync.c complaining s_umount rwsem is not locked. >>> Since this remount is not called by VFS, so s_mounts rwsem is not >>> correctly locked. >> >> That's serious. > > Agreed, we'll pull this out until we get a better handle on things. > Thanks for spending time on it. > > -chris > My patch was initially only a request for comments: - patch pointed out the problem - provided a possible solution/workaround - even has "FIXME" in the code :) So, what I was hoping, that somebody else with more VFS knowledge than me would step up and come up with a sane solution. Pull it out, if the patch causes problems, but _please_ think about the problem and come up with a solution, so that "mount -a" works with a normal fstab. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Harald Hoyer posted on Wed, 02 Jul 2014 09:59:15 +0200 as excerpted: > Pull it out, if the patch causes problems, but _please_ think about the > problem and come up with a solution, so that "mount -a" works with a > normal fstab. FWIW, systemd makes it a LOT easier to manage multi-stage mounts with fstab automounts because with its mount-dependencies, in most cases it "just works". My previous solution, gentoo so sysvinit with openrc mounting, required multiple mount stage scripts and painstaking dependency editing. It worked, but it was a pain! Finding out it pretty much "just worked" on systemd (tho I did have to manually add a single mount-dependency line to a single service unit, due to a cross-mount-point symlink that systemd didn't consider, but documentation was easy to find and the line did exactly what the documentation said it would do) was my most pleasant surprise of the switch! =:^)
-------- Original Message -------- Subject: Re: [RFC PATCH] Revert "btrfs: allow mounting btrfs subvolumes with different ro/rw options" From: Harald Hoyer <harald@redhat.com> To: Chris Mason <clm@fb.com>, dsterba@suse.cz, Qu Wenruo <quwenruo@cn.fujitsu.com>, linux-btrfs@vger.kernel.org Date: 2014?07?02? 15:59 > On 01.07.2014 18:36, Chris Mason wrote: >> On 07/01/2014 11:32 AM, David Sterba wrote: >>> (adding Harald to CC) >>> >>> On Tue, Jul 01, 2014 at 05:30:01PM +0800, Qu Wenruo wrote: >>>> This reverts commit 0723a0473fb48a1c93b113a28665b64ce5faf35a. >>>> This commit has the following problem: >>>> 1) Break the ro mount rule. >>>> When users mount the whole btrfs ro, it is still possible to mount >>>> subvol rw and change the contents. Which make the whole fs ro mount >>>> non-sense. >>> The proposed usecase was to allow mounting subvolumes with different >>> ro/rw flags, and this makes sense to me (provided that the whole >>> filesystem is mounted rw). >>> >>> Anything else seems to lead to all the internal problems you point >>> below. I'm not even sure if mounting the first subvolume 'ro' should >>> imply that the whole filesystem is ro or not. >>> >>>> 2) Cause whole btrfs ro/rw mount change fails. >>>> When mount a subvol ro first, when you can't mount the whole fs mounted >>>> rw. This is due to the check in btrfs_mount() which returns -EBUSY, >>>> which is OK for single fs to prevent mount fs ro in one mount point and >>>> mount the same fs rw in other mount point. >>>> Step to reproduce: >>>> mount -o subvol=subv,ro /dev/sda6 /mnt/btrfs >>>> mount -o rw /dev/sda6 /mnt/btrfs <-this will fail >>> Yeah, so first ro means whole filesystem is ro. >>> >>>> 3) Kernel warn in vfs. >>>> When mount the whole fs ro, and mount a subvol ro, kernel warning will >>>> show in fs/sync.c complaining s_umount rwsem is not locked. >>>> Since this remount is not called by VFS, so s_mounts rwsem is not >>>> correctly locked. >>> That's serious. >> Agreed, we'll pull this out until we get a better handle on things. >> Thanks for spending time on it. >> >> -chris >> > My patch was initially only a request for comments: > - patch pointed out the problem > - provided a possible solution/workaround > - even has "FIXME" in the code :) > > So, what I was hoping, that somebody else with more VFS knowledge than me would > step up and come up with a sane solution. > > Pull it out, if the patch causes problems, but _please_ think about the problem > and come up with a solution, so that "mount -a" works with a normal fstab. Personally, the main points to implement the feature are the following: 1) ro/rw logical (how ro and rw works) IMO, any subvol can't be mounted rw if any of its parent is mounted ro would be good enough. So whole btrfs ro mount will not allow any rw subvol mount, and rw subvol will allow child subvol mounted as ro. For me, the logical thing is not a problem. 2) subvol umount (real hard part to implement) If we choose 1) logical, then we must store the rw/ro mount option will the subvolid when mount and clear rw/ro mount option when *umounting the subvol*. Mounting part can be easily done in fs/btrfs/super.c:get_default_root(), but the problem is that when umount, we have no hook or callback to imform btrfs the subvol umount( I hope I was wrong). It would be quite nice if someone has some good idea about this problem. Thanks, Qu -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/01/2014 11:30 AM, Qu Wenruo wrote: > This commit has the following problem: > 1) Break the ro mount rule. > When users mount the whole btrfs ro, it is still possible to mount > subvol rw and change the contents. Which make the whole fs ro mount > non-sense. Where is the problem ? I see an use case when I want a conservative default: mount all ro except some subvolumes. In any case it is not a security problem because if the user has the capability to mount a subvolume, also he has the capability to remount,rw the whole filesystem.
-------- Original Message -------- Subject: Re: [RFC PATCH] Revert "btrfs: allow mounting btrfs subvolumes with different ro/rw options" From: Goffredo Baroncelli <kreijack@libero.it> To: Qu Wenruo <quwenruo@cn.fujitsu.com>, linux-btrfs@vger.kernel.org Date: 2014?07?03? 01:48 > On 07/01/2014 11:30 AM, Qu Wenruo wrote: >> This commit has the following problem: >> 1) Break the ro mount rule. >> When users mount the whole btrfs ro, it is still possible to mount >> subvol rw and change the contents. Which make the whole fs ro mount >> non-sense. > Where is the problem ? I see an use case when I want a conservative default: mount all ro except some subvolumes. > > In any case it is not a security problem because if the user has the capability to mount a subvolume, also he has the capability to remount,rw the whole filesystem. > > > Not security problem but behavior not consistent. If user mount the whole disk ro, he or she want the fs read only and nothing will change in it. If you mount a subvol rw, then the whole disk ro expectation is broken. Things will change even the whole disk is readonly. The problem also happens when a parent subvol is mounted rw but child subvol is mounted ro. User can still modify the child subvol through parent subvol, still broke the readonly rule. Thanks, Qu -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/2/14, 3:59 AM, "Harald Hoyer" <harald@redhat.com> wrote: >On 01.07.2014 18:36, Chris Mason wrote: >> On 07/01/2014 11:32 AM, David Sterba wrote: >>> (adding Harald to CC) >>> >>> On Tue, Jul 01, 2014 at 05:30:01PM +0800, Qu Wenruo wrote: >>>> This reverts commit 0723a0473fb48a1c93b113a28665b64ce5faf35a. >>>> This commit has the following problem: >>>> 1) Break the ro mount rule. >>>> When users mount the whole btrfs ro, it is still possible to mount >>>> subvol rw and change the contents. Which make the whole fs ro mount >>>> non-sense. >>> >>> The proposed usecase was to allow mounting subvolumes with different >>> ro/rw flags, and this makes sense to me (provided that the whole >>> filesystem is mounted rw). >>> >>> Anything else seems to lead to all the internal problems you point >>> below. I'm not even sure if mounting the first subvolume 'ro' should >>> imply that the whole filesystem is ro or not. >>> >>>> 2) Cause whole btrfs ro/rw mount change fails. >>>> When mount a subvol ro first, when you can't mount the whole fs >>>>mounted >>>> rw. This is due to the check in btrfs_mount() which returns -EBUSY, >>>> which is OK for single fs to prevent mount fs ro in one mount point >>>>and >>>> mount the same fs rw in other mount point. >>>> Step to reproduce: >>>> mount -o subvol=subv,ro /dev/sda6 /mnt/btrfs >>>> mount -o rw /dev/sda6 /mnt/btrfs <-this will fail >>> >>> Yeah, so first ro means whole filesystem is ro. >>> >>>> 3) Kernel warn in vfs. >>>> When mount the whole fs ro, and mount a subvol ro, kernel warning will >>>> show in fs/sync.c complaining s_umount rwsem is not locked. >>>> Since this remount is not called by VFS, so s_mounts rwsem is not >>>> correctly locked. >>> >>> That's serious. >> >> Agreed, we'll pull this out until we get a better handle on things. >> Thanks for spending time on it. >> >> -chris >> > >My patch was initially only a request for comments: >- patch pointed out the problem >- provided a possible solution/workaround >- even has "FIXME" in the code :) > >So, what I was hoping, that somebody else with more VFS knowledge than me >would >step up and come up with a sane solution. > >Pull it out, if the patch causes problems, but _please_ think about the >problem >and come up with a solution, so that "mount -a" works with a normal fstab. Definitely, we don¹t want to drop this functionality over the long term. The biggest problem here is the vfs warning. Let me take another look and see if I can get the locking right. -chris -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[List CCd. I hate Gmail.] Noob alert. On 3 July 2014 02:28, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote: > Subject: Re: [RFC PATCH] Revert "btrfs: allow mounting btrfs subvolumes w= ith > different ro/rw options" > From: Goffredo Baroncelli <kreijack@libero.it> > To: Qu Wenruo <quwenruo@cn.fujitsu.com>, linux-btrfs@vger.kernel.org > Date: 2014=E5=B9=B407=E6=9C=8803=E6=97=A5 01:48 >> >> On 07/01/2014 11:30 AM, Qu Wenruo wrote: >>> >>> This commit has the following problem: >>> 1) Break the ro mount rule. >>> When users mount the whole btrfs ro, it is still possible to mount >>> subvol rw and change the contents. Which make the whole fs ro mount >>> non-sense. >> >> Where is the problem ? I see an use case when I want a conservative >> default: mount all ro except some subvolumes. >> >> In any case it is not a security problem because if the user has the >> capability to mount a subvolume, also he has the capability to remount,r= w >> the whole filesystem. >> >> >> > Not security problem but behavior not consistent. > If user mount the whole disk ro, he or she want the fs read only and noth= ing > will change in it. > If you mount a subvol rw, then the whole disk ro expectation is broken. > Things will change even the whole > disk is readonly. This assumption seems wrong and untenable if considered from a different angle: one doesn't mount the "whole disk" ro, merely the default subvolume. # mount -o ro /dev/sda1 /mnt is merely convenient short-hand for # mount -o ro,subvol=3D@ [or whatever] /dev/sda1 /mnt and anyone who expects this to magically protect the whole disk is, frankly, confused. Substituting partitions for subvolumes: mounting /dev/sda2 read-only should have no effect on /dev/sda3. Even if you went a bit batty and decided to make /dev/sda2 the "default partition": # ln -sf /dev/sda2 /dev/sda # mount -o ro /dev/sda /mnt/this/is/silly syntactic sugar doesn't change anything. Subvolumes are logically discrete entities, the fact that they share trees on-disk is merely a (very nice) implementation detail. It is impossible to mount a "whole disk" under btrfs. Tobias > The problem also happens when a parent subvol is mounted rw but child sub= vol > is mounted ro. > User can still modify the child subvol through parent subvol, still broke > the readonly rule. This makes sense, though. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-------- Original Message -------- Subject: Re: [RFC PATCH] Revert "btrfs: allow mounting btrfs subvolumes with different ro/rw options" From: Tobias Geerinckx-Rice <tobias.geerinckx.rice@gmail.com> To: Qu Wenruo <quwenruo@cn.fujitsu.com> Date: 2014?07?03? 16:06 > [List CCd. I hate Gmail.] > > Noob alert. > > On 3 July 2014 02:28, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote: >> Subject: Re: [RFC PATCH] Revert "btrfs: allow mounting btrfs subvolumes w= > ith >> different ro/rw options" >> From: Goffredo Baroncelli <kreijack@libero.it> >> To: Qu Wenruo <quwenruo@cn.fujitsu.com>, linux-btrfs@vger.kernel.org >> Date: 2014=E5=B9=B407=E6=9C=8803=E6=97=A5 01:48 >>> On 07/01/2014 11:30 AM, Qu Wenruo wrote: >>>> This commit has the following problem: >>>> 1) Break the ro mount rule. >>>> When users mount the whole btrfs ro, it is still possible to mount >>>> subvol rw and change the contents. Which make the whole fs ro mount >>>> non-sense. >>> Where is the problem ? I see an use case when I want a conservative >>> default: mount all ro except some subvolumes. >>> >>> In any case it is not a security problem because if the user has the >>> capability to mount a subvolume, also he has the capability to remount,r= > w >>> the whole filesystem. >>> >>> >>> >> Not security problem but behavior not consistent. >> If user mount the whole disk ro, he or she want the fs read only and noth= > ing >> will change in it. >> If you mount a subvol rw, then the whole disk ro expectation is broken. >> Things will change even the whole >> disk is readonly. > This assumption seems wrong and untenable if considered from a > different angle: one doesn't mount the "whole disk" ro, merely the > default subvolume. > > # mount -o ro /dev/sda1 /mnt > > is merely convenient short-hand for > > # mount -o ro,subvol=3D@ [or whatever] /dev/sda1 /mnt > > and anyone who expects this to magically protect the whole disk is, > frankly, confused. > > Substituting partitions for subvolumes: mounting /dev/sda2 read-only > should have no effect on /dev/sda3. > Even if you went a bit batty and decided to make /dev/sda2 the > "default partition": > > # ln -sf /dev/sda2 /dev/sda > # mount -o ro /dev/sda /mnt/this/is/silly > > syntactic sugar doesn't change anything. > > Subvolumes are logically discrete entities, the fact that they share > trees on-disk is merely a (very nice) implementation detail. It is > impossible to mount a "whole disk" under btrfs. Oh, sorry for my confusing words. To make it clear, when mentioning 'the whole disk(or partition whatever)' I mean the FS_TREE. (Of course not the default subvolume) The problem is that, even you mount a subvolume ro, you can still change contents in the subvolume through its rw parent subvolume. And if a subvolume can still be modified, the ro mount lose it meaning. So we need special rules to prevent such things. Thanks, Qu > > Tobias > >> The problem also happens when a parent subvol is mounted rw but child sub= > vol >> is mounted ro. >> User can still modify the child subvol through parent subvol, still broke >> the readonly rule. > This makes sense, though. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3 July 2014 10:33, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote: > Oh, sorry for my confusing words. And I probably should have waited for my frustration with my mail client/device/public transport to subside before panicking^Creplying. I use a combination of ro & rw (not insanely nested) subvolumes on a few pseudo-embedded home/office servers and would like to keep that arrangement working if possible. I'm also aware that it doesn't protect against all possible bugs. > To make it clear, when mentioning 'the whole disk(or partition whatever)' I mean the FS_TREE. > (Of course not the default subvolume) > > The problem is that, even you mount a subvolume ro, you can still change contents in the subvolume > through its rw parent subvolume. > And if a subvolume can still be modified, the ro mount lose it meaning. That makes so much more sense than my original reading, which was weird and wrong and implied strange subvol-5-only magic. Sorry! > So we need special rules to prevent such things. Not that it matters, but: agreed. Tobias -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/03/2014 02:28 AM, Qu Wenruo wrote: > > -------- Original Message -------- > Subject: Re: [RFC PATCH] Revert "btrfs: allow mounting btrfs subvolumes with different ro/rw options" > From: Goffredo Baroncelli <kreijack@libero.it> > To: Qu Wenruo <quwenruo@cn.fujitsu.com>, linux-btrfs@vger.kernel.org > Date: 2014?07?03? 01:48 >> On 07/01/2014 11:30 AM, Qu Wenruo wrote: >>> This commit has the following problem: >>> 1) Break the ro mount rule. >>> When users mount the whole btrfs ro, it is still possible to mount >>> subvol rw and change the contents. Which make the whole fs ro mount >>> non-sense. >> Where is the problem ? I see an use case when I want a conservative default: mount all ro except some subvolumes. >> >> In any case it is not a security problem because if the user has the capability to mount a subvolume, also he has the capability to remount,rw the whole filesystem. >> >> >> > Not security problem but behavior not consistent. > If user mount the whole disk ro, he or she want the fs read only and nothing will change in it. > If you mount a subvol rw, then the whole disk ro expectation is broken. Things will change even the whole > disk is readonly. Sorry for bother you again, but there is a thing not clear to me: If # mount -o subvolid=5,ro /dev/sda1 /mnt/root # mount -o subvol=subvolname,rw /dev/sda1 /mnt/subvolname I suppose that # touch /mnt/root/touch-test # 1 fails, and # touch /mnt/subvolname/touch-test # 2 succeeded. I understood correctly ? If so this behaviour seems to me correctly. Different is after mounting the subvolume "subvolumename", also the whole filesystem results rw (eg: #1 succeeded). G.Baroncelli > > The problem also happens when a parent subvol is mounted rw but child subvol is mounted ro. > User can still modify the child subvol through parent subvol, still broke the readonly rule. > > Thanks, > Qu >
-------- Original Message -------- Subject: Re: [RFC PATCH] Revert "btrfs: allow mounting btrfs subvolumes with different ro/rw options" From: Goffredo Baroncelli <kreijack@inwind.it> To: Qu Wenruo <quwenruo@cn.fujitsu.com>, linux-btrfs@vger.kernel.org Date: 2014?07?04? 01:37 > On 07/03/2014 02:28 AM, Qu Wenruo wrote: >> -------- Original Message -------- >> Subject: Re: [RFC PATCH] Revert "btrfs: allow mounting btrfs subvolumes with different ro/rw options" >> From: Goffredo Baroncelli <kreijack@libero.it> >> To: Qu Wenruo <quwenruo@cn.fujitsu.com>, linux-btrfs@vger.kernel.org >> Date: 2014?07?03? 01:48 >>> On 07/01/2014 11:30 AM, Qu Wenruo wrote: >>>> This commit has the following problem: >>>> 1) Break the ro mount rule. >>>> When users mount the whole btrfs ro, it is still possible to mount >>>> subvol rw and change the contents. Which make the whole fs ro mount >>>> non-sense. >>> Where is the problem ? I see an use case when I want a conservative default: mount all ro except some subvolumes. >>> >>> In any case it is not a security problem because if the user has the capability to mount a subvolume, also he has the capability to remount,rw the whole filesystem. >>> >>> >>> >> Not security problem but behavior not consistent. >> If user mount the whole disk ro, he or she want the fs read only and nothing will change in it. >> If you mount a subvol rw, then the whole disk ro expectation is broken. Things will change even the whole >> disk is readonly. > Sorry for bother you again, but there is a thing not clear to me: > > If > > # mount -o subvolid=5,ro /dev/sda1 /mnt/root > # mount -o subvol=subvolname,rw /dev/sda1 /mnt/subvolname > > I suppose that > > # touch /mnt/root/touch-test # 1 > > fails, and > > # touch /mnt/subvolname/touch-test # 2 > > succeeded. I understood correctly ? Your understanding is right and that is current behavior. But that should not be the correct behavior. If you mount fs_tree ro, btrfs should ensure the whole fs_tree(including all the subvolumes) ro. Or the whole fs_tree is not restricted readonly since you can modify contents inside the rw subvolume, and it's part of the fs_tree.(partly ro and partly rw status) IMO the perfect logical should be like the following: 1) ro mounted subvolume will force all the children subvolumes only ro mountable subvol 5 (mounted ro /) ??? subvol 257 (mounted rw /mnt/btrfrs) So above mounted should not be allowed. But the following mount should be OK: subvol 5 (mounted rw /) ??? subvol 257 (mounted ro /mnt/btrfrs) 2) ro mounted subvolume will not be modified even through the rw mounted parent subvolume. Only this will ensure restricted ro mount option. If anyone has any other ideas about it, I'm happy to listen. Thanks, Qu > If so this behaviour seems to me correctly. > Different is after mounting the subvolume "subvolumename", also the whole filesystem results rw (eg: #1 succeeded). > > G.Baroncelli > > > > > >> The problem also happens when a parent subvol is mounted rw but child subvol is mounted ro. >> User can still modify the child subvol through parent subvol, still broke the readonly rule. >> >> Thanks, >> Qu >> > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Qu On 07/04/2014 03:28 AM, Qu Wenruo wrote: > > -------- Original Message -------- Subject: Re: [RFC PATCH] Revert > "btrfs: allow mounting btrfs subvolumes with different ro/rw > options" From: Goffredo Baroncelli <kreijack@inwind.it> To: Qu Wenruo > <quwenruo@cn.fujitsu.com>, linux-btrfs@vger.kernel.org Date: 2014?07? > 04? 01:37 >> On 07/03/2014 02:28 AM, Qu Wenruo wrote: >>> -------- Original Message -------- Subject: Re: [RFC PATCH] >>> Revert "btrfs: allow mounting btrfs subvolumes with different >>> ro/rw options" From: Goffredo Baroncelli <kreijack@libero.it> To: >>> Qu Wenruo <quwenruo@cn.fujitsu.com>, linux-btrfs@vger.kernel.org >>> Date: 2014?07?03? 01:48 >>>> On 07/01/2014 11:30 AM, Qu Wenruo wrote: >>>>> This commit has the following problem: 1) Break the ro mount >>>>> rule. When users mount the whole btrfs ro, it is still >>>>> possible to mount subvol rw and change the contents. Which >>>>> make the whole fs ro mount non-sense. >>>> Where is the problem ? I see an use case when I want a >>>> conservative default: mount all ro except some subvolumes. >>>> >>>> In any case it is not a security problem because if the user >>>> has the capability to mount a subvolume, also he has the >>>> capability to remount,rw the whole filesystem. >>>> >>>> >>>> >>> Not security problem but behavior not consistent. If user mount >>> the whole disk ro, he or she want the fs read only and nothing >>> will change in it. If you mount a subvol rw, then the whole disk >>> ro expectation is broken. Things will change even the whole disk >>> is readonly. >> Sorry for bother you again, but there is a thing not clear to me: >> >> If >> >> # mount -o subvolid=5,ro /dev/sda1 /mnt/root # mount -o >> subvol=subvolname,rw /dev/sda1 /mnt/subvolname >> >> I suppose that >> >> # touch /mnt/root/touch-test # 1 >> >> fails, and >> >> # touch /mnt/subvolname/touch-test # 2 >> >> succeeded. I understood correctly ? > Your understanding is right and that is current behavior. > > But that should not be the correct behavior. > > If you mount fs_tree ro, btrfs should ensure the whole > fs_tree(including all the subvolumes) ro. Or the whole fs_tree is not > restricted readonly since you can modify contents inside the rw > subvolume, and it's part of the fs_tree.(partly ro and partly rw > status) > > IMO the perfect logical should be like the following: 1) ro mounted > subvolume will force all the children subvolumes only ro mountable > > subvol 5 (mounted ro /) ??? subvol 257 (mounted rw /mnt/btrfrs) So > above mounted should not be allowed. > > But the following mount should be OK: subvol 5 (mounted rw /) ??? > subvol 257 (mounted ro /mnt/btrfrs) > > 2) ro mounted subvolume will not be modified even through the rw > mounted parent subvolume. > > Only this will ensure restricted ro mount option. > > If anyone has any other ideas about it, I'm happy to listen. Usually I mount both the subvolume as root filesystem and the root of the btrfs filesystem in a subdir . This allow me to perform action like snapshot of the root filesystem. So to me it seems reasonable to have different rw/ro status between btrfs root and btrfs subvolume. As use case think a system which hosts several guests in container. Each guest has its own subvolume as root filesystem. An user would mount the btrfs root RO in order to see all the subvolume but at the same time he avoids to change a file for error; when a guest has to be started, its root filesystem/subvolume can be mounted RW. On the other side, I understand that this could lead to an unexpected behaviour because with the other filesystem it is impossible to mount only a part as RW. In this BTRFS would be different. Following the "least surprise" principle, I prefer that the *mount* RO/RW flag acts globally: the filesystem has only one status. It is possible to change it only globally. In order to having a subvolumes with different RO/RW status we should rely on different flag. I have to point out that the subvolume has already the concept of read-only status. We could adopt the following rules: - if the filesystem is mounted RO then all the subvolumes (event the id=5) are RO - if a subvolume is marked RO, the it is RO - otherwise a subvolume is RW Moreover we can add further rules to inherit the subvolume RO/RW status at the creation time (even tough it makes sense only for the snapshot). We could use an xattr for that. Finally I would like to point out that relying on the relationship parent/child between the subvolumes is very dangerous. With the exception if the subvolid=5 which is the only root one, it is very easy to move up and down the subvolume. I have to point out this because I read on another email that someone likes the idea to having a RO subvolume because its parent is marked RO. But a subvolume may be mounted also by id and not by its path (and or name). So relying on the relationship parent/child would lead to break the "least surprise principle". My 2 ¢ BR G.Baroncelli > > Thanks, Qu >> If so this behaviour seems to me correctly. Different is after >> mounting the subvolume "subvolumename", also the whole filesystem >> results rw (eg: #1 succeeded). >> >> G.Baroncelli >> >> >> >> >> >>> The problem also happens when a parent subvol is mounted rw but >>> child subvol is mounted ro. User can still modify the child >>> subvol through parent subvol, still broke the readonly rule. >>> >>> Thanks, Qu >>> >> > >
-------- Original Message -------- Subject: Re: [RFC PATCH] Revert "btrfs: allow mounting btrfs subvolumes with different ro/rw options" From: Goffredo Baroncelli <kreijack@inwind.it> To: Qu Wenruo <quwenruo@cn.fujitsu.com>, linux-btrfs@vger.kernel.org Date: 2014?07?05? 01:41 > Hi Qu > On 07/04/2014 03:28 AM, Qu Wenruo wrote: >> -------- Original Message -------- Subject: Re: [RFC PATCH] Revert >> "btrfs: allow mounting btrfs subvolumes with different ro/rw >> options" From: Goffredo Baroncelli <kreijack@inwind.it> To: Qu Wenruo >> <quwenruo@cn.fujitsu.com>, linux-btrfs@vger.kernel.org Date: 2014?07? >> 04? 01:37 >>> On 07/03/2014 02:28 AM, Qu Wenruo wrote: >>>> -------- Original Message -------- Subject: Re: [RFC PATCH] >>>> Revert "btrfs: allow mounting btrfs subvolumes with different >>>> ro/rw options" From: Goffredo Baroncelli <kreijack@libero.it> To: >>>> Qu Wenruo <quwenruo@cn.fujitsu.com>, linux-btrfs@vger.kernel.org >>>> Date: 2014?07?03? 01:48 >>>>> On 07/01/2014 11:30 AM, Qu Wenruo wrote: >>>>>> This commit has the following problem: 1) Break the ro mount >>>>>> rule. When users mount the whole btrfs ro, it is still >>>>>> possible to mount subvol rw and change the contents. Which >>>>>> make the whole fs ro mount non-sense. >>>>> Where is the problem ? I see an use case when I want a >>>>> conservative default: mount all ro except some subvolumes. >>>>> >>>>> In any case it is not a security problem because if the user >>>>> has the capability to mount a subvolume, also he has the >>>>> capability to remount,rw the whole filesystem. >>>>> >>>>> >>>>> >>>> Not security problem but behavior not consistent. If user mount >>>> the whole disk ro, he or she want the fs read only and nothing >>>> will change in it. If you mount a subvol rw, then the whole disk >>>> ro expectation is broken. Things will change even the whole disk >>>> is readonly. >>> Sorry for bother you again, but there is a thing not clear to me: >>> >>> If >>> >>> # mount -o subvolid=5,ro /dev/sda1 /mnt/root # mount -o >>> subvol=subvolname,rw /dev/sda1 /mnt/subvolname >>> >>> I suppose that >>> >>> # touch /mnt/root/touch-test # 1 >>> >>> fails, and >>> >>> # touch /mnt/subvolname/touch-test # 2 >>> >>> succeeded. I understood correctly ? >> Your understanding is right and that is current behavior. >> >> But that should not be the correct behavior. >> >> If you mount fs_tree ro, btrfs should ensure the whole >> fs_tree(including all the subvolumes) ro. Or the whole fs_tree is not >> restricted readonly since you can modify contents inside the rw >> subvolume, and it's part of the fs_tree.(partly ro and partly rw >> status) >> >> IMO the perfect logical should be like the following: 1) ro mounted >> subvolume will force all the children subvolumes only ro mountable >> >> subvol 5 (mounted ro /) ??? subvol 257 (mounted rw /mnt/btrfrs) So >> above mounted should not be allowed. >> >> But the following mount should be OK: subvol 5 (mounted rw /) ??? >> subvol 257 (mounted ro /mnt/btrfrs) >> >> 2) ro mounted subvolume will not be modified even through the rw >> mounted parent subvolume. >> >> Only this will ensure restricted ro mount option. >> >> If anyone has any other ideas about it, I'm happy to listen. > Usually I mount both the subvolume as root filesystem and the root of the btrfs filesystem in a subdir . This allow me to perform action like snapshot of the root filesystem. > > So to me it seems reasonable to have different rw/ro status between btrfs root and btrfs subvolume. As use case think a system which hosts several guests in container. Each guest has its own subvolume as root filesystem. An user would mount the btrfs root RO in order to see all the subvolume but at the same time he avoids to change a file for error; when a guest has to be started, its root filesystem/subvolume can be mounted RW. You caught me. Yes, the use case seems quite resonable since currently you need to mount btrfs to get the subvolume list (the only offline method seems btrfs-debug-tree but end-users won't use it anyway) and it's a good admin behavior to mount it ro if no need to write. > > On the other side, I understand that this could lead to an unexpected behaviour because with the other filesystem it is impossible to mount only a part as RW. In this BTRFS would be different. > > Following the "least surprise" principle, I prefer that the *mount* RO/RW flag acts globally: the filesystem has only one status. It is possible to change it only globally. > > In order to having a subvolumes with different RO/RW status we should rely on different flag. I have to point out that the subvolume has already the concept of read-only status. > > We could adopt the following rules: > - if the filesystem is mounted RO then all the subvolumes (event the id=5) are RO > - if a subvolume is marked RO, the it is RO > - otherwise a subvolume is RW I'm confused with rule 1. When mentionting 'mounted RO', you mean mount subvolume id=5 RO? Also you mentioned that using differnt RO/RW flag independent from VFS RO/RW flags, so it also makes me confused that when mentioning RO, did you mean VFS RO or new btrfs RO/RW flags? > > Moreover we can add further rules to inherit the subvolume RO/RW status at the creation time (even tough it makes sense only for the snapshot). We could use an xattr for that. > > Finally I would like to point out that relying on the relationship parent/child between the subvolumes is very dangerous. With the exception if the subvolid=5 which is the only root one, it is very easy to move up and down the subvolume. I have to point out this because I read on another email that someone likes the idea to having a RO subvolume because its parent is marked RO. > But a subvolume may be mounted also by id and not by its path (and or name). So relying on the relationship parent/child would lead to break the "least surprise principle". > > My 2 ¢ > BR > G.Baroncelli Oh I forgot that user can mv subvolumes just like normal dirs. In this case it will certainly make ro/rw disaster if rely on the parent ro/rw status. :( Thanks, Qu > >> Thanks, Qu >>> If so this behaviour seems to me correctly. Different is after >>> mounting the subvolume "subvolumename", also the whole filesystem >>> results rw (eg: #1 succeeded). >>> >>> G.Baroncelli >>> >>> >>> >>> >>> >>>> The problem also happens when a parent subvol is mounted rw but >>>> child subvol is mounted ro. User can still modify the child >>>> subvol through parent subvol, still broke the readonly rule. >>>> >>>> Thanks, Qu >>>> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/07/2014 03:46 AM, Qu Wenruo wrote: > [... cut ...] >> >> So to me it seems reasonable to have different rw/ro status between >> btrfs root and btrfs subvolume. As use case think a system which >> hosts several guests in container. Each guest has its own subvolume >> as root filesystem. An user would mount the btrfs root RO in order >> to see all the subvolume but at the same time he avoids to change a >> file for error; when a guest has to be started, its root >> filesystem/subvolume can be mounted RW. > You caught me. Yes, the use case seems quite resonable since > currently you need to mount btrfs to get the subvolume list (the only > offline method seems btrfs-debug-tree but end-users won't use it > anyway) and it's a good admin behavior to mount it ro if no need to > write. >> >> On the other side, I understand that this could lead to an >> unexpected behaviour because with the other filesystem it is >> impossible to mount only a part as RW. In this BTRFS would be >> different. >> >> Following the "least surprise" principle, I prefer that the *mount* >> RO/RW flag acts globally: the filesystem has only one status. It is >> possible to change it only globally. >> >> In order to having a subvolumes with different RO/RW status we >> should rely on different flag. I have to point out that the >> subvolume has already the concept of read-only status. >> >> We could adopt the following rules: >> - if the filesystem is mounted RO then all the subvolumes (event >> the id=5) are RO >>- if a subvolume is marked RO, the it is RO >> - otherwise a subvolume is RW > I'm confused with rule 1. When mentionting 'mounted RO', you mean > mount subvolume id=5 RO? Also you mentioned that using differnt RO/RW > flag independent from VFS RO/RW flags, so it also makes me confused > that when mentioning RO, did you mean VFS RO or new btrfs RO/RW > flags? For "mounted RO" I mean the VFS flag, the "one" passed via the mount command. I say "one" as 1, because I am convinced that it has to act globally, e.g. on the whole filesystem; the flag should be set at the first mount, then it can be changed (only ?) issuing a "mount -o remount,rw/ro" For example, the following commands # mount -o subvol=subvolname,ro /dev/sda1 /mnt/btrfs-subvol # mount -o subvolid=5 /dev/sda1 /mnt/btrfs-root cause the following ones # touch /mnt/btrfs-subvol/touch-a-file # touch /mnt/btrfs-root/touch-a-file2 to fail; and the following commands # mount -o subvol=subvolname,ro /dev/sda1 /mnt/btrfs-subvol # mount -o subvolid=5 /dev/sda1 /mnt/btrfs-root # mount -o remount,rw /mnt/btrfs-subvol cause the following ones # touch /mnt/btrfs-subvol/touch-a-file # touch /mnt/btrfs-root/touch-a-file2 to succeed So for each filesystem, there is a "globally" flag ro/rw which acts on the whole filesystem. Clear and simple. Step 2: a more fine grained control of the subvolumes. We have already the capability to make a subvolume read-only/read-write doing # btrfs property set -t s /path/to/subvolume ro true or # btrfs property set -t s /path/to/subvolume ro false My idea is to use this flag. It could be done at the mount time for example: # mount -o subvolmode=ro,subvol=subvolname /dev/sda1 / (this example doesn't work, it is only a my idea) So: - we should not add further code - the semantic is simple - the property is linked to the subvolume in a understandable way We should only add the subvolmode=ro option to the mount command. Further discussion need to investigate the following cases: - if the filesystem is mounted as ro (mount -o ro....), does mounting a subvolume rw ( mount -o subvolmode=rw...) should raise an error ? (IMHO yes) - if the filesystem is mounted as ro (mount -o ro....), does mounting the filesystem a 2nd time rw ( mount -o rw...) should raise an error ? (IMHO yes) - if a subvolume is mounter rw (or ro), does mounting the same subvolume a 2nd time as ro (or rw) should raise an error ? (IMHO yes) BR G.Baroncelli >> >> Moreover we can add further rules to inherit the subvolume RO/RW >> status at the creation time (even tough it makes sense only for the >> snapshot). We could use an xattr for that. >> >> Finally I would like to point out that relying on the relationship >> parent/child between the subvolumes is very dangerous. With the >> exception if the subvolid=5 which is the only root one, it is very >> easy to move up and down the subvolume. I have to point out this >> because I read on another email that someone likes the idea to >> having a RO subvolume because its parent is marked RO. But a >> subvolume may be mounted also by id and not by its path (and or >> name). So relying on the relationship parent/child would lead to >> break the "least surprise principle". >> >> My 2 ¢ BR G.Baroncelli > Oh I forgot that user can mv subvolumes just like normal dirs. In > this case it will certainly make ro/rw disaster if rely on the parent > ro/rw status. :( > > Thanks, Qu >> >>> Thanks, Qu >>>> If so this behaviour seems to me correctly. Different is after >>>> mounting the subvolume "subvolumename", also the whole >>>> filesystem results rw (eg: #1 succeeded). >>>> >>>> G.Baroncelli >>>> >>>> >>>> >>>> >>>> >>>>> The problem also happens when a parent subvol is mounted rw >>>>> but child subvol is mounted ro. User can still modify the >>>>> child subvol through parent subvol, still broke the readonly >>>>> rule. >>>>> >>>>> Thanks, Qu >>>>> >>> >> > >
Goffredo Baroncelli posted on Mon, 07 Jul 2014 19:37:53 +0200 as excerpted: > For "mounted RO" I mean the VFS flag, the "one" passed via the mount > command. I say "one" as 1, because I am convinced that it has to act > globally, > e.g. on the whole filesystem; the flag should be set at the first mount, > then it can be changed (only ?) issuing a "mount -o remount,rw/ro" [...] > So for each filesystem, there is a "globally" flag ro/rw which acts on > the whole filesystem. Clear and simple. > > Step 2: a more fine grained control of the subvolumes. > We have already the capability to make a subvolume read-only/read-write > doing > > # btrfs property set -t s /path/to/subvolume ro true > > or > > # btrfs property set -t s /path/to/subvolume ro false > > My idea is to use this flag. It could be done at the mount time for > example: > > # mount -o subvolmode=ro,subvol=subvolname /dev/sda1 / > > (this example doesn't work, it is only a my idea) > > So: > - we should not add further code > - the semantic is simple > - the property is linked to the subvolume in a understandable way > > We should only add the subvolmode=ro option to the mount command. > > Further discussion need to investigate the following cases: > - if the filesystem is mounted as ro (mount -o ro....), does mounting a > subvolume rw ( mount -o subvolmode=rw...) should raise an error ? > (IMHO yes) > - if the filesystem is mounted as ro (mount -o ro....), does mounting > the filesystem a 2nd time rw ( mount -o rw...) should raise an error ? > (IMHO yes) > - if a subvolume is mounter rw (or ro), does mounting the same subvolume > a 2nd time as ro (or rw) should raise an error ? > (IMHO yes) Makes sense. Assuming I'm following you correctly, then, no subvolumes could be rw if the filesystem/vfs flag was set ro. Which would mean that in ordered to mount any particular subvolume rw, the whole filesystem would have to be rw. Extending now: For simplicity and backward compatibility, if subvolmode isn't set, it corresponds to the whole-fs/vfs mode. That way, setting mount -o ro,... (or -o rw,...) with the first mount would naturally propagate to all subsequent subvolume mounts, unless of course (1) all subvolumes and the filesystem itself are umounted, after which a new mount would be the first one again, or (2) a mount -o remount,... is done that changes the whole-fs mode. Further, if it happened that one wanted the first subvolume mounted to be ro, but the filesystem as a whole rw so that other subvolumes could be mounted rw, the following would accomplish that: mount -o rw,subvolmode=ro That way, the subvol would be ro as desired, but the filesystem as a whole would be rw, so other subvolumes could be successfully mounted rw. I like the concept. =:^) The remaining problem to deal with is that if say the root subvol (id=5) is mounted rw,subvolmode=rw, while a subvolume below it is mounted subvolmode=ro, then what happens if someone tries to make an edit in the portion of the filesystem visible in the subvolume, but from the parent, id=5/root in this case? Obviously if that modification is allowed from the parent, it'll change what's visible in the child subvolume as well, which would be rather unexpected. I'd suggest that the snapshotting border rule should apply to writes as well. Snapshots stop at subvolume borders, and writes should as well. Attempting to write in a child subvolume should error out -- child subvolumes are not part of a parent snapshot and shouldn't be writable from the parent subvolume, either. Child-subvolume content should be read-only because it's beyond the subvolume border. That would seem to be the safest. Altho I believe it's a change from current behavior, where it's possible to write into any subvolume visible from the parent (that is, not covered by an over-mount, perhaps even of the same subvolume that would otherwise be visible in the same location from the parent subvolume), provided the parent is writable. Regardless, my biggest take-away from the discussion so far is that I'm glad I decided to go with entirely separate filesystems, each on their own partitions, so my ro vs writable mounts do exactly what I expect them to do without me having to worry or think about it too much! That wasn't the reason I did it -- I did it because I didn't want all my data eggs in the same whole-filesystem basket such that if a filesystem was damaged, the damage was compartmentalized -- but now that see all the subvolume rw/ ro implications discussed in this thread, I'm VERY glad I personally don't have to worry about it, and it all simply "just works" for me, because each filesystem is independent of the others, not simply a subvolume!
On 07/08/2014 04:43 AM, Duncan wrote: > The remaining problem to deal with is that if say the root subvol (id=5) > is mounted rw,subvolmode=rw, while a subvolume below it is mounted > subvolmode=ro, then what happens if someone tries to make an edit in the > portion of the filesystem visible in the subvolume, but from the parent, > id=5/root in this case? Obviously if that modification is allowed from > the parent, it'll change what's visible in the child subvolume as well, > which would be rather unexpected. The ro/rw status is a subvolume flag. So if a subvolume is marked rw (or ro), is writable (not writable) in all the mount(S) This flag is not inheritable. What could be strange is the following: # mount -o subvolid=5,rw /dev/sda1 /mnt/btrfs-root # btrfs subvol create /mnt/btrfs-root/subvolname/ then # touch /mnt/btrfs-root/subvolname/touch-file succeeds; but # mount -o subvolid=5,rw /dev/sda1 /mnt/btrfs-root # btrfs subvol create /mnt/btrfs-root/subvolname/ # mount -o subvol=subvolname,ro /dev/sda1 /mnt/btrfs-subvol then # touch /mnt/btrfs-root/subvolname/touch-file2 fails.
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 4662d92..4a088f8 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -66,8 +66,6 @@ static const struct super_operations btrfs_super_ops; static struct file_system_type btrfs_fs_type; -static int btrfs_remount(struct super_block *sb, int *flags, char *data); - static const char *btrfs_decode_error(int errno) { char *errstr = "unknown"; @@ -1179,31 +1177,7 @@ static struct dentry *mount_subvol(const char *subvol_name, int flags, return ERR_PTR(-ENOMEM); mnt = vfs_kern_mount(&btrfs_fs_type, flags, device_name, newargs); - - if (PTR_RET(mnt) == -EBUSY) { - if (flags & MS_RDONLY) { - mnt = vfs_kern_mount(&btrfs_fs_type, flags & ~MS_RDONLY, device_name, - newargs); - } else { - int r; - mnt = vfs_kern_mount(&btrfs_fs_type, flags | MS_RDONLY, device_name, - newargs); - if (IS_ERR(mnt)) { - kfree(newargs); - return ERR_CAST(mnt); - } - - r = btrfs_remount(mnt->mnt_sb, &flags, NULL); - if (r < 0) { - /* FIXME: release vfsmount mnt ??*/ - kfree(newargs); - return ERR_PTR(r); - } - } - } - kfree(newargs); - if (IS_ERR(mnt)) return ERR_CAST(mnt);
This reverts commit 0723a0473fb48a1c93b113a28665b64ce5faf35a. This commit has the following problem: 1) Break the ro mount rule. When users mount the whole btrfs ro, it is still possible to mount subvol rw and change the contents. Which make the whole fs ro mount non-sense. 2) Cause whole btrfs ro/rw mount change fails. When mount a subvol ro first, when you can't mount the whole fs mounted rw. This is due to the check in btrfs_mount() which returns -EBUSY, which is OK for single fs to prevent mount fs ro in one mount point and mount the same fs rw in other mount point. Step to reproduce: mount -o subvol=subv,ro /dev/sda6 /mnt/btrfs mount -o rw /dev/sda6 /mnt/btrfs <-this will fail 3) Kernel warn in vfs. When mount the whole fs ro, and mount a subvol ro, kernel warning will show in fs/sync.c complaining s_umount rwsem is not locked. Since this remount is not called by VFS, so s_mounts rwsem is not correctly locked. 4) Lacks ro/rw conflicint check. The commit uses a trick to 'cheat' vfs about ro/rw flags to allow different ro/rw mount options, however this breaks the original ro/rw conflicting rule: one fs(even subvolume) can't be mounted rw in one place and mounted ro in someplace else. This can be triggered quite easily: mount -o subvol=subv,ro /dev/sda6 /mnt/btrfs mount -o subvol=subv,rw /dev/sda6 /mnt/other Also the check logical about checking the ro/rw needed to be investigated further. e.g. When the subvolume is mounted ro, the whole btrfs fs is mounted rw, so one can change the ro mounted subvolume from the mounted whole btrfs. Due to the above reasons, the per-subvolume ro/rw mount options should be investigated further to ensure the correctness and better reverting it before correct implement (It may takes a lone time to find the right logic about ro/rw option in subvolume) Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> Conflicts: fs/btrfs/super.c --- fs/btrfs/super.c | 26 -------------------------- 1 file changed, 26 deletions(-)