Re: [PATCH] btrfs: do not allow mounting non-subvolumes via subvol option
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] btrfs: do not allow mounting non-subvolumes via subvol option
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-btrfsm=130208585106572w=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-ow...@vger.kernel.org [mailto:linux-btrfs-
ow...@vger.kernel.org] On Behalf Of David Sterba
Sent: Friday, July 29, 2011 6:14 PM
To: linux-btrfs@vger.kernel.org
Cc: chris.ma...@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 dste...@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 majord...@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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] btrfs: do not allow mounting non-subvolumes via subvol option
On Wed, Aug 3, 2011 at 1:47 PM, David Sterba d...@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-btrfsm=130208585106572w=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-btrfsm=129091436915724w=2 ...and a patch proposed: http://marc.info/?l=linux-btrfsm=129091815217860w=2 C Anthony -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] btrfs: do not allow mounting non-subvolumes via subvol option
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 Sterbadste...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
