* [PATCH] Btrfs: added new ioctl to set fs label
@ 2011-09-01 8:47 Jeff Liu
2011-09-01 9:00 ` Hugo Mills
0 siblings, 1 reply; 5+ messages in thread
From: Jeff Liu @ 2011-09-01 8:47 UTC (permalink / raw)
To: linux-btrfs; +Cc: chris.mason
Hello,
I'd like to introduce a new ioctl to set file system label.
With this feature, we can execute `btrfs filesystem label [label]
[path]` through btrfs tools to set or change the label.
Signed-off-by: Jie Liu <jeff.liu@oracle.com>
---
fs/btrfs/ctree.h | 6 ++++++
fs/btrfs/ioctl.c | 37 +++++++++++++++++++++++++++++++++++++
fs/btrfs/ioctl.h | 2 ++
3 files changed, 45 insertions(+), 0 deletions(-)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 03912c5..2889b5e 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1259,6 +1259,12 @@ struct btrfs_ioctl_defrag_range_args {
};
+struct btrfs_ioctl_fs_label_args {
+ /* label length in bytes */
+ __u32 len;
+ char label[BTRFS_LABEL_SIZE];
+};
+
/*
* inode items have the data typically returned from stat and store other
* info about object characteristics. There is one for every file and
dir in
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 970977a..9e01628 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -268,6 +268,41 @@ static int btrfs_ioctl_getversion(struct file
*file, int __user *arg)
return put_user(inode->i_generation, arg);
}
+static int btrfs_ioctl_fs_setlabel(struct btrfs_root *root, void __user
*arg)
+{
+ struct btrfs_super_block *super_block = &(root->fs_info->super_copy);
+ struct btrfs_ioctl_fs_label_args *label_args;
+ struct btrfs_trans_handle *trans;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (btrfs_root_readonly(root))
+ return -EROFS;
+
+ label_args = memdup_user(arg, sizeof(*label_args));
+ if (IS_ERR(label_args))
+ return PTR_ERR(label_args);
+
+ if (label_args->len >= sizeof(label_args->label))
+ return -EINVAL;
+
+ mutex_lock(&root->fs_info->volume_mutex);
+ trans = btrfs_start_transaction(root, 0);
+ if (IS_ERR(trans))
+ return PTR_ERR(trans);
+
+ if (snprintf(super_block->label, BTRFS_LABEL_SIZE, "%s",
+ label_args->label) != label_args->len)
+ return -EFAULT;
+
+ btrfs_end_transaction(trans, root);
+ mutex_unlock(&root->fs_info->volume_mutex);
+
+ kfree(label_args);
+ return 0;
+}
+
static noinline int btrfs_ioctl_fitrim(struct file *file, void __user
*arg)
{
struct btrfs_root *root = fdentry(file)->d_sb->s_fs_info;
@@ -2876,6 +2911,8 @@ long btrfs_ioctl(struct file *file, unsigned int
return btrfs_ioctl_fs_info(root, argp);
case BTRFS_IOC_DEV_INFO:
return btrfs_ioctl_dev_info(root, argp);
+ case BTRFS_IOC_FS_SETLABEL:
+ return btrfs_ioctl_fs_setlabel(root, argp);
case BTRFS_IOC_BALANCE:
return btrfs_balance(root->fs_info->dev_root);
case BTRFS_IOC_CLONE:
diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
index ad1ea78..1e0ca2a 100644
--- a/fs/btrfs/ioctl.h
+++ b/fs/btrfs/ioctl.h
@@ -248,4 +248,6 @@ struct btrfs_ioctl_space_args {
struct btrfs_ioctl_dev_info_args)
#define BTRFS_IOC_FS_INFO _IOR(BTRFS_IOCTL_MAGIC, 31, \
struct btrfs_ioctl_fs_info_args)
+#define BTRFS_IOC_FS_SETLABEL _IOW(BTRFS_IOCTL_MAGIC, 32, \
+ struct btrfs_ioctl_fs_label_args)
#endif
--
1.7.4.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Btrfs: added new ioctl to set fs label
2011-09-01 8:47 [PATCH] Btrfs: added new ioctl to set fs label Jeff Liu
@ 2011-09-01 9:00 ` Hugo Mills
2011-09-01 9:18 ` Jeff Liu
0 siblings, 1 reply; 5+ messages in thread
From: Hugo Mills @ 2011-09-01 9:00 UTC (permalink / raw)
To: Jeff Liu; +Cc: linux-btrfs, chris.mason
[-- Attachment #1: Type: text/plain, Size: 4393 bytes --]
On Thu, Sep 01, 2011 at 04:47:47PM +0800, Jeff Liu wrote:
> Hello,
>
> I'd like to introduce a new ioctl to set file system label.
> With this feature, we can execute `btrfs filesystem label [label]
> [path]` through btrfs tools to set or change the label.
>
> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
>
> ---
> fs/btrfs/ctree.h | 6 ++++++
> fs/btrfs/ioctl.c | 37 +++++++++++++++++++++++++++++++++++++
> fs/btrfs/ioctl.h | 2 ++
> 3 files changed, 45 insertions(+), 0 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 03912c5..2889b5e 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1259,6 +1259,12 @@ struct btrfs_ioctl_defrag_range_args {
> };
>
>
> +struct btrfs_ioctl_fs_label_args {
> + /* label length in bytes */
> + __u32 len;
> + char label[BTRFS_LABEL_SIZE];
> +};
Why do we need to provide a length here? Simply force a zero byte
at the end of the string when you copy it into kernel space, and then
use strcpy(). Then you have no need to test for length at all.
> /*
> * inode items have the data typically returned from stat and store other
> * info about object characteristics. There is one for every file
> and dir in
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 970977a..9e01628 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -268,6 +268,41 @@ static int btrfs_ioctl_getversion(struct file
> *file, int __user *arg)
> return put_user(inode->i_generation, arg);
> }
>
> +static int btrfs_ioctl_fs_setlabel(struct btrfs_root *root, void
> __user *arg)
> +{
> + struct btrfs_super_block *super_block = &(root->fs_info->super_copy);
> + struct btrfs_ioctl_fs_label_args *label_args;
> + struct btrfs_trans_handle *trans;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + if (btrfs_root_readonly(root))
> + return -EROFS;
> +
> + label_args = memdup_user(arg, sizeof(*label_args));
> + if (IS_ERR(label_args))
> + return PTR_ERR(label_args);
> +
> + if (label_args->len >= sizeof(label_args->label))
> + return -EINVAL;
Memory leak... you're not freeing label_args
> + mutex_lock(&root->fs_info->volume_mutex);
> + trans = btrfs_start_transaction(root, 0);
> + if (IS_ERR(trans))
> + return PTR_ERR(trans);
and here -- and you're leaving the mutex locked
> + if (snprintf(super_block->label, BTRFS_LABEL_SIZE, "%s",
> + label_args->label) != label_args->len)
> + return -EFAULT;
and here -- plus the transaction is still running
> + btrfs_end_transaction(trans, root);
> + mutex_unlock(&root->fs_info->volume_mutex);
> +
> + kfree(label_args);
> + return 0;
> +}
> +
> static noinline int btrfs_ioctl_fitrim(struct file *file, void
> __user *arg)
> {
> struct btrfs_root *root = fdentry(file)->d_sb->s_fs_info;
> @@ -2876,6 +2911,8 @@ long btrfs_ioctl(struct file *file, unsigned int
> return btrfs_ioctl_fs_info(root, argp);
> case BTRFS_IOC_DEV_INFO:
> return btrfs_ioctl_dev_info(root, argp);
> + case BTRFS_IOC_FS_SETLABEL:
> + return btrfs_ioctl_fs_setlabel(root, argp);
> case BTRFS_IOC_BALANCE:
> return btrfs_balance(root->fs_info->dev_root);
> case BTRFS_IOC_CLONE:
> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
> index ad1ea78..1e0ca2a 100644
> --- a/fs/btrfs/ioctl.h
> +++ b/fs/btrfs/ioctl.h
> @@ -248,4 +248,6 @@ struct btrfs_ioctl_space_args {
> struct btrfs_ioctl_dev_info_args)
> #define BTRFS_IOC_FS_INFO _IOR(BTRFS_IOCTL_MAGIC, 31, \
> struct btrfs_ioctl_fs_info_args)
> +#define BTRFS_IOC_FS_SETLABEL _IOW(BTRFS_IOCTL_MAGIC, 32, \
> + struct btrfs_ioctl_fs_label_args)
Could you use an unassigned number from [1], and update the wiki
page, please? (The page only went up yesterday, but it's been needed
for a while).
> #endif
Will there be a userspace patch for this along shortly?
Hugo.
[1] https://btrfs.wiki.kernel.org/index.php/Project_ideas#Development_notes.2C_please_read
--
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
--- You can't expect a boy to be depraved until he's gone to ---
a good school.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Btrfs: added new ioctl to set fs label
2011-09-01 9:00 ` Hugo Mills
@ 2011-09-01 9:18 ` Jeff Liu
2011-09-01 9:32 ` Hugo Mills
0 siblings, 1 reply; 5+ messages in thread
From: Jeff Liu @ 2011-09-01 9:18 UTC (permalink / raw)
To: Hugo Mills, linux-btrfs, chris.mason
Hi Hugo,
On 09/01/2011 05:00 PM, Hugo Mills wrote:
> On Thu, Sep 01, 2011 at 04:47:47PM +0800, Jeff Liu wrote:
>> Hello,
>>
>> I'd like to introduce a new ioctl to set file system label.
>> With this feature, we can execute `btrfs filesystem label [label]
>> [path]` through btrfs tools to set or change the label.
>>
>> Signed-off-by: Jie Liu<jeff.liu@oracle.com>
>>
>> ---
>> fs/btrfs/ctree.h | 6 ++++++
>> fs/btrfs/ioctl.c | 37 +++++++++++++++++++++++++++++++++++++
>> fs/btrfs/ioctl.h | 2 ++
>> 3 files changed, 45 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index 03912c5..2889b5e 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -1259,6 +1259,12 @@ struct btrfs_ioctl_defrag_range_args {
>> };
>>
>>
>> +struct btrfs_ioctl_fs_label_args {
>> + /* label length in bytes */
>> + __u32 len;
>> + char label[BTRFS_LABEL_SIZE];
>> +};
> Why do we need to provide a length here? Simply force a zero byte
> at the end of the string when you copy it into kernel space, and then
> use strcpy(). Then you have no need to test for length at all.
At first, I am afraid if an evil user may input an unexpected label
string with huge bytes to consume memory.
>> /*
>> * inode items have the data typically returned from stat and store other
>> * info about object characteristics. There is one for every file
>> and dir in
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 970977a..9e01628 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -268,6 +268,41 @@ static int btrfs_ioctl_getversion(struct file
>> *file, int __user *arg)
>> return put_user(inode->i_generation, arg);
>> }
>>
>> +static int btrfs_ioctl_fs_setlabel(struct btrfs_root *root, void
>> __user *arg)
>> +{
>> + struct btrfs_super_block *super_block =&(root->fs_info->super_copy);
>> + struct btrfs_ioctl_fs_label_args *label_args;
>> + struct btrfs_trans_handle *trans;
>> +
>> + if (!capable(CAP_SYS_ADMIN))
>> + return -EPERM;
>> +
>> + if (btrfs_root_readonly(root))
>> + return -EROFS;
>> +
>> + label_args = memdup_user(arg, sizeof(*label_args));
>> + if (IS_ERR(label_args))
>> + return PTR_ERR(label_args);
>> +
>> + if (label_args->len>= sizeof(label_args->label))
>> + return -EINVAL;
> Memory leak... you're not freeing label_args
>> + mutex_lock(&root->fs_info->volume_mutex);
>> + trans = btrfs_start_transaction(root, 0);
>> + if (IS_ERR(trans))
>> + return PTR_ERR(trans);
> and here -- and you're leaving the mutex locked
>
>> + if (snprintf(super_block->label, BTRFS_LABEL_SIZE, "%s",
>> + label_args->label) != label_args->len)
>> + return -EFAULT;
> and here -- plus the transaction is still running
Sorry for my stupid mistake and thanks for pointing those out!
>> + btrfs_end_transaction(trans, root);
>> + mutex_unlock(&root->fs_info->volume_mutex);
>> +
>> + kfree(label_args);
>> + return 0;
>> +}
>> +
>> static noinline int btrfs_ioctl_fitrim(struct file *file, void
>> __user *arg)
>> {
>> struct btrfs_root *root = fdentry(file)->d_sb->s_fs_info;
>> @@ -2876,6 +2911,8 @@ long btrfs_ioctl(struct file *file, unsigned int
>> return btrfs_ioctl_fs_info(root, argp);
>> case BTRFS_IOC_DEV_INFO:
>> return btrfs_ioctl_dev_info(root, argp);
>> + case BTRFS_IOC_FS_SETLABEL:
>> + return btrfs_ioctl_fs_setlabel(root, argp);
>> case BTRFS_IOC_BALANCE:
>> return btrfs_balance(root->fs_info->dev_root);
>> case BTRFS_IOC_CLONE:
>> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
>> index ad1ea78..1e0ca2a 100644
>> --- a/fs/btrfs/ioctl.h
>> +++ b/fs/btrfs/ioctl.h
>> @@ -248,4 +248,6 @@ struct btrfs_ioctl_space_args {
>> struct btrfs_ioctl_dev_info_args)
>> #define BTRFS_IOC_FS_INFO _IOR(BTRFS_IOCTL_MAGIC, 31, \
>> struct btrfs_ioctl_fs_info_args)
>> +#define BTRFS_IOC_FS_SETLABEL _IOW(BTRFS_IOCTL_MAGIC, 32, \
>> + struct btrfs_ioctl_fs_label_args)
> Could you use an unassigned number from [1], and update the wiki
> page, please? (The page only went up yesterday, but it's been needed
> for a while).
Ok, looks number 5 is free. :)
I'll update the wiki later.
Regards,
-Jeff
>> #endif
> Will there be a userspace patch for this along shortly?
>
> Hugo.
>
> [1] https://btrfs.wiki.kernel.org/index.php/Project_ideas#Development_notes.2C_please_read
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Btrfs: added new ioctl to set fs label
2011-09-01 9:18 ` Jeff Liu
@ 2011-09-01 9:32 ` Hugo Mills
2011-09-01 11:48 ` [PATCH] Btrfs: added new ioctl to set fs label V2 Jeff Liu
0 siblings, 1 reply; 5+ messages in thread
From: Hugo Mills @ 2011-09-01 9:32 UTC (permalink / raw)
To: Jeff Liu; +Cc: linux-btrfs, chris.mason
[-- Attachment #1: Type: text/plain, Size: 5661 bytes --]
On Thu, Sep 01, 2011 at 05:18:38PM +0800, Jeff Liu wrote:
> Hi Hugo,
>
> On 09/01/2011 05:00 PM, Hugo Mills wrote:
> >On Thu, Sep 01, 2011 at 04:47:47PM +0800, Jeff Liu wrote:
> >>Hello,
> >>
> >>I'd like to introduce a new ioctl to set file system label.
> >>With this feature, we can execute `btrfs filesystem label [label]
> >>[path]` through btrfs tools to set or change the label.
> >>
> >> Signed-off-by: Jie Liu<jeff.liu@oracle.com>
> >>
> >>---
> >> fs/btrfs/ctree.h | 6 ++++++
> >> fs/btrfs/ioctl.c | 37 +++++++++++++++++++++++++++++++++++++
> >> fs/btrfs/ioctl.h | 2 ++
> >> 3 files changed, 45 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> >>index 03912c5..2889b5e 100644
> >>--- a/fs/btrfs/ctree.h
> >>+++ b/fs/btrfs/ctree.h
> >>@@ -1259,6 +1259,12 @@ struct btrfs_ioctl_defrag_range_args {
> >> };
> >>
> >>
> >>+struct btrfs_ioctl_fs_label_args {
> >>+ /* label length in bytes */
> >>+ __u32 len;
> >>+ char label[BTRFS_LABEL_SIZE];
> >>+};
> > Why do we need to provide a length here? Simply force a zero byte
> >at the end of the string when you copy it into kernel space, and then
> >use strcpy(). Then you have no need to test for length at all.
> At first, I am afraid if an evil user may input an unexpected label
> string with huge bytes to consume memory.
That's why you force a known 0 byte at the end of the string when
you do the copy. (See below) Note also that the evil user can't
consume more than sizeof(btrfs_ioctl_fs_label_args) anyway, because
that's how much you're memdup()ing. The only issue is dealing with an
unterminated string... which you can fix by explicitly terminating it.
> >> /*
> >> * inode items have the data typically returned from stat and store other
> >> * info about object characteristics. There is one for every file
> >>and dir in
> >>diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> >>index 970977a..9e01628 100644
> >>--- a/fs/btrfs/ioctl.c
> >>+++ b/fs/btrfs/ioctl.c
> >>@@ -268,6 +268,41 @@ static int btrfs_ioctl_getversion(struct file
> >>*file, int __user *arg)
> >> return put_user(inode->i_generation, arg);
> >> }
> >>
> >>+static int btrfs_ioctl_fs_setlabel(struct btrfs_root *root, void
> >>__user *arg)
> >>+{
> >>+ struct btrfs_super_block *super_block =&(root->fs_info->super_copy);
> >>+ struct btrfs_ioctl_fs_label_args *label_args;
> >>+ struct btrfs_trans_handle *trans;
> >>+
> >>+ if (!capable(CAP_SYS_ADMIN))
> >>+ return -EPERM;
> >>+
> >>+ if (btrfs_root_readonly(root))
> >>+ return -EROFS;
> >>+
> >>+ label_args = memdup_user(arg, sizeof(*label_args));
> >>+ if (IS_ERR(label_args))
> >>+ return PTR_ERR(label_args);
label_args->label[BTRFS_LABEL_SIZE-1] = 0;
This guarantees that the string is no longer than
BTRFS_LABEL_SIZE-1 bytes long.
> >>+ if (label_args->len>= sizeof(label_args->label))
> >>+ return -EINVAL;
Having ensured that the string is no longer than our buffers, we
don't need this.
> > Memory leak... you're not freeing label_args
> >>+ mutex_lock(&root->fs_info->volume_mutex);
> >>+ trans = btrfs_start_transaction(root, 0);
> >>+ if (IS_ERR(trans))
> >>+ return PTR_ERR(trans);
> > and here -- and you're leaving the mutex locked
> >
> >>+ if (snprintf(super_block->label, BTRFS_LABEL_SIZE, "%s",
> >>+ label_args->label) != label_args->len)
It's now safe to use strcpy() here, since we know that the string
*must* be zero terminated at or before BTRFS_LABEL_SIZE.
> >>+ return -EFAULT;
> > and here -- plus the transaction is still running
> Sorry for my stupid mistake and thanks for pointing those out!
> >>+ btrfs_end_transaction(trans, root);
> >>+ mutex_unlock(&root->fs_info->volume_mutex);
> >>+
> >>+ kfree(label_args);
> >>+ return 0;
> >>+}
> >>+
> >> static noinline int btrfs_ioctl_fitrim(struct file *file, void
> >>__user *arg)
> >> {
> >> struct btrfs_root *root = fdentry(file)->d_sb->s_fs_info;
> >>@@ -2876,6 +2911,8 @@ long btrfs_ioctl(struct file *file, unsigned int
> >> return btrfs_ioctl_fs_info(root, argp);
> >> case BTRFS_IOC_DEV_INFO:
> >> return btrfs_ioctl_dev_info(root, argp);
> >>+ case BTRFS_IOC_FS_SETLABEL:
> >>+ return btrfs_ioctl_fs_setlabel(root, argp);
> >> case BTRFS_IOC_BALANCE:
> >> return btrfs_balance(root->fs_info->dev_root);
> >> case BTRFS_IOC_CLONE:
> >>diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
> >>index ad1ea78..1e0ca2a 100644
> >>--- a/fs/btrfs/ioctl.h
> >>+++ b/fs/btrfs/ioctl.h
> >>@@ -248,4 +248,6 @@ struct btrfs_ioctl_space_args {
> >> struct btrfs_ioctl_dev_info_args)
> >> #define BTRFS_IOC_FS_INFO _IOR(BTRFS_IOCTL_MAGIC, 31, \
> >> struct btrfs_ioctl_fs_info_args)
> >>+#define BTRFS_IOC_FS_SETLABEL _IOW(BTRFS_IOCTL_MAGIC, 32, \
> >>+ struct btrfs_ioctl_fs_label_args)
> > Could you use an unassigned number from [1], and update the wiki
> >page, please? (The page only went up yesterday, but it's been needed
> >for a while).
> Ok, looks number 5 is free. :)
> I'll update the wiki later.
>
>
> Regards,
> -Jeff
> >> #endif
> > Will there be a userspace patch for this along shortly?
> >
> > Hugo.
> >
> >[1] https://btrfs.wiki.kernel.org/index.php/Project_ideas#Development_notes.2C_please_read
> >
>
--
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
--- He's playing Schubert. I think Schubert is losing. ---
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] Btrfs: added new ioctl to set fs label V2
2011-09-01 9:32 ` Hugo Mills
@ 2011-09-01 11:48 ` Jeff Liu
0 siblings, 0 replies; 5+ messages in thread
From: Jeff Liu @ 2011-09-01 11:48 UTC (permalink / raw)
To: Hugo Mills; +Cc: linux-btrfs, chris.mason
Thanks again for your nice comments!
The wiki update is in progress.
Btw, is it make sense to improve the "struct btrfs_ioctl_fs_info_args"
to retrieve the label info through BTRFS_IOC_FS_INFO?
Would you please review the revised version?
Signed-off-by: Jie Liu <jeff.liu@oracle.com>
---
fs/btrfs/ctree.h | 4 ++++
fs/btrfs/ioctl.c | 36 ++++++++++++++++++++++++++++++++++++
fs/btrfs/ioctl.h | 2 ++
3 files changed, 42 insertions(+), 0 deletions(-)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 03912c5..a4669f0 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1259,6 +1259,10 @@ struct btrfs_ioctl_defrag_range_args {
};
+struct btrfs_ioctl_fs_label_args {
+ char label[BTRFS_LABEL_SIZE];
+};
+
/*
* inode items have the data typically returned from stat and store other
* info about object characteristics. There is one for every file and
dir in
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 970977a..c872e88 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -268,6 +268,40 @@ static int btrfs_ioctl_getversion(struct file
*file, int __user *arg)
return put_user(inode->i_generation, arg);
}
+static int btrfs_ioctl_fs_setlabel(struct btrfs_root *root, void __user
*arg)
+{
+ struct btrfs_super_block *super_block = &(root->fs_info->super_copy);
+ struct btrfs_ioctl_fs_label_args *label_args;
+ struct btrfs_trans_handle *trans;
+ int ret;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (btrfs_root_readonly(root))
+ return -EROFS;
+
+ label_args = memdup_user(arg, sizeof(*label_args));
+ if (IS_ERR(label_args))
+ return PTR_ERR(label_args);
+
+ label_args->label[BTRFS_LABEL_SIZE - 1] = '\0';
+
+ mutex_lock(&root->fs_info->volume_mutex);
+ trans = btrfs_start_transaction(root, 0);
+ if (IS_ERR(trans)) {
+ ret = PTR_ERR(trans);
+ goto out_unlock;
+ }
+ strcpy(super_block->label, label_args->label);
+ btrfs_end_transaction(trans, root);
+
+out_unlock:
+ mutex_unlock(&root->fs_info->volume_mutex);
+ kfree(label_args);
+ return 0;
+}
+
static noinline int btrfs_ioctl_fitrim(struct file *file, void __user
*arg)
{
struct btrfs_root *root = fdentry(file)->d_sb->s_fs_info;
@@ -2876,6 +2910,8 @@ long btrfs_ioctl(struct file *file, unsigned int
return btrfs_ioctl_fs_info(root, argp);
case BTRFS_IOC_DEV_INFO:
return btrfs_ioctl_dev_info(root, argp);
+ case BTRFS_IOC_FS_SETLABEL:
+ return btrfs_ioctl_fs_setlabel(root, argp);
case BTRFS_IOC_BALANCE:
return btrfs_balance(root->fs_info->dev_root);
case BTRFS_IOC_CLONE:
diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
index ad1ea78..cc3e33b 100644
--- a/fs/btrfs/ioctl.h
+++ b/fs/btrfs/ioctl.h
@@ -201,6 +201,8 @@ struct btrfs_ioctl_space_args {
struct btrfs_ioctl_vol_args)
#define BTRFS_IOC_SCAN_DEV _IOW(BTRFS_IOCTL_MAGIC, 4, \
struct btrfs_ioctl_vol_args)
+#define BTRFS_IOC_FS_SETLABEL _IOW(BTRFS_IOCTL_MAGIC, 5, \
+ struct btrfs_ioctl_fs_label_args)
/* trans start and trans end are dangerous, and only for
* use by applications that know how to avoid the
* resulting deadlocks
--
1.7.4.1
On 09/01/2011 05:32 PM, Hugo Mills wrote:
> On Thu, Sep 01, 2011 at 05:18:38PM +0800, Jeff Liu wrote:
>> Hi Hugo,
>>
>> On 09/01/2011 05:00 PM, Hugo Mills wrote:
>>> On Thu, Sep 01, 2011 at 04:47:47PM +0800, Jeff Liu wrote:
>>>> Hello,
>>>>
>>>> I'd like to introduce a new ioctl to set file system label.
>>>> With this feature, we can execute `btrfs filesystem label [label]
>>>> [path]` through btrfs tools to set or change the label.
>>>>
>>>> Signed-off-by: Jie Liu<jeff.liu@oracle.com>
>>>>
>>>> ---
>>>> fs/btrfs/ctree.h | 6 ++++++
>>>> fs/btrfs/ioctl.c | 37 +++++++++++++++++++++++++++++++++++++
>>>> fs/btrfs/ioctl.h | 2 ++
>>>> 3 files changed, 45 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>>>> index 03912c5..2889b5e 100644
>>>> --- a/fs/btrfs/ctree.h
>>>> +++ b/fs/btrfs/ctree.h
>>>> @@ -1259,6 +1259,12 @@ struct btrfs_ioctl_defrag_range_args {
>>>> };
>>>>
>>>>
>>>> +struct btrfs_ioctl_fs_label_args {
>>>> + /* label length in bytes */
>>>> + __u32 len;
>>>> + char label[BTRFS_LABEL_SIZE];
>>>> +};
>>> Why do we need to provide a length here? Simply force a zero byte
>>> at the end of the string when you copy it into kernel space, and then
>>> use strcpy(). Then you have no need to test for length at all.
>> At first, I am afraid if an evil user may input an unexpected label
>> string with huge bytes to consume memory.
> That's why you force a known 0 byte at the end of the string when
> you do the copy. (See below) Note also that the evil user can't
> consume more than sizeof(btrfs_ioctl_fs_label_args) anyway, because
> that's how much you're memdup()ing. The only issue is dealing with an
> unterminated string... which you can fix by explicitly terminating it.
>
>>>> /*
>>>> * inode items have the data typically returned from stat and store other
>>>> * info about object characteristics. There is one for every file
>>>> and dir in
>>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>>> index 970977a..9e01628 100644
>>>> --- a/fs/btrfs/ioctl.c
>>>> +++ b/fs/btrfs/ioctl.c
>>>> @@ -268,6 +268,41 @@ static int btrfs_ioctl_getversion(struct file
>>>> *file, int __user *arg)
>>>> return put_user(inode->i_generation, arg);
>>>> }
>>>>
>>>> +static int btrfs_ioctl_fs_setlabel(struct btrfs_root *root, void
>>>> __user *arg)
>>>> +{
>>>> + struct btrfs_super_block *super_block =&(root->fs_info->super_copy);
>>>> + struct btrfs_ioctl_fs_label_args *label_args;
>>>> + struct btrfs_trans_handle *trans;
>>>> +
>>>> + if (!capable(CAP_SYS_ADMIN))
>>>> + return -EPERM;
>>>> +
>>>> + if (btrfs_root_readonly(root))
>>>> + return -EROFS;
>>>> +
>>>> + label_args = memdup_user(arg, sizeof(*label_args));
>>>> + if (IS_ERR(label_args))
>>>> + return PTR_ERR(label_args);
> label_args->label[BTRFS_LABEL_SIZE-1] = 0;
>
> This guarantees that the string is no longer than
> BTRFS_LABEL_SIZE-1 bytes long.
>
>>>> + if (label_args->len>= sizeof(label_args->label))
>>>> + return -EINVAL;
> Having ensured that the string is no longer than our buffers, we
> don't need this.
>
>>> Memory leak... you're not freeing label_args
>>>> + mutex_lock(&root->fs_info->volume_mutex);
>>>> + trans = btrfs_start_transaction(root, 0);
>>>> + if (IS_ERR(trans))
>>>> + return PTR_ERR(trans);
>>> and here -- and you're leaving the mutex locked
>>>
>>>> + if (snprintf(super_block->label, BTRFS_LABEL_SIZE, "%s",
>>>> + label_args->label) != label_args->len)
> It's now safe to use strcpy() here, since we know that the string
> *must* be zero terminated at or before BTRFS_LABEL_SIZE.
>
>>>> + return -EFAULT;
>>> and here -- plus the transaction is still running
>> Sorry for my stupid mistake and thanks for pointing those out!
>>>> + btrfs_end_transaction(trans, root);
>>>> + mutex_unlock(&root->fs_info->volume_mutex);
>>>> +
>>>> + kfree(label_args);
>>>> + return 0;
>>>> +}
>>>> +
>>>> static noinline int btrfs_ioctl_fitrim(struct file *file, void
>>>> __user *arg)
>>>> {
>>>> struct btrfs_root *root = fdentry(file)->d_sb->s_fs_info;
>>>> @@ -2876,6 +2911,8 @@ long btrfs_ioctl(struct file *file, unsigned int
>>>> return btrfs_ioctl_fs_info(root, argp);
>>>> case BTRFS_IOC_DEV_INFO:
>>>> return btrfs_ioctl_dev_info(root, argp);
>>>> + case BTRFS_IOC_FS_SETLABEL:
>>>> + return btrfs_ioctl_fs_setlabel(root, argp);
>>>> case BTRFS_IOC_BALANCE:
>>>> return btrfs_balance(root->fs_info->dev_root);
>>>> case BTRFS_IOC_CLONE:
>>>> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
>>>> index ad1ea78..1e0ca2a 100644
>>>> --- a/fs/btrfs/ioctl.h
>>>> +++ b/fs/btrfs/ioctl.h
>>>> @@ -248,4 +248,6 @@ struct btrfs_ioctl_space_args {
>>>> struct btrfs_ioctl_dev_info_args)
>>>> #define BTRFS_IOC_FS_INFO _IOR(BTRFS_IOCTL_MAGIC, 31, \
>>>> struct btrfs_ioctl_fs_info_args)
>>>> +#define BTRFS_IOC_FS_SETLABEL _IOW(BTRFS_IOCTL_MAGIC, 32, \
>>>> + struct btrfs_ioctl_fs_label_args)
>>> Could you use an unassigned number from [1], and update the wiki
>>> page, please? (The page only went up yesterday, but it's been needed
>>> for a while).
>> Ok, looks number 5 is free. :)
>> I'll update the wiki later.
>>
>>
>> Regards,
>> -Jeff
>>>> #endif
>>> Will there be a userspace patch for this along shortly?
>>>
>>> Hugo.
>>>
>>> [1] https://btrfs.wiki.kernel.org/index.php/Project_ideas#Development_notes.2C_please_read
>>>
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-09-01 11:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-01 8:47 [PATCH] Btrfs: added new ioctl to set fs label Jeff Liu
2011-09-01 9:00 ` Hugo Mills
2011-09-01 9:18 ` Jeff Liu
2011-09-01 9:32 ` Hugo Mills
2011-09-01 11:48 ` [PATCH] Btrfs: added new ioctl to set fs label V2 Jeff Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox