diff mbox

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

Message ID 50CFDD9F.2080706@oracle.com (mailing list archive)
State New, archived
Headers show

Commit Message

jeff.liu Dec. 18, 2012, 3:06 a.m. UTC 
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>
Cc: Goffredo Baroncelli <kreijack@inwind.it>

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

Comments

Miao Xie Dec. 18, 2012, 3:31 a.m. UTC | #1 
On Tue, 18 Dec 2012 11:06:07 +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>
> Cc: Goffredo Baroncelli <kreijack@inwind.it>


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

> 
> ---
>  fs/btrfs/ioctl.c |   39 +++++++++++++++++++++++++++++++++++++++
>  fs/btrfs/ioctl.h |    2 ++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 6a2488a..2bb037e 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -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);
> +	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 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
Goffredo Baroncelli Dec. 18, 2012, 6 p.m. UTC | #2 
Hi Jeff,

On 12/18/2012 04:31 AM, Miao Xie wrote:
[...]
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
[...]

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

I think that you removed for mistake the following line

+	label[BTRFS_LABEL_SIZE - 1] = '\0';

In the V5 patch it was present.

May be we could replace strcpy() with strlcpy(super_block->label, label,
BTRFS_LABEL_SIZE-1) ?

BR
G.Baroncelli
jeff.liu Dec. 19, 2012, 3:42 a.m. UTC | #3 
Hi Goffredo,

Thanks for your review.

On 12/19/2012 02:00 AM, Goffredo Baroncelli wrote:
> Hi Jeff,
> 
> On 12/18/2012 04:31 AM, Miao Xie wrote:
> [...]
>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> [...]
> 
>>> +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);
> 
> I think that you removed for mistake the following line
> 
> +	label[BTRFS_LABEL_SIZE - 1] = '\0';
I removed it since it was used to cut the label string off the max array
size but now we have the previous strnlen().
> 
> In the V5 patch it was present.
> 
> May be we could replace strcpy() with strlcpy(super_block->label, label,
> BTRFS_LABEL_SIZE-1) ?
That is ok to me. However, it should be strlcpy(super_block->label,
label, BTRFS_LABEL_SIZE) ranther than 'BTRFS_LABREL_SIZE -1' because
strlcpy() does "size - 1" internally. i.e.

strlcpy(char *d, const char *s, size_t size)
{	
	size_t ret = strlen(s);

	.....
	size_t len = (ret >= size) ? size - 1 : ret;
	....
}

But does the current implementation make anything wrong? :)

Thanks,
-Jeff
> 
> BR
> G.Baroncelli
> 

--
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
David Sterba Dec. 19, 2012, 2:21 p.m. UTC | #4 
On Tue, Dec 18, 2012 at 11:06:07AM +0800, Jeff Liu wrote:
> +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);
> +	btrfs_end_transaction(trans, root);

If this fails, eg. with EIO, it will not be reported back to the user

        ret = btrfs_end_transaction(trans, root);

should fix it.

> +
> +out_unlock:
> +	mutex_unlock(&root->fs_info->volume_mutex);
> +	mnt_drop_write_file(file);
> +	return ret;
> +}
--
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
jeff.liu Dec. 20, 2012, 8:18 a.m. UTC | #5 
On 12/19/2012 10:21 PM, David Sterba wrote:
> On Tue, Dec 18, 2012 at 11:06:07AM +0800, Jeff Liu wrote:
>> +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);
>> +	btrfs_end_transaction(trans, root);
> 
> If this fails, eg. with EIO, it will not be reported back to the user
> 
>         ret = btrfs_end_transaction(trans, root);
> 
> should fix it.
Thank you, I'll send out a revised version after a little while.

-Jeff
> 
>> +
>> +out_unlock:
>> +	mutex_unlock(&root->fs_info->volume_mutex);
>> +	mnt_drop_write_file(file);
>> +	return ret;
>> +}
> --
> 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
diff mbox

Patch

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 6a2488a..2bb037e 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -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);
+	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 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