Btrfs: get/set label of a mounted file system
RFC PATCH v7 1/2 Btrfs: Add a new ioctl to get the label of a mounted file system
In reply to Jeff LiuWith the new ioctl(2) BTRFS_IOC_GET_FSLABEL we can fetch the label of a mounted file system.
--- fs/btrfs/ioctl.c | 15 +++++++++++++++ fs/btrfs/ioctl.h | 2 ++ 2 files changed, 17 insertions(+)
fs/btrfs/ioctl.c +15 -0
@@ -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 intreturn 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
@@ -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
RFC PATCH v7 2/2 Btrfs: Add a new ioctl to set/change the label of a mounted file system
In reply to Jeff LiuWith the new ioctl(2) BTRFS_IOC_SET_FSLABEL, we can set/change the label of a mounted file system.
--- fs/btrfs/ioctl.c | 39 +++++++++++++++++++++++++++++++++++++++ fs/btrfs/ioctl.h | 2 ++ 2 files changed, 41 insertions(+)
fs/btrfs/ioctl.c +39 -0
@@ -3712,6 +3712,43 @@ 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; +struct btrfs_trans_handle *trans; +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 (strnlen (label,BTRFS_LABEL_SIZE ) ==BTRFS_LABEL_SIZE ) +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; + } + +strcpy (super_block->label, label); + ret =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 +3849,8 @@ long btrfs_ioctl(struct file *file, unsigned intreturn 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
@@ -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
v7 cover Btrfs: get/set label of a mounted file system
Hello,
Per David's comments upon v6, I missed a check up against the return value of btrfs_end_transaction for btrfs_ioctl_fs_setlabel(), it was fixed in this version.
v7->v6:
- take care of btrfs_end_transaction() in btrfs_ioctl_fs_setlabel() rather than keeping silence.
The old versions can be found at:
v6:
http://www.spinics.net/lists/linux-btrfs/msg20922.html
v5:
http://www.spinics.net/lists/linux-btrfs/msg20888.html
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
Re: [RFC PATCH v7 1/2] Btrfs: Add a new ioctl to get the label of a mounted file system
In reply to Jeff LiuHI Jeff,
Quoted 7 lines · Jeff Liu
With the new ioctl(2)
BTRFS_IOC_GET_FSLABELwe can 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]>Cc Goffredo Baroncelli <[email protected]>Cc David Sterba <[email protected]>
[...]
fs/btrfs/ioctl.c out: · line 3709
Quoted 8 lines
fs/btrfs/ioctl.c +8
-0+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));
Sorry for pointing out my doubt too late, but should we trust super_copy->label ?
An user could insert a usb-key with a btrfs filesystem with a label without zero. In this case strlen() could access outside super_copy->label[].
I think that it should be quite easy to alter artificially a filesystem to crash the kernel. So I not consider this as big problem. However *in case* of a further cycle of this patch I suggest to replace strlen() with strnlen().
Quoted 30 lines
fs/btrfs/ioctl.c +7
-0+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 intreturn 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@@ -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 · 2 lines
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
Re: [RFC PATCH v7 2/2] Btrfs: Add a new ioctl to set/change the label of a mounted file system
In reply to Jeff LiuQuoted 7 lines · Jeff Liu
With the new ioctl(2)
BTRFS_IOC_SET_FSLABEL, we can set/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]>Cc Goffredo Baroncelli <[email protected]>Cc David Sterba <[email protected]>
Quoted 75 lines
--- fs/btrfs/ioctl.c | 39 +++++++++++++++++++++++++++++++++++++++ fs/btrfs/ioctl.h | 2 ++ 2 files changed, 41 insertions(+)fs/btrfs/ioctl.c +39
-0@@ -3712,6 +3712,43 @@ 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; +struct btrfs_trans_handle *trans; +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 (strnlen (label,BTRFS_LABEL_SIZE ) ==BTRFS_LABEL_SIZE ) +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; + } + +strcpy (super_block->label, label); + ret =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 +3849,8 @@ long btrfs_ioctl(struct file *file, unsigned intreturn 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@@ -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 · 2 lines
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
Re: [RFC PATCH v7 1/2] Btrfs: Add a new ioctl to get the label of a mounted file system
In reply to Goffredo BaroncelliHi Goffredo,
Quoted 11 lines · Goffredo Baroncelli
HI Jeff,
Quoted 7 lines · Jeff Liu
With the new ioctl(2)
BTRFS_IOC_GET_FSLABELwe can 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]>Cc Goffredo Baroncelli <[email protected]>Cc David Sterba <[email protected]>[...]
Quoted 14 lines
fs/btrfs/ioctl.c out: · line 3709
Quoted 8 lines
fs/btrfs/ioctl.c +8
-0+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));Sorry for pointing out my doubt too late, but should we trust
super_copy->label?
An user could insert a usb-key with a btrfs filesystem with a label without zero. In this casestrlen()could access outside super_copy->label[].
Thank you for letting me be aware of this situation.
First of all, if the user set label via btrfs tools, he can not make it length exceeding BTRFS_LABLE_SIZE - 1.
If the user does that through codes wrote by himself like:btrfslabel.c->set_label_unmounted(), he can do that.
However, it most likely he did that for evil purpose or any other reasons?
Quoted 5 lines
I think that it should be quite easy to alter artificially a filesystem to crash the kernel. So I not consider this as big problem. However *in case* of a further cycle of this patch I suggest to replace
strlen()withstrnlen().
I don't think we should replace strlen() with strnlen() since it's totally wrong if the length of label is more than BTRFS_LABEL_SIZE -1, we can not just truncating the label and return it in this case. Add BUG_ON(strlen(label) > BTRFS_LABEL_SIZE - 1) is reasonable instead.
Thanks,
-Jeff
Quoted 33 lines
Quoted 31 lines
fs/btrfs/ioctl.c +7
-0+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 intreturn 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@@ -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 v7 1/2] Btrfs: Add a new ioctl to get the label of a mounted file system
Quoted 25 lines · Jeff Liu
Quoted 15 lines · Goffredo Baroncelli
fs/btrfs/ioctl.c out: · line 3709
Quoted 8 lines · Jeff Liu
fs/btrfs/ioctl.c +8
-0+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));Sorry for pointing out my doubt too late, but should we trust
super_copy->label?
An user could insert a usb-key with a btrfs filesystem with a label without zero. In this casestrlen()could access outside super_copy->label[].Thank you for letting me be aware of this situation.
First of all, if the user set label via btrfs tools, he can not make it length exceeding
BTRFS_LABLE_SIZE- 1.If the user does that through codes wrote by himself like:
btrfslabel.c->set_label_unmounted(), he can do that.
However, it most likely he did that for evil purpose or any other reasons?Quoted 1 line
Quoted 8 lines
Quoted 4 lines
I think that it should be quite easy to alter artificially a filesystem to crash the kernel. So I not consider this as big problem. However *in case* of a further cycle of this patch I suggest to replace
strlen()withstrnlen().I don't think we should replace
strlen()withstrnlen()since it's totally wrong if the length of label is more thanBTRFS_LABEL_SIZE-1, we can not just truncating the label and return it in this case. AddBUG_ON(strlen(label) > BTRFS_LABEL_SIZE - 1)is reasonable instead.
Don't allow users to attack the kernel! This would add a severe security issue. A BUG_ON() is something that you can use before the code would crash anyway, to prevent any additional damage and to help in debugging. A BUG() is not a method to report or handle user errors.
A Linux system is supposed to run until it is shutdown by the administrator, not until somebody inserts an USB stick.
Re: [RFC PATCH v7 1/2] Btrfs: Add a new ioctl to get the label of a mounted file system
In reply to Jeff LiuQuoted 28 lines · Jeff Liu
Hi Goffredo,
Quoted 11 lines · Goffredo Baroncelli
HI Jeff,
Quoted 7 lines · Jeff Liu
With the new ioctl(2)
BTRFS_IOC_GET_FSLABELwe can 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]>Cc Goffredo Baroncelli <[email protected]>Cc David Sterba <[email protected]>[...]
Quoted 14 lines
fs/btrfs/ioctl.c out: · line 3709
Quoted 8 lines
fs/btrfs/ioctl.c +8
-0+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));Sorry for pointing out my doubt too late, but should we trust
super_copy->label?
An user could insert a usb-key with a btrfs filesystem with a label without zero. In this casestrlen()could access outside super_copy->label[].
Quoted 8 lines
Thank you for letting me be aware of this situation.
First of all, if the user set label via btrfs tools, he can not make it length exceeding
BTRFS_LABLE_SIZE- 1.If the user does that through codes wrote by himself like:
btrfslabel.c->set_label_unmounted(), he can do that.
However, it most likely he did that for evil purpose or any other reasons?
I think the most likely case is the "evil purpose".
Quoted 8 lines
Quoted 5 lines
I think that it should be quite easy to alter artificially a filesystem to crash the kernel. So I not consider this as big problem. However *in case* of a further cycle of this patch I suggest to replace
strlen()withstrnlen().I don't think we should replace
strlen()withstrnlen()since it's totally wrong if the length of label is more thanBTRFS_LABEL_SIZE-1, we can not just truncating the label and return it in this case.
This for me is sufficient, or we could copy all the label buffer, without further check:
copy_to_user(arg, label, BTRFS_LABEL_SIZE)
Quoted 1 line
Add
BUG_ON(strlen(label) > BTRFS_LABEL_SIZE - 1)is reasonable instead.
I agree with Stefan, this is not a correct use of BUG_ON; a warning is sufficient (there is un-correct data read from disk).
Quoted 42 lines
Thanks,
-JeffQuoted 17 lines
Quoted 17 lines
fs/btrfs/ioctl.c +7
-0+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 intreturn 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); }Quoted 16 lines
Quoted 6 lines
fs/btrfs/ioctl.c +0
-0return -ENOTTY ;fs/btrfs/ioctl.h +0
-0fs/btrfs/ioctl.h struct btrfs_ioctl_send_args { · line 458
Quoted 8 lines
fs/btrfs/ioctl.h +2
-0@@ -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 --
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
Re: [RFC PATCH v7 1/2] Btrfs: Add a new ioctl to get the label of a mounted file system
Quoted 45 lines · Goffredo Baroncelli
Quoted 28 lines · Jeff Liu
Hi Goffredo,
Quoted 11 lines · Goffredo Baroncelli
HI Jeff,
Quoted 7 lines · Jeff Liu
With the new ioctl(2)
BTRFS_IOC_GET_FSLABELwe can 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]>Cc Goffredo Baroncelli <[email protected]>Cc David Sterba <[email protected]>[...]
Quoted 14 lines
fs/btrfs/ioctl.c out: · line 3709
Quoted 8 lines
fs/btrfs/ioctl.c +8
-0+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));Sorry for pointing out my doubt too late, but should we trust
super_copy->label?
An user could insert a usb-key with a btrfs filesystem with a label without zero. In this casestrlen()could access outside super_copy->label[].Quoted 8 lines
Thank you for letting me be aware of this situation.
First of all, if the user set label via btrfs tools, he can not make it length exceeding
BTRFS_LABLE_SIZE- 1.If the user does that through codes wrote by himself like:
btrfslabel.c->set_label_unmounted(), he can do that.
However, it most likely he did that for evil purpose or any other reasons?I think the most likely case is the "evil purpose".
Quoted 5 lines
Quoted 5 lines
I think that it should be quite easy to alter artificially a filesystem to crash the kernel. So I not consider this as big problem. However *in case* of a further cycle of this patch I suggest to replace
strlen()withstrnlen().
Quoted 8 lines
Quoted 3 lines
I don't think we should replace
strlen()withstrnlen()since it's totally wrong if the length of label is more thanBTRFS_LABEL_SIZE-1, we can not just truncating the label and return it in this case.This for me is sufficient, or we could copy all the label buffer, without further check:
copy_to_user(arg, label, BTRFS_LABEL_SIZE)
That sounds ok to me, but it's better to limit the length to be 'BTRFS_LABEL_SIZE - 1' bytes, or else, it would copy 256 bytes back to the user space if the label length is beyond or equal to the array limits.
Quoted 6 lines
Quoted 3 lines
Add
BUG_ON(strlen(label) > BTRFS_LABEL_SIZE - 1)is reasonable instead.I agree with Stefan, this is not a correct use of
BUG_ON; a warning is sufficient (there is un-correct data read from disk).
Maybe like following?
size_t len = strlen(label);
if (len > BTRFS_LABEL_SIZE - 1) {
WARN(1, "btrfs: device label has weird length %zu bytes, "
"it will be truncated to %d bytes.\n",
len, BTRFS_LABEL_SIZE - 1);
len = BTRFS_LABEL_SIZE - 1;
}I'll post a new patch with those changes if no other objections.
Happy Christmas.
-Jeff
Quoted 45 lines
Quoted 43 lines
Thanks,
-JeffQuoted 17 lines
Quoted 17 lines
fs/btrfs/ioctl.c +7
-0+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 intreturn 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); }Quoted 16 lines
Quoted 6 lines
fs/btrfs/ioctl.c +0
-0return -ENOTTY ;fs/btrfs/ioctl.h +0
-0fs/btrfs/ioctl.h struct btrfs_ioctl_send_args { · line 458
Quoted 8 lines
fs/btrfs/ioctl.h +2
-0@@ -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 --
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
Re: [RFC PATCH v7 1/2] Btrfs: Add a new ioctl to get the label of a mounted file system
Hi Jeff,
Quoted 2 lines · Jeff Liu
Quoted 1 line · Goffredo Baroncelli
On 12/21/2012 07:42 AM, Jeff Liu wrote:
[...]
Quoted 11 lines
Quoted 8 lines
Quoted 3 lines
I don't think we should replace
strlen()withstrnlen()since it's totally wrong if the length of label is more thanBTRFS_LABEL_SIZE-1, we can not just truncating the label and return it in this case.This for me is sufficient, or we could copy all the label buffer, without further check:
copy_to_user(arg, label, BTRFS_LABEL_SIZE)That sounds ok to me, but it's better to limit the length to be '
BTRFS_LABEL_SIZE- 1' bytes, or else, it would copy 256 bytes back to the user space if the label length is beyond or equal to the array limits.
Sorry I don't understand your reply. The ioctl has as argument an array of BTRFS_LABEL_SIZE characters, so it should not be any problem of page fault (with the exception of an user who passes a shorter array). So it wouldn't be any problem to copyBTRFS_LABEL_SIZE character, or I am missing something ?
The question is another. Is a kernel responsibility to assure that the returned string is zero terminated ? If yes (and I think so) we should truncate the string before the copy:
label[BTRFS_LABEL_SIZE-1] = 0; copy_to_user(arg, label, BTRFS_LABEL_SIZE);
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
Add
BUG_ON(strlen(label) > BTRFS_LABEL_SIZE - 1)is reasonable instead.I agree with Stefan, this is not a correct use of
BUG_ON; a warning is sufficient (there is un-correct data read from disk).Maybe like following?
size_tlen = strlen(label);
If label (who is an array read from the disk) is not zero terminated, this would lead to a possible page fault. I suggest to use strnlen() or similar.
Quoted 8 lines
if (len > BTRFS_LABEL_SIZE - 1) {
WARN(1, "btrfs: device label has weird length %zu bytes, "
"it will be truncated to %d bytes.\n",
len, BTRFS_LABEL_SIZE - 1);
len = BTRFS_LABEL_SIZE - 1;
}As message I suggest:
WARN(1, "btrfs: device label is not zero terminated, it will be truncated to %d bytes.\n",
BTRFS_LABEL-1 );
NOTE: it is suggested to not span the string on more lines, to help searching in the source a messages with grep. It is an exception of the rule of the max 80 columns text width.
Happy Christmas you too
GB
Quoted 52 lines
I'll post a new patch with those changes if no other objections.
Happy Christmas.
-JeffQuoted 45 lines
Quoted 43 lines
Thanks,
-JeffQuoted 17 lines
Quoted 17 lines
fs/btrfs/ioctl.c +7
-0+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 intreturn 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); }Quoted 16 lines
Quoted 6 lines
fs/btrfs/ioctl.c +0
-0return -ENOTTY ;fs/btrfs/ioctl.h +0
-0fs/btrfs/ioctl.h struct btrfs_ioctl_send_args { · line 458
Quoted 8 lines
fs/btrfs/ioctl.h +2
-0@@ -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 --
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
Re: [RFC PATCH v7 1/2] Btrfs: Add a new ioctl to get the label of a mounted file system
Quoted 18 lines · Goffredo Baroncelli
Hi Jeff,
Quoted 2 lines · Jeff Liu
Quoted 1 line · Goffredo Baroncelli
On 12/21/2012 07:42 AM, Jeff Liu wrote:
[...]
Quoted 11 lines
Quoted 8 lines
Quoted 3 lines
I don't think we should replace
strlen()withstrnlen()since it's totally wrong if the length of label is more thanBTRFS_LABEL_SIZE-1, we can not just truncating the label and return it in this case.This for me is sufficient, or we could copy all the label buffer, without further check:
copy_to_user(arg, label, BTRFS_LABEL_SIZE)That sounds ok to me, but it's better to limit the length to be '
BTRFS_LABEL_SIZE- 1' bytes, or else, it would copy 256 bytes back to the user space if the label length is beyond or equal to the array limits.
Quoted 5 lines
Sorry I don't understand your reply. The ioctl has as argument an array of
BTRFS_LABEL_SIZEcharacters, so it should not be any problem of page fault (with the exception of an user who passes a shorter array). So it wouldn't be any problem to copyBTRFS_LABEL_SIZEcharacter, or I am missing something ?
We can copy label length up to BTRFS_LABEL_SIZE back to the user space, but please see my response which were shown as following.
Quoted 4 lines
The question is another. Is a kernel responsibility to assure that the returned string is zero terminated ? If yes (and I think so) we should truncate the string before the copy:
On both the old btrfslabel.c->change_label_unmounted() and the new ioctl(BTRFS_IOC_SET_FSLABE), we all made the terminated character to be NUL(i.e. actually, the maximum length is 255), so that it's better to assure that the returned string to be same as well IMO.
Quoted 3 lines
label[BTRFS_LABEL_SIZE-1] = 0; copy_to_user(arg, label, BTRFS_LABEL_SIZE);
This definitely works, please see my response below again.
Quoted 10 lines
Quoted 10 lines
Quoted 7 lines
Quoted 4 lines
Add
BUG_ON(strlen(label) > BTRFS_LABEL_SIZE - 1)is reasonable instead.I agree with Stefan, this is not a correct use of
BUG_ON; a warning is sufficient (there is un-correct data read from disk).Maybe like following?
size_tlen = strlen(label);
As we have already evaluated the real length of the label, why not just use it to indicate the length that will be copied back?
Quoted 4 lines
If label (who is an array read from the disk) is not zero terminated, this would lead to a possible page fault. I suggest to use
strnlen()or similar.
Good point, that would make code better, i.e. avoid page fault. :)
Quoted 7 lines
Quoted 7 lines
if (len >
BTRFS_LABEL_SIZE- 1) {
WARN(1, "btrfs: device label has weird length %zu bytes, "
"it will be truncated to %d bytes.\n", len,BTRFS_LABEL_SIZE- 1);
Quoted 8 lines
Quoted 2 lines
len = BTRFS_LABEL_SIZE - 1; }As message I suggest:
WARN(1, "btrfs: device label is not zero terminated, it will be truncated to %d bytes.\n",
BTRFS_LABEL-1 );
Ok, I'd like to follow up with your suggestions as am not english native.
Quoted 5 lines
NOTE: it is suggested to not span the string on more lines, to help searching in the source a messages with grep. It is an exception of the rule of the max 80 columns text width.
That's why I splitted the warning info into two lines.
Thanks,
-Jeff
Quoted 58 lines
Happy Christmas you too
GBQuoted 52 lines
I'll post a new patch with those changes if no other objections.
Happy Christmas.
-JeffQuoted 45 lines
Quoted 43 lines
Thanks,
-JeffQuoted 17 lines
Quoted 17 lines
fs/btrfs/ioctl.c +7
-0+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 intreturn 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); }Quoted 16 lines
Quoted 6 lines
fs/btrfs/ioctl.c +0
-0return -ENOTTY ;fs/btrfs/ioctl.h +0
-0fs/btrfs/ioctl.h struct btrfs_ioctl_send_args { · line 458
Quoted 8 lines
fs/btrfs/ioctl.h +2
-0@@ -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 --
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
Re: [RFC PATCH v7 2/2] Btrfs: Add a new ioctl to set/change the label of a mounted file system
In reply to Jeff LiuQuoted 7 lines · Jeff Liu
With the new ioctl(2)
BTRFS_IOC_SET_FSLABEL, we can set/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]>Cc Goffredo Baroncelli <[email protected]>Cc David Sterba <[email protected]>