Btrfs: get/set label of mounted file system

RFC PATCH Superseded Jeff Liu 3 participants 15 messages Dec 17, 2012 – Dec 18, 2012 Btrfs file system

lore

Share & tools

lore

Jeff Liu

author

2012-12-17 11:22 UTC #0 lore raw

v5 cover Btrfs: get/set label of mounted file system

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

Jeff Liu

author

2012-12-17 11:22 UTC #3 lore raw

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

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

Signed-off-by Jie Liu <[email protected]>

Signed-off-by Anand Jain <[email protected]>

Cc Miao Xie <[email protected]>

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

fs/btrfs/ioctl.c +39 -0

index 6a2488a..0186651 100644
@@ -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;

fs/btrfs/ioctl.h +2 -0

index 5b2cbef..2abe239 100644
@@ -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

Signature · 1 line

1.7.9.5

Jeff Liu

author

2012-12-17 11:22 UTC #1 lore raw

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

In reply to Jeff Liu

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

Signed-off-by Jie Liu <[email protected]>

Signed-off-by Anand Jain <[email protected]>

Cc Miao Xie <[email protected]>

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

fs/btrfs/ioctl.c +15 -0

index 8fcf9a5..6a2488a 100644
@@ -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;

fs/btrfs/ioctl.h +2 -0

index 731e287..5b2cbef 100644
@@ -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

Signature · 1 line

1.7.9.5

Jeff Liu

author

2012-12-17 11:35 UTC #11 lore raw

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

In reply to 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 <[email protected]>

Signed-off-by Anand Jain <[email protected]>

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

btrfslabel.c +49 ~~-25~~

index cb142b0..443eff6 100644
@@ -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)
 {

ioctl.h +2 -0

index 6fda3a1..0807b05 100644
@@ -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

utils.c +1 -1

index 205e667..d59bca3 100644
@@ -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;

utils.h +1 -0

index 3a0368b..8750f28 100644
@@ -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

Signature · 1 line

1.7.9.5

Jeff Liu

author

2012-12-17 11:35 UTC #12 lore raw

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

In reply to 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 <[email protected]>

Signed-off-by Anand Jain <[email protected]>

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

btrfslabel.c +55 ~~-35~~

index 443eff6..938e8b4 100644
@@ -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);
 }

ioctl.h +2 -0

index 0807b05..0882d6c 100644
@@ -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

Signature · 1 line

1.7.9.5

Jeff Liu

author

2012-12-17 11:35 UTC #13 lore raw

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

In reply to 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 <[email protected]>

Signed-off-by Anand Jain <[email protected]>

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

btrfslabel.c +8 -2

index 938e8b4..2826050 100644
@@ -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. */

Signature · 1 line

1.7.9.5

Jeff Liu

author

2012-12-17 11:35 UTC #14 lore raw

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

In reply to 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 <[email protected]>

Signed-off-by Anand Jain <[email protected]>

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

cmds-filesystem.c +2 -2

index 9c43d35..5770d8b 100644
@@ -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

Signature · 1 line

1.7.9.5

Miao Xie

2012-12-17 11:57 UTC #4 lore raw

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

In reply to Jeff Liu

Quoted 24 lines · Jeff Liu

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

Signed-off-by Jie Liu <[email protected]>

Signed-off-by Anand Jain <[email protected]>

Cc Miao Xie <[email protected]>

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

fs/btrfs/ioctl.c +5 -0

index 6a2488a..0186651 100644
@@ -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];

fs/btrfs/ioctl.c static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg) · line 3729

Quoted 10 lines

fs/btrfs/ioctl.c +10 -0

+	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

Quoted 48 lines

fs/btrfs/ioctl.c +24 -0

+
+	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;

fs/btrfs/ioctl.h +2 -0

index 5b2cbef..2abe239 100644
@@ -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

Miao Xie

Reviewed-by on v5 1/2

2012-12-17 11:58 UTC #2 lore raw

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

In reply to Jeff Liu

Quoted 5 lines · Jeff Liu

Introduce a new ioctl BTRFS_IOC_GET_FSLABEL to fetch the label of a mounted file system.
Signed-off-by Jie Liu <[email protected]>
Signed-off-by Anand Jain <[email protected]>
Cc Miao Xie <[email protected]>

Reviewed-by Miao Xie <[email protected]>

Quoted 53 lines

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

fs/btrfs/ioctl.c +15 -0

index 8fcf9a5..6a2488a 100644
@@ -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;

fs/btrfs/ioctl.h +2 -0

index 731e287..5b2cbef 100644
@@ -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

Jeff Liu

author

2012-12-17 13:30 UTC #5 lore raw

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

In reply to Miao Xie

Quoted 29 lines · Miao Xie

Quoted 28 lines · Jeff Liu

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

Signed-off-by Jie Liu <[email protected]>

Signed-off-by Anand Jain <[email protected]>

Cc Miao Xie <[email protected]>

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

fs/btrfs/ioctl.c +9 -0

index 6a2488a..0186651 100644
@@ -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;

Quoted 8 lines

fs/btrfs/ioctl.c static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg) · line 3729
Quoted 6 lines
fs/btrfs/ioctl.c +6 -0
+
+	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

Quoted 57 lines

Thanks
Miao

Quoted 40 lines

fs/btrfs/ioctl.c +24 -0

+
+	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;

fs/btrfs/ioctl.h +0 -0

index 5b2cbef..2abe239 100644

fs/btrfs/ioctl.h struct btrfs_ioctl_send_args { · line 460

Quoted 8 lines

fs/btrfs/ioctl.h +2 -0

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Goffredo Baroncelli

2012-12-17 17:32 UTC #6 lore raw

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

Quoted 7 lines · Jeff Liu

Quoted 6 lines · Miao Xie
Quoted 5 lines · Jeff Liu
Introduce a new ioctl BTRFS_IOC_SET_FSLABEL to change the label of a mounted file system.
Signed-off-by Jie Liu <[email protected]>
Signed-off-by Anand Jain <[email protected]>
Cc Miao Xie <[email protected]>

[...]

Quoted 10 lines

Quoted 5 lines
fs/btrfs/ioctl.c static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg) · line 3729
Quoted 3 lines
fs/btrfs/ioctl.c +3 -0
+
+	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,

Quoted 6 lines

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

Quoted 68 lines

Thanks,
-Jeff

Quoted 48 lines

Thanks
Miao

Quoted 45 lines

fs/btrfs/ioctl.c +24 -0

+
+	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;

fs/btrfs/ioctl.h +2 -0

index 5b2cbef..2abe239 100644
@@ -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, \

Quoted 9 lines

fs/btrfs/ioctl.h struct btrfs_ioctl_send_args { · line 460
Quoted 3 lines
fs/btrfs/ioctl.h +0 -0
 				      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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

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

Jeff Liu

author

2012-12-18 02:20 UTC #7 lore raw

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

Quoted 31 lines · Goffredo Baroncelli

Quoted 7 lines · Jeff Liu
Quoted 6 lines · Miao Xie
Quoted 5 lines · Jeff Liu
Introduce a new ioctl BTRFS_IOC_SET_FSLABEL to change the label of a mounted file system.
Signed-off-by Jie Liu <[email protected]>
Signed-off-by Anand Jain <[email protected]>
Cc Miao Xie <[email protected]>
[...]
Quoted 10 lines
Quoted 5 lines
fs/btrfs/ioctl.c static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg) · line 3729
Quoted 3 lines
fs/btrfs/ioctl.c +3 -0
+
+	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,
Quoted 6 lines
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

Review

Quoted 1 line

(quoted diff) +1 -0
+	label[BTRFS_LABEL_SIZE - 1] = 0;

Ah, I moved above line for strcpy()...

fs/btrfs/ioctl.c static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg) · line 3729

Quoted 3 lines

fs/btrfs/ioctl.c +3 -0

+
+	if (strlen(label) > BTRFS_LABEL_SIZE - 1)
+		return -EINVAL;

That's right, thank you!

-Jeff

Quoted 74 lines

My 2¢

Ciao
G.Baroncelli

Quoted 68 lines

Thanks,
-Jeff

Quoted 48 lines

Thanks
Miao

Quoted 45 lines

fs/btrfs/ioctl.c +24 -0

+
+	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;

fs/btrfs/ioctl.h +2 -0

index 5b2cbef..2abe239 100644
@@ -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, \

Quoted 9 lines

fs/btrfs/ioctl.h struct btrfs_ioctl_send_args { · line 460
Quoted 3 lines
fs/btrfs/ioctl.h +0 -0
 				      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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Miao Xie

2012-12-18 02:21 UTC #8 lore raw

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

In reply to Goffredo Baroncelli

Quoted 23 lines · Goffredo Baroncelli

Quoted 7 lines · Jeff Liu
Quoted 6 lines · Miao Xie
Quoted 5 lines · Jeff Liu
Introduce a new ioctl BTRFS_IOC_SET_FSLABEL to change the label of a mounted file system.
Signed-off-by Jie Liu <[email protected]>
Signed-off-by Anand Jain <[email protected]>
Cc Miao Xie <[email protected]>
[...]
Quoted 14 lines
Quoted 5 lines
fs/btrfs/ioctl.c static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg) · line 3729
Quoted 3 lines
fs/btrfs/ioctl.c +3 -0
+
+	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) {
	....
}

Quoted 5 lines

Quoted 2 lines
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

Quoted 81 lines

I think that we should change the code as

fs/btrfs/ioctl.c +4 -0

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

My 2¢

Ciao
G.Baroncelli

Quoted 68 lines

Thanks,
-Jeff

Quoted 48 lines

Thanks
Miao

Quoted 45 lines

fs/btrfs/ioctl.c +24 -0

+
+	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;

fs/btrfs/ioctl.h +2 -0

index 5b2cbef..2abe239 100644
@@ -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, \

Quoted 9 lines

fs/btrfs/ioctl.h struct btrfs_ioctl_send_args { · line 460
Quoted 3 lines
fs/btrfs/ioctl.h +0 -0
 				      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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jeff Liu

author

2012-12-18 02:33 UTC #9 lore raw

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

Quoted 27 lines · Miao Xie

Quoted 26 lines · Goffredo Baroncelli
Quoted 7 lines · Jeff Liu
Quoted 6 lines · Miao Xie
Quoted 5 lines · Jeff Liu
Introduce a new ioctl BTRFS_IOC_SET_FSLABEL to change the label of a mounted file system.
Signed-off-by Jie Liu <[email protected]>
Signed-off-by Anand Jain <[email protected]>
Cc Miao Xie <[email protected]>
[...]
Quoted 16 lines
Quoted 5 lines
fs/btrfs/ioctl.c static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg) · line 3729
Quoted 3 lines
fs/btrfs/ioctl.c +3 -0
+
+	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?

Quoted 7 lines

Quoted 2 lines
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--, ;).

Quoted 9 lines

Thanks
Miao
fs/btrfs/ioctl.c static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg) · line 3729
Quoted 6 lines
I think that we should change the code as
fs/btrfs/ioctl.c +4 -0
+	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

Jeff Liu

author

2012-12-18 02:47 UTC #10 lore raw

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

Quoted 41 lines · Jeff Liu

Quoted 27 lines · Miao Xie
Quoted 26 lines · Goffredo Baroncelli
Quoted 7 lines · Jeff Liu
Quoted 6 lines · Miao Xie
Quoted 5 lines · Jeff Liu
Introduce a new ioctl BTRFS_IOC_SET_FSLABEL to change the label of a mounted file system.
Signed-off-by Jie Liu <[email protected]>
Signed-off-by Anand Jain <[email protected]>
Cc Miao Xie <[email protected]>
[...]
Quoted 16 lines
Quoted 5 lines
fs/btrfs/ioctl.c static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg) · line 3729
Quoted 3 lines
fs/btrfs/ioctl.c +3 -0
+
+	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?
Quoted 7 lines
Quoted 2 lines
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--, ;).
Quoted 3 lines
Thanks
Miao

Quoted 7 lines

Quoted 6 lines
fs/btrfs/ioctl.c static int btrfs_ioctl_get_fslabel(struct file *file, void __user *arg) · line 3729
Quoted 6 lines
I think that we should change the code as
fs/btrfs/ioctl.c +4 -0
+	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

Quoted 8 lines

Thanks,
-Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html