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
--
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
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).> #endifWill 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.
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 runningSorry 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 >-- 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
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. ---
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
>>>
--
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