* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.