| Message ID | 1311934448-887-1-git-send-email-dsterba@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
I believe I have submit a similar patch months ago: http://marc.info/?l=linux-btrfs&m=130208585106572&w=2 Hope it can be integrated this time, :-). > -----Original Message----- > From: linux-btrfs-owner@vger.kernel.org [mailto:linux-btrfs- > owner@vger.kernel.org] On Behalf Of David Sterba > Sent: Friday, July 29, 2011 6:14 PM > To: linux-btrfs@vger.kernel.org > Cc: chris.mason@oracle.com; David Sterba > Subject: [PATCH] btrfs: do not allow mounting non-subvolumes via subvol > option > > There's a missing test whether the path passed to subvol=path option > during mount is a real subvolume, allowing any directory located in > default subovlume to be passed and accepted for mount. > > (current btrfs progs prevent this early) > $ btrfs subvol snapshot . p1-snap > ERROR: '.' is not a subvolume > > (with "is subvolume?" test bypassed) > $ btrfs subvol snapshot . p1-snap > Create a snapshot of '.' in './p1-snap' > > $ btrfs subvol list -p . > ID 258 parent 5 top level 5 path subvol > ID 259 parent 5 top level 5 path subvol1 > ID 260 parent 5 top level 5 path default-subvol1 > ID 262 parent 5 top level 5 path p1/p1-snapshot > ID 263 parent 259 top level 5 path subvol1/subvol1-snap > > The problem I see is that this makes a false impression of snapshotting > the > given subvolume but in fact snapshots the default one: a user expects > outcome > like ID 263 but in fact gets ID 262 . > > This patch makes mount fail with EINVAL with a message in syslog. > > Signed-off-by: David Sterba <dsterba@suse.cz> > --- > > I did not find a better errno than EINVAL, probably adding someting > like > ENSUBVOL would be better so that other filesystems with such > functionality may > use it in future. > > fs/btrfs/super.c | 19 +++++++++++++++++++ > 1 files changed, 19 insertions(+), 0 deletions(-) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index 15634d4..0c2a1d1 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -753,6 +753,15 @@ static int btrfs_set_super(struct super_block *s, > void *data) > return set_anon_super(s, data); > } > > +/* > + * subvolumes are identified by ino 256 > + */ > +static inline int is_subvolume_inode(struct inode *inode) > +{ > + if (inode && inode->i_ino == BTRFS_FIRST_FREE_OBJECTID) > + return 1; > + return 0; > +} > > /* > * Find a superblock for the given device / mount point. > @@ -873,6 +882,16 @@ static struct dentry *btrfs_mount(struct > file_system_type *fs_type, int flags, > error = -ENXIO; > goto error_free_subvol_name; > } > + > + if (!is_subvolume_inode(new_root->d_inode)) { > + dput(root); > + dput(new_root); > + deactivate_locked_super(s); > + error = -EINVAL; > + printk(KERN_ERR "btrfs: '%s' is not a valid > subvolume\n", > + subvol_name); > + goto error_free_subvol_name; > + } > dput(root); > root = new_root; > } else { > -- > 1.7.6 > > -- > 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 -- 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 David, On 07/29/2011 12:14 PM, David Sterba wrote: > There's a missing test whether the path passed to subvol=path option > during mount is a real subvolume, allowing any directory located in > default subovlume to be passed and accepted for mount. > > (current btrfs progs prevent this early) > $ btrfs subvol snapshot . p1-snap > ERROR: '.' is not a subvolume > > (with "is subvolume?" test bypassed) > $ btrfs subvol snapshot . p1-snap > Create a snapshot of '.' in './p1-snap' > > $ btrfs subvol list -p . > ID 258 parent 5 top level 5 path subvol > ID 259 parent 5 top level 5 path subvol1 > ID 260 parent 5 top level 5 path default-subvol1 > ID 262 parent 5 top level 5 path p1/p1-snapshot > ID 263 parent 259 top level 5 path subvol1/subvol1-snap > > The problem I see is that this makes a false impression of snapshotting the > given subvolume but in fact snapshots the default one: a user expects outcome Not that matter too much, but the old behavior was to snapshot not the "default one" but the one which contains the directory. This behavior leaded to a lot of misunderstanding about the btrfs capability of snapshot subvolume __only__. Only one question, what happens now if an user pass subvol=<dir> ? > like ID 263 but in fact gets ID 262 . > > This patch makes mount fail with EINVAL with a message in syslog. > > Signed-off-by: David Sterba<dsterba@suse.cz> > --- > > I did not find a better errno than EINVAL, probably adding someting like > ENSUBVOL would be better so that other filesystems with such functionality may > use it in future. > > fs/btrfs/super.c | 19 +++++++++++++++++++ > 1 files changed, 19 insertions(+), 0 deletions(-) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index 15634d4..0c2a1d1 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -753,6 +753,15 @@ static int btrfs_set_super(struct super_block *s, void *data) > return set_anon_super(s, data); > } > > +/* > + * subvolumes are identified by ino 256 > + */ > +static inline int is_subvolume_inode(struct inode *inode) > +{ > + if (inode&& inode->i_ino == BTRFS_FIRST_FREE_OBJECTID) > + return 1; > + return 0; > +} > > /* > * Find a superblock for the given device / mount point. > @@ -873,6 +882,16 @@ static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags, > error = -ENXIO; > goto error_free_subvol_name; > } > + > + if (!is_subvolume_inode(new_root->d_inode)) { > + dput(root); > + dput(new_root); > + deactivate_locked_super(s); > + error = -EINVAL; > + printk(KERN_ERR "btrfs: '%s' is not a valid subvolume\n", > + subvol_name); > + goto error_free_subvol_name; > + } > dput(root); > root = new_root; > } else { -- 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 Fri, Jul 29, 2011 at 07:11:28PM +0200, Goffredo Baroncelli wrote: > >$ btrfs subvol list -p . > >ID 258 parent 5 top level 5 path subvol > >ID 259 parent 5 top level 5 path subvol1 > >ID 260 parent 5 top level 5 path default-subvol1 > >ID 262 parent 5 top level 5 path p1/p1-snapshot > >ID 263 parent 259 top level 5 path subvol1/subvol1-snap > > > >The problem I see is that this makes a false impression of snapshotting the > >given subvolume but in fact snapshots the default one: a user expects outcome > > Not that matter too much, but the old behavior was to snapshot not > the "default one" but the one which contains the directory. > This behavior leaded to a lot of misunderstanding about the btrfs > capability of snapshot subvolume __only__. > > Only one question, what happens now if an user pass subvol=<dir> ? $ mount /dev/sda5 /mnt/sda5 $ cd sda5 $ mkdir p $ cd .. $ umount sda5 $ mount -o subvol=p /dev/sda5 /mnt/sda5 mount: wrong fs type, bad option, bad superblock on /dev/sda5, missing codepage or helper program, or other error In some cases useful info is found in syslog - try dmesg | tail or so and dmesg says: [ 7285.905195] device fsid 46e97521-a1c7-4509-954f-b32c90bd1d1e devid 1 transid 10 /dev/sdb5 [ 7285.954435] btrfs: disk space caching is enabled [ 7286.600155] btrfs: 'p' is not a valid subvolume There could be a specific error code like ENSUBVOL and mount could be taught to give better description of what has happened. Otherwise, I took the approach of being verbose in dmesg. HTH, david -- 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 Sat, Jul 30, 2011 at 12:16:44AM +0800, Zhong, Xin wrote: > I believe I have submit a similar patch months ago: > http://marc.info/?l=linux-btrfs&m=130208585106572&w=2 You did! I was not aware of that. I believe adding a helper make things more clear (if it were used all over the code). > Hope it can be integrated this time, :-). mehopes too, david > > > > -----Original Message----- > > From: linux-btrfs-owner@vger.kernel.org [mailto:linux-btrfs- > > owner@vger.kernel.org] On Behalf Of David Sterba > > Sent: Friday, July 29, 2011 6:14 PM > > To: linux-btrfs@vger.kernel.org > > Cc: chris.mason@oracle.com; David Sterba > > Subject: [PATCH] btrfs: do not allow mounting non-subvolumes via subvol > > option > > > > There's a missing test whether the path passed to subvol=path option > > during mount is a real subvolume, allowing any directory located in > > default subovlume to be passed and accepted for mount. > > > > (current btrfs progs prevent this early) > > $ btrfs subvol snapshot . p1-snap > > ERROR: '.' is not a subvolume > > > > (with "is subvolume?" test bypassed) > > $ btrfs subvol snapshot . p1-snap > > Create a snapshot of '.' in './p1-snap' > > > > $ btrfs subvol list -p . > > ID 258 parent 5 top level 5 path subvol > > ID 259 parent 5 top level 5 path subvol1 > > ID 260 parent 5 top level 5 path default-subvol1 > > ID 262 parent 5 top level 5 path p1/p1-snapshot > > ID 263 parent 259 top level 5 path subvol1/subvol1-snap > > > > The problem I see is that this makes a false impression of snapshotting > > the > > given subvolume but in fact snapshots the default one: a user expects > > outcome > > like ID 263 but in fact gets ID 262 . > > > > This patch makes mount fail with EINVAL with a message in syslog. > > > > Signed-off-by: David Sterba <dsterba@suse.cz> > > --- > > > > I did not find a better errno than EINVAL, probably adding someting > > like > > ENSUBVOL would be better so that other filesystems with such > > functionality may > > use it in future. > > > > fs/btrfs/super.c | 19 +++++++++++++++++++ > > 1 files changed, 19 insertions(+), 0 deletions(-) > > > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > > index 15634d4..0c2a1d1 100644 > > --- a/fs/btrfs/super.c > > +++ b/fs/btrfs/super.c > > @@ -753,6 +753,15 @@ static int btrfs_set_super(struct super_block *s, > > void *data) > > return set_anon_super(s, data); > > } > > > > +/* > > + * subvolumes are identified by ino 256 > > + */ > > +static inline int is_subvolume_inode(struct inode *inode) > > +{ > > + if (inode && inode->i_ino == BTRFS_FIRST_FREE_OBJECTID) > > + return 1; > > + return 0; > > +} > > > > /* > > * Find a superblock for the given device / mount point. > > @@ -873,6 +882,16 @@ static struct dentry *btrfs_mount(struct > > file_system_type *fs_type, int flags, > > error = -ENXIO; > > goto error_free_subvol_name; > > } > > + > > + if (!is_subvolume_inode(new_root->d_inode)) { > > + dput(root); > > + dput(new_root); > > + deactivate_locked_super(s); > > + error = -EINVAL; > > + printk(KERN_ERR "btrfs: '%s' is not a valid > > subvolume\n", > > + subvol_name); > > + goto error_free_subvol_name; > > + } > > dput(root); > > root = new_root; > > } else { > > -- > > 1.7.6 > > > > -- > > 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 > -- > 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 -- 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 Wed, Aug 3, 2011 at 1:47 PM, David Sterba <dave@jikos.cz> wrote: > On Sat, Jul 30, 2011 at 12:16:44AM +0800, Zhong, Xin wrote: >> I believe I have submit a similar patch months ago: >> http://marc.info/?l=linux-btrfs&m=130208585106572&w=2 > > You did! I was not aware of that. I believe adding a helper make things > more clear (if it were used all over the code). > >> Hope it can be integrated this time, :-). > > mehopes too i corrupted an FS after doing this back in Nov of last year (though i was also --bind mounting it after the fact) http://marc.info/?l=linux-btrfs&m=129091436915724&w=2 ...and a patch proposed: http://marc.info/?l=linux-btrfs&m=129091815217860&w=2 C Anthony -- 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
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 15634d4..0c2a1d1 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -753,6 +753,15 @@ static int btrfs_set_super(struct super_block *s, void *data) return set_anon_super(s, data); } +/* + * subvolumes are identified by ino 256 + */ +static inline int is_subvolume_inode(struct inode *inode) +{ + if (inode && inode->i_ino == BTRFS_FIRST_FREE_OBJECTID) + return 1; + return 0; +} /* * Find a superblock for the given device / mount point. @@ -873,6 +882,16 @@ static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags, error = -ENXIO; goto error_free_subvol_name; } + + if (!is_subvolume_inode(new_root->d_inode)) { + dput(root); + dput(new_root); + deactivate_locked_super(s); + error = -EINVAL; + printk(KERN_ERR "btrfs: '%s' is not a valid subvolume\n", + subvol_name); + goto error_free_subvol_name; + } dput(root); root = new_root; } else {
There's a missing test whether the path passed to subvol=path option during mount is a real subvolume, allowing any directory located in default subovlume to be passed and accepted for mount. (current btrfs progs prevent this early) $ btrfs subvol snapshot . p1-snap ERROR: '.' is not a subvolume (with "is subvolume?" test bypassed) $ btrfs subvol snapshot . p1-snap Create a snapshot of '.' in './p1-snap' $ btrfs subvol list -p . ID 258 parent 5 top level 5 path subvol ID 259 parent 5 top level 5 path subvol1 ID 260 parent 5 top level 5 path default-subvol1 ID 262 parent 5 top level 5 path p1/p1-snapshot ID 263 parent 259 top level 5 path subvol1/subvol1-snap The problem I see is that this makes a false impression of snapshotting the given subvolume but in fact snapshots the default one: a user expects outcome like ID 263 but in fact gets ID 262 . This patch makes mount fail with EINVAL with a message in syslog. Signed-off-by: David Sterba <dsterba@suse.cz> --- I did not find a better errno than EINVAL, probably adding someting like ENSUBVOL would be better so that other filesystems with such functionality may use it in future. fs/btrfs/super.c | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-)