[RFC PATCH V5 0/2] Btrfs: get/set label of mounted file system

[RFC PATCH V5 0/2] Btrfs: get/set label of mounted file system

[Jeff Liu]

Hello,

This patch set is trying to make Btrfs support get/set label for a mounted file sytem via ioctl(2).
There are a couple of changes according to Miao's comments which were shown as following.

Changes of V5->V4 in kernel:
- Revise the ioctl number of BTRFS_IOC_GET_FSLABEL/BTRFS_IOC_SET_FSLABEL to 49/50 separately.
- Replace btrfs_root_readonly() check up with mnt_want_write_file().
- Validate the input label length, return -EINVAL if the specified length is exceeding
  "BTRFS_LABEL_SIZE -1".

Changes of V5->V4 in user space:
- Revise the ioctl number accordingly.
- Don't proceed to get/set the label upon a mounted file system if the dev patch is specified, i.e.
btrfs filesystem label /dev/sdaX [new label]
  Instead, alert the user to execute this command against the mount point, i.e.
btrfs filesystem label /btrfs_mount_path [new label]
- Validate the input label length for changing the label upon an unmounted file system as well.
  We does not check it up currently, the command just keeping silent and truncate the label
  characters which are beyond "BTRFS_LABEL_SIZE - 1".


Tests:
======
/dev/sda6 on /btrfs type btrfs (rw)

jeff@koala:~/oss/btrfs-progs$ sudo ./btrfs filesystem label /btrfs 
btrfs_label

Failure if the length is exceeding 255.
jeff@koala:~/oss/btrfs-progs$ sudo ./btrfs filesystem label /btrfs `perl -e 'print "A"x256'`
ERROR: unable to set label Invalid argument

Otherwise, that's ok.
jeff@koala:~/oss/btrfs-progs$ sudo ./btrfs filesystem label /btrfs `perl -e 'print "A"x255'`

jeff@koala:~/oss/btrfs-progs$ sudo ./btrfs filesystem label /btrfs
AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA


The old versions can be found at:
v4:
http://permalink.gmane.org/gmane.comp.file-systems.btrfs/21618

v3:
https://patchwork.kernel.org/patch/1124642/

v2:
http://permalink.gmane.org/gmane.comp.file-systems.btrfs/12877

v1:
http://permalink.gmane.org/gmane.comp.file-systems.btrfs/12872

Thanks,
-Jeff

[RFC PATCH V5 1/2] Btrfs: Add a new ioctl to get the label of a mounted filesystem

[Jeff Liu]

Introduce a new ioctl BTRFS_IOC_GET_FSLABEL to fetch the label of a mounted file system.

Signed-off-by: Jie Liu <jeff.liu@oracle.com>
Signed-off-by: Anand Jain <anand.jain@oracle.com>
Cc: Miao Xie <miaox@cn.fujitsu.com>

---
 fs/btrfs/ioctl.c |   15 +++++++++++++++
 fs/btrfs/ioctl.h |    2 ++
 2 files changed, 17 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 8fcf9a5..6a2488a 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3699,6 +3699,19 @@ out:
 	return ret;
 }
 
+static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg)
+{
+	struct btrfs_root *root = BTRFS_I(fdentry(file)->d_inode)->root;
+	const char *label = root->fs_info->super_copy->label;
+	int ret;
+
+	mutex_lock(&root->fs_info->volume_mutex);
+	ret = copy_to_user(arg, label, strlen(label));
+	mutex_unlock(&root->fs_info->volume_mutex);
+
+	return ret ? -EFAULT : 0;
+}
+
 long btrfs_ioctl(struct file *file, unsigned int
 		cmd, unsigned long arg)
 {
@@ -3797,6 +3810,8 @@ long btrfs_ioctl(struct file *file, unsigned int
 		return btrfs_ioctl_qgroup_create(root, argp);
 	case BTRFS_IOC_QGROUP_LIMIT:
 		return btrfs_ioctl_qgroup_limit(root, argp);
+	case BTRFS_IOC_GET_FSLABEL:
+		return btrfs_ioctl_get_fslabel(file, argp);
 	}
 
 	return -ENOTTY;
diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
index 731e287..5b2cbef 100644
--- a/fs/btrfs/ioctl.h
+++ b/fs/btrfs/ioctl.h
@@ -451,6 +451,8 @@ struct btrfs_ioctl_send_args {
 			       struct btrfs_ioctl_qgroup_create_args)
 #define BTRFS_IOC_QGROUP_LIMIT _IOR(BTRFS_IOCTL_MAGIC, 43, \
 			       struct btrfs_ioctl_qgroup_limit_args)
+#define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \
+				   char[BTRFS_LABEL_SIZE])
 #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
 				      struct btrfs_ioctl_get_dev_stats)
 #endif
-- 
1.7.9.5

[RFC PATCH V5 2/2] Btrfs: Add a new ioctl to change the label of a mounted file system

[Jeff Liu]

Introduce a new ioctl BTRFS_IOC_SET_FSLABEL to change the label of a mounted file system.

Signed-off-by: Jie Liu <jeff.liu@oracle.com>
Signed-off-by: Anand Jain <anand.jain@oracle.com>
Cc: Miao Xie <miaox@cn.fujitsu.com>

---
 fs/btrfs/ioctl.c |   40 ++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/ioctl.h |    2 ++
 2 files changed, 42 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 6a2488a..0186651 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3712,6 +3712,44 @@ static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg)
 	return ret ? -EFAULT : 0;
 }
 
+static int btrfs_ioctl_set_fslabel(struct file *file, void __user *arg)
+{
+	struct btrfs_root *root = BTRFS_I(fdentry(file)->d_inode)->root;
+	struct btrfs_super_block *super_block = root->fs_info->super_copy;
+	char label[BTRFS_LABEL_SIZE];
+	int ret;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (copy_from_user(label, arg, sizeof(label)))
+		return -EFAULT;
+
+	if (strlen(label) > BTRFS_LABEL_SIZE - 1)
+		return -EINVAL;
+
+	ret = mnt_want_write_file(file);
+	if (ret)
+		return ret;
+
+	mutex_lock(&root->fs_info->volume_mutex);
+	trans = btrfs_start_transaction(root, 1);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		goto out_unlock;
+	}
+
+	label[BTRFS_LABEL_SIZE - 1] = '\0';
+	strcpy(super_block->label, label);
+	btrfs_end_transaction(trans, root);
+
+out_unlock:
+	mutex_unlock(&root->fs_info->volume_mutex);
+	mnt_drop_write_file(file);
+	return ret;
+}
+
 long btrfs_ioctl(struct file *file, unsigned int
 		cmd, unsigned long arg)
 {
@@ -3812,6 +3850,8 @@ long btrfs_ioctl(struct file *file, unsigned int
 		return btrfs_ioctl_qgroup_limit(root, argp);
 	case BTRFS_IOC_GET_FSLABEL:
 		return btrfs_ioctl_get_fslabel(file, argp);
+	case BTRFS_IOC_SET_FSLABEL:
+		return btrfs_ioctl_set_fslabel(file, argp);
 	}
 
 	return -ENOTTY;
diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
index 5b2cbef..2abe239 100644
--- a/fs/btrfs/ioctl.h
+++ b/fs/btrfs/ioctl.h
@@ -453,6 +453,8 @@ struct btrfs_ioctl_send_args {
 			       struct btrfs_ioctl_qgroup_limit_args)
 #define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \
 				   char[BTRFS_LABEL_SIZE])
+#define BTRFS_IOC_SET_FSLABEL _IOW(BTRFS_IOCTL_MAGIC, 50, \
+				   char[BTRFS_LABEL_SIZE])
 #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
 				      struct btrfs_ioctl_get_dev_stats)
 #endif
-- 
1.7.9.5

[PATCH v5 1/4] Btrfs-progs: get label of a mounted file system

[Jeff Liu]

With this new ioctl(2), we can get the label for a mounted file system.
It still does normal process to fetch label if the specified file system is unmounted.

Signed-off-by: Jie Liu <jeff.liu@oracle.com>
Signed-off-by: Anand Jain <anand.jain@oracle.com>

---
 btrfslabel.c |   74 ++++++++++++++++++++++++++++++++++++++--------------------
 ioctl.h      |    2 ++
 utils.c      |    2 +-
 utils.h      |    1 +
 4 files changed, 53 insertions(+), 26 deletions(-)

diff --git a/btrfslabel.c b/btrfslabel.c
index cb142b0..443eff6 100644
--- a/btrfslabel.c
+++ b/btrfslabel.c
@@ -67,45 +67,69 @@ static void change_label_unmounted(char *dev, char *nLabel)
        close_ctree(root);
 }
 
-int get_label_unmounted(char *dev)
+static int get_label_unmounted(const char *dev)
 {
-       struct btrfs_root *root;
+	struct btrfs_root *root;
+	int ret;
 
-       /* Open the super_block at the default location
-        * and as read-only.
-        */
-       root = open_ctree(dev, 0, 0);
+	ret = check_mounted(dev);
+	if (ret < 0) {
+	       fprintf(stderr, "FATAL: error checking %s mount status\n", dev);
+	       return -1;
+	}
+	if (ret > 0) {
+		fprintf(stderr, "ERROR: dev %s is mounted, use mount point\n",
+			dev);
+		return -1;
+	}
 
-       if(!root)
-         return -1;
+	/* Open the super_block at the default location
+	 * and as read-only.
+	 */
+	root = open_ctree(dev, 0, 0);
+	if(!root)
+		return -1;
 
-       fprintf(stdout, "%s\n", root->fs_info->super_copy.label);
+	fprintf(stdout, "%s\n", root->fs_info->super_copy.label);
 
-       /* Now we close it since we are done. */
-       close_ctree(root);
-       return 0;
+	/* Now we close it since we are done. */
+	close_ctree(root);
+	return 0;
 }
 
-int get_label(char *btrfs_dev)
+/*
+ * If a partition is mounted, try to get the filesystem label via its
+ * mounted path rather than device.  Return the corresponding error
+ * the user specified the device path.
+ */
+static int get_label_mounted(const char *mount_path)
 {
+	char label[BTRFS_LABEL_SIZE];
+	int fd;
 
-	int ret;
-	ret = check_mounted(btrfs_dev);
-	if (ret < 0)
-	{
-	       fprintf(stderr, "FATAL: error checking %s mount status\n", btrfs_dev);
-	       return -1;
+	fd = open(mount_path, O_RDONLY | O_NOATIME);
+	if (fd < 0) {
+		fprintf(stderr, "ERROR: unable access to '%s'\n", mount_path);
+		return -1;
 	}
 
-	if(ret != 0)
-	{
-	       fprintf(stderr, "FATAL: the filesystem has to be unmounted\n");
-	       return -2;
+	memset(label, '\0', sizeof(label));
+	if (ioctl(fd, BTRFS_IOC_GET_FSLABEL, label) < 0) {
+		fprintf(stderr, "ERROR: unable get label %s\n", strerror(errno));
+		close(fd);
+		return -1;
 	}
-	ret = get_label_unmounted(btrfs_dev);
-	return ret;
+
+	fprintf(stdout, "%s\n", label);
+	return 0;
 }
 
+int get_label(const char *btrfs_dev)
+{
+	return is_existing_blk_or_reg_file(btrfs_dev) ?
+		get_label_unmounted(btrfs_dev) :
+		get_label_mounted(btrfs_dev);
+}
 
 int set_label(char *btrfs_dev, char *nLabel)
 {
diff --git a/ioctl.h b/ioctl.h
index 6fda3a1..0807b05 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -432,4 +432,6 @@ struct btrfs_ioctl_clone_range_args {
 					struct btrfs_ioctl_qgroup_create_args)
 #define BTRFS_IOC_QGROUP_LIMIT _IOR(BTRFS_IOCTL_MAGIC, 43, \
 					struct btrfs_ioctl_qgroup_limit_args)
+#define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \
+				   char[BTRFS_LABEL_SIZE])
 #endif
diff --git a/utils.c b/utils.c
index 205e667..d59bca3 100644
--- a/utils.c
+++ b/utils.c
@@ -811,7 +811,7 @@ int check_mounted(const char* file)
 		return -errno;
 	}
 
-	ret =  check_mounted_where(fd, file, NULL, 0, NULL);
+	ret = check_mounted_where(fd, file, NULL, 0, NULL);
 	close(fd);
 
 	return ret;
diff --git a/utils.h b/utils.h
index 3a0368b..8750f28 100644
--- a/utils.h
+++ b/utils.h
@@ -46,4 +46,5 @@ int check_label(char *input);
 int get_mountpt(char *dev, char *mntpt, size_t size);
 
 int btrfs_scan_block_devices(int run_ioctl);
+int is_existing_blk_or_reg_file(const char* filename);
 #endif
-- 
1.7.9.5

[PATCH V5 2/4] Btrfs-progs: Change the label of a mounted file system

[Jeff Liu]

With this new ioctl(2), we can set/change the label for a mounted file system.
It still does normal process for an umounted file system.

Signed-off-by: Jie Liu <jeff.liu@oracle.com>
Signed-off-by: Anand Jain <anand.jain@oracle.com>

---
 btrfslabel.c |   90 +++++++++++++++++++++++++++++++++++-----------------------
 ioctl.h      |    2 ++
 2 files changed, 57 insertions(+), 35 deletions(-)

diff --git a/btrfslabel.c b/btrfslabel.c
index 443eff6..938e8b4 100644
--- a/btrfslabel.c
+++ b/btrfslabel.c
@@ -46,25 +46,58 @@
 #define GET_LABEL                      3
 #define SET_LABEL                      4
 
-static void change_label_unmounted(char *dev, char *nLabel)
+static int set_label_unmounted(const char *dev, const char *label)
 {
-       struct btrfs_root *root;
-       struct btrfs_trans_handle *trans;
-
-       /* Open the super_block at the default location
-        * and as read-write.
-        */
-       root = open_ctree(dev, 0, 1);
-       if (!root) /* errors are printed by open_ctree() */
-         return;
-
-       trans = btrfs_start_transaction(root, 1);
-       strncpy(root->fs_info->super_copy.label, nLabel, BTRFS_LABEL_SIZE);
-       root->fs_info->super_copy.label[BTRFS_LABEL_SIZE-1] = 0;
-       btrfs_commit_transaction(trans, root);
-
-       /* Now we close it since we are done. */
-       close_ctree(root);
+	struct btrfs_trans_handle *trans;
+	struct btrfs_root *root;
+	int ret;
+
+	ret = check_mounted(dev);
+	if (ret < 0) {
+	       fprintf(stderr, "FATAL: error checking %s mount status\n", dev);
+	       return -1;
+	}
+	if (ret > 0) {
+		fprintf(stderr, "ERROR: dev %s is mounted, use mount point\n",
+			dev);
+		return -1;
+	}
+
+	/* Open the super_block at the default location
+	 * and as read-write.
+	 */
+	root = open_ctree(dev, 0, 1);
+	if (!root) /* errors are printed by open_ctree() */
+		return -1;
+
+	trans = btrfs_start_transaction(root, 1);
+	strncpy(root->fs_info->super_copy.label, label, BTRFS_LABEL_SIZE);
+	root->fs_info->super_copy.label[BTRFS_LABEL_SIZE-1] = 0;
+	btrfs_commit_transaction(trans, root);
+
+	/* Now we close it since we are done. */
+	close_ctree(root);
+	return 0;
+}
+
+static int set_label_mounted(const char *mount_path, const char *label)
+{
+	int fd;
+
+	fd = open(mount_path, O_RDONLY | O_NOATIME);
+	if (fd < 0) {
+		fprintf(stderr, "ERROR: unable access to '%s'\n", mount_path);
+		return -1;
+	}
+
+	if (ioctl(fd, BTRFS_IOC_SET_FSLABEL, label) < 0) {
+		fprintf(stderr, "ERROR: unable to set label %s\n",
+			strerror(errno));
+		close(fd);
+		return -1;
+	}
+
+	return 0;
 }
 
 static int get_label_unmounted(const char *dev)
@@ -131,22 +164,9 @@ int get_label(const char *btrfs_dev)
 		get_label_mounted(btrfs_dev);
 }
 
-int set_label(char *btrfs_dev, char *nLabel)
+int set_label(char *btrfs_dev, char *label)
 {
-
-	int ret;
-	ret = check_mounted(btrfs_dev);
-	if (ret < 0)
-	{
-	       fprintf(stderr, "FATAL: error checking %s mount status\n", btrfs_dev);
-	       return -1;
-	}
-
-	if(ret != 0)
-	{
-	       fprintf(stderr, "FATAL: the filesystem has to be unmounted\n");
-	       return -2;
-	}
-	change_label_unmounted(btrfs_dev, nLabel);
-	return 0;
+	return is_existing_blk_or_reg_file(btrfs_dev) ?
+		set_label_unmounted(btrfs_dev, label) :
+		set_label_mounted(btrfs_dev, label);
 }
diff --git a/ioctl.h b/ioctl.h
index 0807b05..0882d6c 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -434,4 +434,6 @@ struct btrfs_ioctl_clone_range_args {
 					struct btrfs_ioctl_qgroup_limit_args)
 #define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \
 				   char[BTRFS_LABEL_SIZE])
+#define BTRFS_IOC_SET_FSLABEL _IOW(BTRFS_IOCTL_MAGIC, 50, \
+				   char[BTRFS_LABEL_SIZE])
 #endif
-- 
1.7.9.5

[PATCH V5 3/4] Btrfs-progs: Fix set_label_unmounted() with label length validation

[Jeff Liu]

Currently, we keeping silent if the label length is exceeding BTRFS_LABEL_SIZE - 1, and just
truncating the characters beyond that.

This patch make it return error and exit in this situation.

Signed-off-by: Jie Liu <jeff.liu@oracle.com>
Signed-off-by: Anand Jain <anand.jain@oracle.com>

---
 btrfslabel.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/btrfslabel.c b/btrfslabel.c
index 938e8b4..2826050 100644
--- a/btrfslabel.c
+++ b/btrfslabel.c
@@ -63,6 +63,12 @@ static int set_label_unmounted(const char *dev, const char *label)
 		return -1;
 	}
 
+	if (strlen(label) > BTRFS_LABEL_SIZE - 1) {
+		fprintf(stderr, "ERROR: Label %s is too long (max %d)\n",
+			label, BTRFS_LABEL_SIZE - 1);
+		return -1;
+	}
+
 	/* Open the super_block at the default location
 	 * and as read-write.
 	 */
@@ -71,8 +77,8 @@ static int set_label_unmounted(const char *dev, const char *label)
 		return -1;
 
 	trans = btrfs_start_transaction(root, 1);
-	strncpy(root->fs_info->super_copy.label, label, BTRFS_LABEL_SIZE);
-	root->fs_info->super_copy.label[BTRFS_LABEL_SIZE-1] = 0;
+	snprintf(root->fs_info->super_copy.label, BTRFS_LABEL_SIZE, "%s",
+		 label);
 	btrfs_commit_transaction(trans, root);
 
 	/* Now we close it since we are done. */
-- 
1.7.9.5

[PATCH v5 4/4] Btrfs-progs: fix cmd_label_usage to reflect this change.

[Jeff Liu]

Fix the command usage of "btrfs filesystem label" to reflect this change. i.e. so that
we can get/set the label of a mounted filesystem against the mountpoint.

Signed-off-by: Jie Liu <jeff.liu@oracle.com>
Signed-off-by: Anand Jain <anand.jain@oracle.com>

---
 cmds-filesystem.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 9c43d35..5770d8b 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -517,8 +517,8 @@ static int cmd_resize(int argc, char **argv)
 }
 
 static const char * const cmd_label_usage[] = {
-	"btrfs filesystem label <device> [<newlabel>]",
-	"Get or change the label of an unmounted filesystem",
+	"btrfs filesystem label [<device>|<mountpoint>] [<newlabel>]",
+	"Get or change the label of a filesystem",
 	"With one argument, get the label of filesystem on <device>.",
 	"If <newlabel> is passed, set the filesystem label to <newlabel>.",
 	NULL
-- 
1.7.9.5

Re: [RFC PATCH V5 2/2] Btrfs: Add a new ioctl to change the label of a mounted file system

[Miao Xie]

On 	mon, 17 Dec 2012 19:22:11 +0800, Jeff Liu wrote:
> Introduce a new ioctl BTRFS_IOC_SET_FSLABEL to change the label of a mounted file system.
> 
> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> Cc: Miao Xie <miaox@cn.fujitsu.com>
> 
> ---
>  fs/btrfs/ioctl.c |   40 ++++++++++++++++++++++++++++++++++++++++
>  fs/btrfs/ioctl.h |    2 ++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 6a2488a..0186651 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3712,6 +3712,44 @@ static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg)
>  	return ret ? -EFAULT : 0;
>  }
>  
> +static int btrfs_ioctl_set_fslabel(struct file *file, void __user *arg)
> +{
> +	struct btrfs_root *root = BTRFS_I(fdentry(file)->d_inode)->root;
> +	struct btrfs_super_block *super_block = root->fs_info->super_copy;
> +	char label[BTRFS_LABEL_SIZE];
> +	int ret;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	if (copy_from_user(label, arg, sizeof(label)))
> +		return -EFAULT;
> +
> +	if (strlen(label) > BTRFS_LABEL_SIZE - 1)
> +		return -EINVAL;

I think we should use strnlen()

Thanks
Miao

> +
> +	ret = mnt_want_write_file(file);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&root->fs_info->volume_mutex);
> +	trans = btrfs_start_transaction(root, 1);
> +	if (IS_ERR(trans)) {
> +		ret = PTR_ERR(trans);
> +		goto out_unlock;
> +	}
> +
> +	label[BTRFS_LABEL_SIZE - 1] = '\0';
> +	strcpy(super_block->label, label);
> +	btrfs_end_transaction(trans, root);
> +
> +out_unlock:
> +	mutex_unlock(&root->fs_info->volume_mutex);
> +	mnt_drop_write_file(file);
> +	return ret;
> +}
> +
>  long btrfs_ioctl(struct file *file, unsigned int
>  		cmd, unsigned long arg)
>  {
> @@ -3812,6 +3850,8 @@ long btrfs_ioctl(struct file *file, unsigned int
>  		return btrfs_ioctl_qgroup_limit(root, argp);
>  	case BTRFS_IOC_GET_FSLABEL:
>  		return btrfs_ioctl_get_fslabel(file, argp);
> +	case BTRFS_IOC_SET_FSLABEL:
> +		return btrfs_ioctl_set_fslabel(file, argp);
>  	}
>  
>  	return -ENOTTY;
> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
> index 5b2cbef..2abe239 100644
> --- a/fs/btrfs/ioctl.h
> +++ b/fs/btrfs/ioctl.h
> @@ -453,6 +453,8 @@ struct btrfs_ioctl_send_args {
>  			       struct btrfs_ioctl_qgroup_limit_args)
>  #define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \
>  				   char[BTRFS_LABEL_SIZE])
> +#define BTRFS_IOC_SET_FSLABEL _IOW(BTRFS_IOCTL_MAGIC, 50, \
> +				   char[BTRFS_LABEL_SIZE])
>  #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
>  				      struct btrfs_ioctl_get_dev_stats)
>  #endif
> 

Re: [RFC PATCH V5 1/2] Btrfs: Add a new ioctl to get the label of a mounted filesystem

[Miao Xie]

On 	mon, 17 Dec 2012 19:22:02 +0800, Jeff Liu wrote:
> Introduce a new ioctl BTRFS_IOC_GET_FSLABEL to fetch the label of a mounted file system.
> 
> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> Cc: Miao Xie <miaox@cn.fujitsu.com>

Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>

> 
> ---
>  fs/btrfs/ioctl.c |   15 +++++++++++++++
>  fs/btrfs/ioctl.h |    2 ++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 8fcf9a5..6a2488a 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3699,6 +3699,19 @@ out:
>  	return ret;
>  }
>  
> +static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg)
> +{
> +	struct btrfs_root *root = BTRFS_I(fdentry(file)->d_inode)->root;
> +	const char *label = root->fs_info->super_copy->label;
> +	int ret;
> +
> +	mutex_lock(&root->fs_info->volume_mutex);
> +	ret = copy_to_user(arg, label, strlen(label));
> +	mutex_unlock(&root->fs_info->volume_mutex);
> +
> +	return ret ? -EFAULT : 0;
> +}
> +
>  long btrfs_ioctl(struct file *file, unsigned int
>  		cmd, unsigned long arg)
>  {
> @@ -3797,6 +3810,8 @@ long btrfs_ioctl(struct file *file, unsigned int
>  		return btrfs_ioctl_qgroup_create(root, argp);
>  	case BTRFS_IOC_QGROUP_LIMIT:
>  		return btrfs_ioctl_qgroup_limit(root, argp);
> +	case BTRFS_IOC_GET_FSLABEL:
> +		return btrfs_ioctl_get_fslabel(file, argp);
>  	}
>  
>  	return -ENOTTY;
> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
> index 731e287..5b2cbef 100644
> --- a/fs/btrfs/ioctl.h
> +++ b/fs/btrfs/ioctl.h
> @@ -451,6 +451,8 @@ struct btrfs_ioctl_send_args {
>  			       struct btrfs_ioctl_qgroup_create_args)
>  #define BTRFS_IOC_QGROUP_LIMIT _IOR(BTRFS_IOCTL_MAGIC, 43, \
>  			       struct btrfs_ioctl_qgroup_limit_args)
> +#define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \
> +				   char[BTRFS_LABEL_SIZE])
>  #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
>  				      struct btrfs_ioctl_get_dev_stats)
>  #endif
> 

Re: [RFC PATCH V5 2/2] Btrfs: Add a new ioctl to change the label of a mounted file system

[Jeff Liu]

On 12/17/2012 07:57 PM, Miao Xie wrote:
> On 	mon, 17 Dec 2012 19:22:11 +0800, Jeff Liu wrote:
>> Introduce a new ioctl BTRFS_IOC_SET_FSLABEL to change the label of a mounted file system.
>>
>> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> Cc: Miao Xie <miaox@cn.fujitsu.com>
>>
>> ---
>>  fs/btrfs/ioctl.c |   40 ++++++++++++++++++++++++++++++++++++++++
>>  fs/btrfs/ioctl.h |    2 ++
>>  2 files changed, 42 insertions(+)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 6a2488a..0186651 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -3712,6 +3712,44 @@ static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg)
>>  	return ret ? -EFAULT : 0;
>>  }
>>  
>> +static int btrfs_ioctl_set_fslabel(struct file *file, void __user *arg)
>> +{
>> +	struct btrfs_root *root = BTRFS_I(fdentry(file)->d_inode)->root;
>> +	struct btrfs_super_block *super_block = root->fs_info->super_copy;
>> +	char label[BTRFS_LABEL_SIZE];
>> +	int ret;
>> +
>> +	if (!capable(CAP_SYS_ADMIN))
>> +		return -EPERM;
>> +
>> +	if (copy_from_user(label, arg, sizeof(label)))
>> +		return -EFAULT;
>> +
>> +	if (strlen(label) > BTRFS_LABEL_SIZE - 1)
>> +		return -EINVAL;
> 
> I think we should use strnlen()
AFAICS, strnlen() is better only if the caller need to get the length of
a length-limited string and make use of it proceeding, which means that
the procedure would not return an error even if the length is beyond the
limit.  Or if the caller need to examine if a length-limited string is
nul-terminated or not in a manner below,
if (strnlen(buf, MAX_BUF_SIZE) == MAX_BUF_SIZE) {
	....
}

I don't think it really needed here since the logic is clear with
strlen(), or Am I miss anything?


Thanks,
-Jeff

> Thanks
> Miao
> 
>> +
>> +	ret = mnt_want_write_file(file);
>> +	if (ret)
>> +		return ret;
>> +
>> +	mutex_lock(&root->fs_info->volume_mutex);
>> +	trans = btrfs_start_transaction(root, 1);
>> +	if (IS_ERR(trans)) {
>> +		ret = PTR_ERR(trans);
>> +		goto out_unlock;
>> +	}
>> +
>> +	label[BTRFS_LABEL_SIZE - 1] = '\0';
>> +	strcpy(super_block->label, label);
>> +	btrfs_end_transaction(trans, root);
>> +
>> +out_unlock:
>> +	mutex_unlock(&root->fs_info->volume_mutex);
>> +	mnt_drop_write_file(file);
>> +	return ret;
>> +}
>> +
>>  long btrfs_ioctl(struct file *file, unsigned int
>>  		cmd, unsigned long arg)
>>  {
>> @@ -3812,6 +3850,8 @@ long btrfs_ioctl(struct file *file, unsigned int
>>  		return btrfs_ioctl_qgroup_limit(root, argp);
>>  	case BTRFS_IOC_GET_FSLABEL:
>>  		return btrfs_ioctl_get_fslabel(file, argp);
>> +	case BTRFS_IOC_SET_FSLABEL:
>> +		return btrfs_ioctl_set_fslabel(file, argp);
>>  	}
>>  
>>  	return -ENOTTY;
>> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
>> index 5b2cbef..2abe239 100644
>> --- a/fs/btrfs/ioctl.h
>> +++ b/fs/btrfs/ioctl.h
>> @@ -453,6 +453,8 @@ struct btrfs_ioctl_send_args {
>>  			       struct btrfs_ioctl_qgroup_limit_args)
>>  #define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \
>>  				   char[BTRFS_LABEL_SIZE])
>> +#define BTRFS_IOC_SET_FSLABEL _IOW(BTRFS_IOCTL_MAGIC, 50, \
>> +				   char[BTRFS_LABEL_SIZE])
>>  #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
>>  				      struct btrfs_ioctl_get_dev_stats)
>>  #endif
>>
> 
> --
> 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
> 

Re: [RFC PATCH V5 2/2] Btrfs: Add a new ioctl to change the label of a mounted file system

[Goffredo Baroncelli]

On 12/17/2012 02:30 PM, Jeff Liu wrote:
> On 12/17/2012 07:57 PM, Miao Xie wrote:
>> On 	mon, 17 Dec 2012 19:22:11 +0800, Jeff Liu wrote:
>>> Introduce a new ioctl BTRFS_IOC_SET_FSLABEL to change the label of a mounted file system.
>>>
>>> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> Cc: Miao Xie <miaox@cn.fujitsu.com>
[...]
>>> +
>>> +	if (strlen(label) > BTRFS_LABEL_SIZE - 1)
>>> +		return -EINVAL;
>>
>> I think we should use strnlen()
> AFAICS, strnlen() is better only if the caller need to get the length of
> a length-limited string and make use of it proceeding, which means that
> the procedure would not return an error even if the length is beyond the
> limit.  Or if the caller need to examine if a length-limited string is
> nul-terminated or not in a manner below,
> if (strnlen(buf, MAX_BUF_SIZE) == MAX_BUF_SIZE) {
> 	....
> }
> 
> I don't think it really needed here since the logic is clear with
> strlen(), or Am I miss anything?

I think that Miao fears strlen() searching a zero could go beyond the
page limit touching an un-mapped page and raising an segmentation fault....

I think that we should change the code as

+	label[BTRFS_LABEL_SIZE - 1] = 0;
+
+	if (strlen(label) > BTRFS_LABEL_SIZE - 1)
+		return -EINVAL;

My 2¢

Ciao
G.Baroncelli
> 
> 
> Thanks,
> -Jeff
> 
>> Thanks
>> Miao
>>
>>> +
>>> +	ret = mnt_want_write_file(file);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	mutex_lock(&root->fs_info->volume_mutex);
>>> +	trans = btrfs_start_transaction(root, 1);
>>> +	if (IS_ERR(trans)) {
>>> +		ret = PTR_ERR(trans);
>>> +		goto out_unlock;
>>> +	}
>>> +
>>> +	label[BTRFS_LABEL_SIZE - 1] = '\0';
>>> +	strcpy(super_block->label, label);
>>> +	btrfs_end_transaction(trans, root);
>>> +
>>> +out_unlock:
>>> +	mutex_unlock(&root->fs_info->volume_mutex);
>>> +	mnt_drop_write_file(file);
>>> +	return ret;
>>> +}
>>> +
>>>  long btrfs_ioctl(struct file *file, unsigned int
>>>  		cmd, unsigned long arg)
>>>  {
>>> @@ -3812,6 +3850,8 @@ long btrfs_ioctl(struct file *file, unsigned int
>>>  		return btrfs_ioctl_qgroup_limit(root, argp);
>>>  	case BTRFS_IOC_GET_FSLABEL:
>>>  		return btrfs_ioctl_get_fslabel(file, argp);
>>> +	case BTRFS_IOC_SET_FSLABEL:
>>> +		return btrfs_ioctl_set_fslabel(file, argp);
>>>  	}
>>>  
>>>  	return -ENOTTY;
>>> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
>>> index 5b2cbef..2abe239 100644
>>> --- a/fs/btrfs/ioctl.h
>>> +++ b/fs/btrfs/ioctl.h
>>> @@ -453,6 +453,8 @@ struct btrfs_ioctl_send_args {
>>>  			       struct btrfs_ioctl_qgroup_limit_args)
>>>  #define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \
>>>  				   char[BTRFS_LABEL_SIZE])
>>> +#define BTRFS_IOC_SET_FSLABEL _IOW(BTRFS_IOCTL_MAGIC, 50, \
>>> +				   char[BTRFS_LABEL_SIZE])
>>>  #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
>>>  				      struct btrfs_ioctl_get_dev_stats)
>>>  #endif
>>>
>>
>> --
>> 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
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [RFC PATCH V5 2/2] Btrfs: Add a new ioctl to change the label of a mounted file system

[Jeff Liu]

On 12/18/2012 01:34 AM, Goffredo Baroncelli wrote:
> On 12/17/2012 02:30 PM, Jeff Liu wrote:
>> On 12/17/2012 07:57 PM, Miao Xie wrote:
>>> On 	mon, 17 Dec 2012 19:22:11 +0800, Jeff Liu wrote:
>>>> Introduce a new ioctl BTRFS_IOC_SET_FSLABEL to change the label of a mounted file system.
>>>>
>>>> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>> Cc: Miao Xie <miaox@cn.fujitsu.com>
> [...]
>>>> +
>>>> +	if (strlen(label) > BTRFS_LABEL_SIZE - 1)
>>>> +		return -EINVAL;
>>>
>>> I think we should use strnlen()
>> AFAICS, strnlen() is better only if the caller need to get the length of
>> a length-limited string and make use of it proceeding, which means that
>> the procedure would not return an error even if the length is beyond the
>> limit.  Or if the caller need to examine if a length-limited string is
>> nul-terminated or not in a manner below,
>> if (strnlen(buf, MAX_BUF_SIZE) == MAX_BUF_SIZE) {
>> 	....
>> }
>>
>> I don't think it really needed here since the logic is clear with
>> strlen(), or Am I miss anything?
> 
> I think that Miao fears strlen() searching a zero could go beyond the
> page limit touching an un-mapped page and raising an segmentation fault....
> 
> I think that we should change the code as
> 
> +	label[BTRFS_LABEL_SIZE - 1] = 0;
Ah, I moved above line for strcpy()...
> +
> +	if (strlen(label) > BTRFS_LABEL_SIZE - 1)
> +		return -EINVAL;
That's right, thank you!

-Jeff
> My 2¢
> 
> Ciao
> G.Baroncelli
>>
>>
>> Thanks,
>> -Jeff
>>
>>> Thanks
>>> Miao
>>>
>>>> +
>>>> +	ret = mnt_want_write_file(file);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	mutex_lock(&root->fs_info->volume_mutex);
>>>> +	trans = btrfs_start_transaction(root, 1);
>>>> +	if (IS_ERR(trans)) {
>>>> +		ret = PTR_ERR(trans);
>>>> +		goto out_unlock;
>>>> +	}
>>>> +
>>>> +	label[BTRFS_LABEL_SIZE - 1] = '\0';
>>>> +	strcpy(super_block->label, label);
>>>> +	btrfs_end_transaction(trans, root);
>>>> +
>>>> +out_unlock:
>>>> +	mutex_unlock(&root->fs_info->volume_mutex);
>>>> +	mnt_drop_write_file(file);
>>>> +	return ret;
>>>> +}
>>>> +
>>>>  long btrfs_ioctl(struct file *file, unsigned int
>>>>  		cmd, unsigned long arg)
>>>>  {
>>>> @@ -3812,6 +3850,8 @@ long btrfs_ioctl(struct file *file, unsigned int
>>>>  		return btrfs_ioctl_qgroup_limit(root, argp);
>>>>  	case BTRFS_IOC_GET_FSLABEL:
>>>>  		return btrfs_ioctl_get_fslabel(file, argp);
>>>> +	case BTRFS_IOC_SET_FSLABEL:
>>>> +		return btrfs_ioctl_set_fslabel(file, argp);
>>>>  	}
>>>>  
>>>>  	return -ENOTTY;
>>>> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
>>>> index 5b2cbef..2abe239 100644
>>>> --- a/fs/btrfs/ioctl.h
>>>> +++ b/fs/btrfs/ioctl.h
>>>> @@ -453,6 +453,8 @@ struct btrfs_ioctl_send_args {
>>>>  			       struct btrfs_ioctl_qgroup_limit_args)
>>>>  #define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \
>>>>  				   char[BTRFS_LABEL_SIZE])
>>>> +#define BTRFS_IOC_SET_FSLABEL _IOW(BTRFS_IOCTL_MAGIC, 50, \
>>>> +				   char[BTRFS_LABEL_SIZE])
>>>>  #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
>>>>  				      struct btrfs_ioctl_get_dev_stats)
>>>>  #endif
>>>>
>>>
>>> --
>>> 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
>>
> 
> 

Re: [RFC PATCH V5 2/2] Btrfs: Add a new ioctl to change the label of a mounted file system

[Miao Xie]

On 	mon, 17 Dec 2012 18:34:41 +0100, Goffredo Baroncelli wrote:
> On 12/17/2012 02:30 PM, Jeff Liu wrote:
>> On 12/17/2012 07:57 PM, Miao Xie wrote:
>>> On 	mon, 17 Dec 2012 19:22:11 +0800, Jeff Liu wrote:
>>>> Introduce a new ioctl BTRFS_IOC_SET_FSLABEL to change the label of a mounted file system.
>>>>
>>>> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>> Cc: Miao Xie <miaox@cn.fujitsu.com>
> [...]
>>>> +
>>>> +	if (strlen(label) > BTRFS_LABEL_SIZE - 1)
>>>> +		return -EINVAL;
>>>
>>> I think we should use strnlen()
>> AFAICS, strnlen() is better only if the caller need to get the length of
>> a length-limited string and make use of it proceeding, which means that
>> the procedure would not return an error even if the length is beyond the
>> limit.  Or if the caller need to examine if a length-limited string is
>> nul-terminated or not in a manner below,
>> if (strnlen(buf, MAX_BUF_SIZE) == MAX_BUF_SIZE) {
>> 	....
>> }
>>
>> I don't think it really needed here since the logic is clear with
>> strlen(), or Am I miss anything?
> 
> I think that Miao fears strlen() searching a zero could go beyond the
> page limit touching an un-mapped page and raising an segmentation fault....

Yes, so I think the following check is better.

if (strnlen(buf, BTRFS_LABEL_SIZE) == BTRFS_LABEL_SIZE)
	return -EINVAL;

Thanks
Miao

> I think that we should change the code as
> 
> +	label[BTRFS_LABEL_SIZE - 1] = 0;
> +
> +	if (strlen(label) > BTRFS_LABEL_SIZE - 1)
> +		return -EINVAL;
> 
> My 2¢
> 
> Ciao
> G.Baroncelli
>>
>>
>> Thanks,
>> -Jeff
>>
>>> Thanks
>>> Miao
>>>
>>>> +
>>>> +	ret = mnt_want_write_file(file);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	mutex_lock(&root->fs_info->volume_mutex);
>>>> +	trans = btrfs_start_transaction(root, 1);
>>>> +	if (IS_ERR(trans)) {
>>>> +		ret = PTR_ERR(trans);
>>>> +		goto out_unlock;
>>>> +	}
>>>> +
>>>> +	label[BTRFS_LABEL_SIZE - 1] = '\0';
>>>> +	strcpy(super_block->label, label);
>>>> +	btrfs_end_transaction(trans, root);
>>>> +
>>>> +out_unlock:
>>>> +	mutex_unlock(&root->fs_info->volume_mutex);
>>>> +	mnt_drop_write_file(file);
>>>> +	return ret;
>>>> +}
>>>> +
>>>>  long btrfs_ioctl(struct file *file, unsigned int
>>>>  		cmd, unsigned long arg)
>>>>  {
>>>> @@ -3812,6 +3850,8 @@ long btrfs_ioctl(struct file *file, unsigned int
>>>>  		return btrfs_ioctl_qgroup_limit(root, argp);
>>>>  	case BTRFS_IOC_GET_FSLABEL:
>>>>  		return btrfs_ioctl_get_fslabel(file, argp);
>>>> +	case BTRFS_IOC_SET_FSLABEL:
>>>> +		return btrfs_ioctl_set_fslabel(file, argp);
>>>>  	}
>>>>  
>>>>  	return -ENOTTY;
>>>> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
>>>> index 5b2cbef..2abe239 100644
>>>> --- a/fs/btrfs/ioctl.h
>>>> +++ b/fs/btrfs/ioctl.h
>>>> @@ -453,6 +453,8 @@ struct btrfs_ioctl_send_args {
>>>>  			       struct btrfs_ioctl_qgroup_limit_args)
>>>>  #define BTRFS_IOC_GET_FSLABEL _IOR(BTRFS_IOCTL_MAGIC, 49, \
>>>>  				   char[BTRFS_LABEL_SIZE])
>>>> +#define BTRFS_IOC_SET_FSLABEL _IOW(BTRFS_IOCTL_MAGIC, 50, \
>>>> +				   char[BTRFS_LABEL_SIZE])
>>>>  #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
>>>>  				      struct btrfs_ioctl_get_dev_stats)
>>>>  #endif
>>>>
>>>
>>> --
>>> 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
>>
> 
> 

Re: [RFC PATCH V5 2/2] Btrfs: Add a new ioctl to change the label of a mounted file system

[Jeff Liu]

On 12/18/2012 10:21 AM, Miao Xie wrote:
> On 	mon, 17 Dec 2012 18:34:41 +0100, Goffredo Baroncelli wrote:
>> On 12/17/2012 02:30 PM, Jeff Liu wrote:
>>> On 12/17/2012 07:57 PM, Miao Xie wrote:
>>>> On 	mon, 17 Dec 2012 19:22:11 +0800, Jeff Liu wrote:
>>>>> Introduce a new ioctl BTRFS_IOC_SET_FSLABEL to change the label of a mounted file system.
>>>>>
>>>>> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
>>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>>> Cc: Miao Xie <miaox@cn.fujitsu.com>
>> [...]
>>>>> +
>>>>> +	if (strlen(label) > BTRFS_LABEL_SIZE - 1)
>>>>> +		return -EINVAL;
>>>>
>>>> I think we should use strnlen()
>>> AFAICS, strnlen() is better only if the caller need to get the length of
>>> a length-limited string and make use of it proceeding, which means that
>>> the procedure would not return an error even if the length is beyond the
>>> limit.  Or if the caller need to examine if a length-limited string is
>>> nul-terminated or not in a manner below,
>>> if (strnlen(buf, MAX_BUF_SIZE) == MAX_BUF_SIZE) {
>>> 	....
>>> }
>>>
>>> I don't think it really needed here since the logic is clear with
>>> strlen(), or Am I miss anything?
>>
>> I think that Miao fears strlen() searching a zero could go beyond the
>> page limit touching an un-mapped page and raising an segmentation fault....
> 
> Yes, so I think the following check is better.
> 
> if (strnlen(buf, BTRFS_LABEL_SIZE) == BTRFS_LABEL_SIZE)
> 	return -EINVAL;
Generally speaking, the user would not input a large string for normal
purpose, so strnlen() will always have a bit waste(can be ignore here)
with the counter self-check. i.e. for (; count--, ;).
> Thanks
> Miao
> 
>> I think that we should change the code as
>>
>> +	label[BTRFS_LABEL_SIZE - 1] = 0;
>> +
>> +	if (strlen(label) > BTRFS_LABEL_SIZE - 1)
>> +		return -EINVAL;
Both suggestion are fine to me, but I prefer to above approach.

Thanks,
-Jeff

Re: [RFC PATCH V5 2/2] Btrfs: Add a new ioctl to change the label of a mounted file system

[Jeff Liu]

On 12/18/2012 10:33 AM, Jeff Liu wrote:
> On 12/18/2012 10:21 AM, Miao Xie wrote:
>> On 	mon, 17 Dec 2012 18:34:41 +0100, Goffredo Baroncelli wrote:
>>> On 12/17/2012 02:30 PM, Jeff Liu wrote:
>>>> On 12/17/2012 07:57 PM, Miao Xie wrote:
>>>>> On 	mon, 17 Dec 2012 19:22:11 +0800, Jeff Liu wrote:
>>>>>> Introduce a new ioctl BTRFS_IOC_SET_FSLABEL to change the label of a mounted file system.
>>>>>>
>>>>>> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
>>>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>>>> Cc: Miao Xie <miaox@cn.fujitsu.com>
>>> [...]
>>>>>> +
>>>>>> +	if (strlen(label) > BTRFS_LABEL_SIZE - 1)
>>>>>> +		return -EINVAL;
>>>>>
>>>>> I think we should use strnlen()
>>>> AFAICS, strnlen() is better only if the caller need to get the length of
>>>> a length-limited string and make use of it proceeding, which means that
>>>> the procedure would not return an error even if the length is beyond the
>>>> limit.  Or if the caller need to examine if a length-limited string is
>>>> nul-terminated or not in a manner below,
>>>> if (strnlen(buf, MAX_BUF_SIZE) == MAX_BUF_SIZE) {
>>>> 	....
>>>> }
>>>>
>>>> I don't think it really needed here since the logic is clear with
>>>> strlen(), or Am I miss anything?
>>>
>>> I think that Miao fears strlen() searching a zero could go beyond the
>>> page limit touching an un-mapped page and raising an segmentation fault....
>>
>> Yes, so I think the following check is better.
>>
>> if (strnlen(buf, BTRFS_LABEL_SIZE) == BTRFS_LABEL_SIZE)
>> 	return -EINVAL;
> Generally speaking, the user would not input a large string for normal
> purpose, so strnlen() will always have a bit waste(can be ignore here)
> with the counter self-check. i.e. for (; count--, ;).
>> Thanks
>> Miao
>>
>>> I think that we should change the code as
>>>
>>> +	label[BTRFS_LABEL_SIZE - 1] = 0;
>>> +
>>> +	if (strlen(label) > BTRFS_LABEL_SIZE - 1)
>>> +		return -EINVAL;
> Both suggestion are fine to me, but I prefer to above approach.
Oh No, Miao is right.  We can not perform the check as above because we
have already made the last character of label to NUL, hence
"strlen(label) > BTRFS_LABEL_SIZE -1" will be an invalid checking even
if the input string is longer than BTRFS_LABEL_SIZE -1.

Thanks,
-Jeff
> 
> Thanks,
> -Jeff
> --
> 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
>