| Message ID | 20190402100742.8355-6-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix property bugs | expand |
On 2.04.19 г. 13:07 ч., Anand Jain wrote: > When the property fails to pass the prop_handlers::validate() check, the > thread should exit with no changes in the kernel, but as we are starting > the transaction too early, we have just updated the generation even if > there is no change. > > For example: > btrfs prop get /btrfs compression > compression=lzo > sync > btrfs in dump-super /dev/sdb | grep "^generation" > generation 32 > > Try to set an incomplete compression type > > btrfs prop set /btrfs compression zli > ERROR: failed to set compression for /btrfs: Invalid argument > sync > > Set failed but generation is incremented > btrfs in dump-super /dev/sdb | grep "^generation" > generation 33 <-- > > Fix it by collapsing btrfs_set_prop_trans() into btrfs_set_prop(), which > provides flexibility to start the transaction after the > prop_handlers::validate() has been checked. > > As of now if the prop_handlers::apply() fails then we still increment > the generation, but if there is strong prop_handlers::validate() then > the prop_handlers::apply() can never fail. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> SO what do we gain by this patch? Whether the transaction generation number (actually in your changelog it will be an improvement to mention transaction generation explicitly, because there is also inode generation...) is incremented or not is an implementation detail. It can be argued both ways whether it should or shouldn't be incremented? Does this have to do with how btrfs' log works? I.e the transaction generation is used to
On Tue, Apr 02, 2019 at 04:45:23PM +0300, Nikolay Borisov wrote: > > > On 2.04.19 г. 13:07 ч., Anand Jain wrote: > > When the property fails to pass the prop_handlers::validate() check, the > > thread should exit with no changes in the kernel, but as we are starting > > the transaction too early, we have just updated the generation even if > > there is no change. > > > > For example: > > btrfs prop get /btrfs compression > > compression=lzo > > sync > > btrfs in dump-super /dev/sdb | grep "^generation" > > generation 32 > > > > Try to set an incomplete compression type > > > > btrfs prop set /btrfs compression zli > > ERROR: failed to set compression for /btrfs: Invalid argument > > sync > > > > Set failed but generation is incremented > > btrfs in dump-super /dev/sdb | grep "^generation" > > generation 33 <-- > > > > Fix it by collapsing btrfs_set_prop_trans() into btrfs_set_prop(), which > > provides flexibility to start the transaction after the > > prop_handlers::validate() has been checked. > > > > As of now if the prop_handlers::apply() fails then we still increment > > the generation, but if there is strong prop_handlers::validate() then > > the prop_handlers::apply() can never fail. > > > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > > SO what do we gain by this patch? Whether the transaction generation > number (actually in your changelog it will be an improvement to mention > transaction generation explicitly, because there is also inode > generation...) is incremented or not is an implementation detail. No, the problem is that the order of the preliminary checks and the transaction start is reversed. So first the transaction, then do sanity checks, that are otherwise don't depend on the transaction. Many other places have been updatd to do the lighweight checks first and keep the transaction region minimal.
On Tue, Apr 02, 2019 at 06:07:42PM +0800, Anand Jain wrote: > When the property fails to pass the prop_handlers::validate() check, the > thread should exit with no changes in the kernel, but as we are starting > the transaction too early, we have just updated the generation even if > there is no change. Agreed, this should be fixed. > > For example: > btrfs prop get /btrfs compression > compression=lzo > sync > btrfs in dump-super /dev/sdb | grep "^generation" > generation 32 > > Try to set an incomplete compression type > > btrfs prop set /btrfs compression zli > ERROR: failed to set compression for /btrfs: Invalid argument > sync > > Set failed but generation is incremented > btrfs in dump-super /dev/sdb | grep "^generation" > generation 33 <-- > > Fix it by collapsing btrfs_set_prop_trans() into btrfs_set_prop(), which > provides flexibility to start the transaction after the > prop_handlers::validate() has been checked. Please split the changes, the collapsed function make it difficult to review. It does not matter if you first clean up the functions and then switch the transaction, or the other way around. > As of now if the prop_handlers::apply() fails then we still increment > the generation, but if there is strong prop_handlers::validate() then > the prop_handlers::apply() can never fail. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/ioctl.c | 10 ++++------ > fs/btrfs/props.c | 59 +++++++++++++++++++++++++------------------------------- > fs/btrfs/props.h | 4 ++-- > fs/btrfs/xattr.c | 2 +- > 4 files changed, 33 insertions(+), 42 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 28ee9fe6edb4..0eeaf9a68082 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -284,8 +284,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg) > binode->flags &= ~BTRFS_INODE_COMPRESS; > binode->flags |= BTRFS_INODE_NOCOMPRESS; > > - ret = btrfs_set_prop_trans(inode, "btrfs.compression", NULL, > - 0, 0); > + ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0); > if (ret && ret != -ENODATA) > goto out_drop; > } else if (fsflags & FS_COMPR_FL) { > @@ -303,14 +302,13 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg) > if (!comp || comp[0] == 0) > comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB); > > - ret = btrfs_set_prop_trans(inode, "btrfs.compression", comp, > - strlen(comp), 0); > + ret = btrfs_set_prop(inode, "btrfs.compression", comp, > + strlen(comp), 0); > if (ret) > goto out_drop; > > } else { > - ret = btrfs_set_prop_trans(inode, "btrfs.compression", NULL, > - 0, 0); > + ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0); > if (ret && ret != -ENODATA) > goto out_drop; > binode->flags &= ~(BTRFS_INODE_COMPRESS | BTRFS_INODE_NOCOMPRESS); > diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c > index fb84e76f3b1d..99fa2459ba61 100644 > --- a/fs/btrfs/props.c > +++ b/fs/btrfs/props.c > @@ -56,11 +56,12 @@ static const struct hlist_head *find_prop_handlers_by_hash(const u64 hash) > return NULL; > } > > -static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, > - const char *name, const char *value, size_t value_len, > - int flags) > +int btrfs_set_prop(struct inode *inode, const char *name, const char *value, > + size_t value_len, int flags) > { > + struct btrfs_root *root = BTRFS_I(inode)->root; > const struct prop_handler *handler; > + struct btrfs_trans_handle *trans; > int ret; > > if (btrfs_root_readonly(BTRFS_I(inode)->root)) > @@ -74,50 +75,40 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, > return -EINVAL; > > if (value_len == 0) { > + /* Its called to reset the property */ > + trans = btrfs_start_transaction(root, 2); > + if (IS_ERR(trans)) > + return PTR_ERR(trans); > + > ret = btrfs_setxattr(trans, inode, handler->xattr_name, > NULL, 0, flags); > - if (ret) > - return ret; > - > - ret = handler->apply(inode, NULL, 0); > - ASSERT(ret == 0); > - > - return ret; > + if (!ret) { > + ret = handler->apply(inode, NULL, 0); > + ASSERT(ret == 0); > + } > + goto out; This block would be better in a separate helper, now that it starts the transaction, eg. if (value_len == 0) return do_reset_property(inode, handler, flags); > } > > ret = handler->validate(value, value_len); > if (ret) > return ret; > + > + trans = btrfs_start_transaction(root, 2); > + if (IS_ERR(trans)) > + return PTR_ERR(trans); > + > ret = btrfs_setxattr(trans, inode, handler->xattr_name, > value, value_len, flags); > if (ret) > - return ret; > + goto out; > + > ret = handler->apply(inode, value, value_len); > if (ret) { > + /* Apply failed. Reset the property. */ > btrfs_setxattr(trans, inode, handler->xattr_name, > NULL, 0, flags); So that's not new code, but I wonder whether this is correct at all. Reset means to the original value or to empty value? The problem here is that it's under transaction, so this should be followed by an abort. It maybe is possible to safely revert the value without aborting, as the inode is locked. > - return ret; > - } > - > - set_bit(BTRFS_INODE_HAS_PROPS, &BTRFS_I(inode)->runtime_flags); > - > - return 0; > -} > - > -int btrfs_set_prop_trans(struct inode *inode, const char *name, > - const char *value, size_t value_len, int flags) > -{ > - struct btrfs_root *root = BTRFS_I(inode)->root; > - struct btrfs_trans_handle *trans; > - int ret; > - > - trans = btrfs_start_transaction(root, 2); > - if (IS_ERR(trans)) > - return PTR_ERR(trans); > - > - ret = btrfs_set_prop(trans, inode, name, value, value_len, flags); > - > - if (!ret) { > + } else { > + set_bit(BTRFS_INODE_HAS_PROPS, &BTRFS_I(inode)->runtime_flags); > inode_inc_iversion(inode); > inode->i_ctime = current_time(inode); > set_bit(BTRFS_INODE_COPY_EVERYTHING, > @@ -125,6 +116,8 @@ int btrfs_set_prop_trans(struct inode *inode, const char *name, > ret = btrfs_update_inode(trans, root, inode); > BUG_ON(ret); > } > + > +out: > btrfs_end_transaction(trans); > return ret; > }
On 3/4/19 6:04 AM, David Sterba wrote: > On Tue, Apr 02, 2019 at 06:07:42PM +0800, Anand Jain wrote: >> When the property fails to pass the prop_handlers::validate() check, the >> thread should exit with no changes in the kernel, but as we are starting >> the transaction too early, we have just updated the generation even if >> there is no change. > > Agreed, this should be fixed. > >> >> For example: >> btrfs prop get /btrfs compression >> compression=lzo >> sync >> btrfs in dump-super /dev/sdb | grep "^generation" >> generation 32 >> >> Try to set an incomplete compression type >> >> btrfs prop set /btrfs compression zli >> ERROR: failed to set compression for /btrfs: Invalid argument >> sync >> >> Set failed but generation is incremented >> btrfs in dump-super /dev/sdb | grep "^generation" >> generation 33 <-- >> >> Fix it by collapsing btrfs_set_prop_trans() into btrfs_set_prop(), which >> provides flexibility to start the transaction after the >> prop_handlers::validate() has been checked. > > Please split the changes, the collapsed function make it difficult to > review. It does not matter if you first clean up the functions and then > switch the transaction, or the other way around. will do. [1] >> As of now if the prop_handlers::apply() fails then we still increment >> the generation, but if there is strong prop_handlers::validate() then >> the prop_handlers::apply() can never fail. >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> fs/btrfs/ioctl.c | 10 ++++------ >> fs/btrfs/props.c | 59 +++++++++++++++++++++++++------------------------------- >> fs/btrfs/props.h | 4 ++-- >> fs/btrfs/xattr.c | 2 +- >> 4 files changed, 33 insertions(+), 42 deletions(-) >> >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >> index 28ee9fe6edb4..0eeaf9a68082 100644 >> --- a/fs/btrfs/ioctl.c >> +++ b/fs/btrfs/ioctl.c >> @@ -284,8 +284,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg) >> binode->flags &= ~BTRFS_INODE_COMPRESS; >> binode->flags |= BTRFS_INODE_NOCOMPRESS; >> >> - ret = btrfs_set_prop_trans(inode, "btrfs.compression", NULL, >> - 0, 0); >> + ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0); >> if (ret && ret != -ENODATA) >> goto out_drop; >> } else if (fsflags & FS_COMPR_FL) { >> @@ -303,14 +302,13 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg) >> if (!comp || comp[0] == 0) >> comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB); >> >> - ret = btrfs_set_prop_trans(inode, "btrfs.compression", comp, >> - strlen(comp), 0); >> + ret = btrfs_set_prop(inode, "btrfs.compression", comp, >> + strlen(comp), 0); >> if (ret) >> goto out_drop; >> >> } else { >> - ret = btrfs_set_prop_trans(inode, "btrfs.compression", NULL, >> - 0, 0); >> + ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0); >> if (ret && ret != -ENODATA) >> goto out_drop; >> binode->flags &= ~(BTRFS_INODE_COMPRESS | BTRFS_INODE_NOCOMPRESS); >> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c >> index fb84e76f3b1d..99fa2459ba61 100644 >> --- a/fs/btrfs/props.c >> +++ b/fs/btrfs/props.c >> @@ -56,11 +56,12 @@ static const struct hlist_head *find_prop_handlers_by_hash(const u64 hash) >> return NULL; >> } >> >> -static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, >> - const char *name, const char *value, size_t value_len, >> - int flags) >> +int btrfs_set_prop(struct inode *inode, const char *name, const char *value, >> + size_t value_len, int flags) >> { >> + struct btrfs_root *root = BTRFS_I(inode)->root; >> const struct prop_handler *handler; >> + struct btrfs_trans_handle *trans; >> int ret; >> >> if (btrfs_root_readonly(BTRFS_I(inode)->root)) >> @@ -74,50 +75,40 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, >> return -EINVAL; >> >> if (value_len == 0) { >> + /* Its called to reset the property */ >> + trans = btrfs_start_transaction(root, 2); >> + if (IS_ERR(trans)) >> + return PTR_ERR(trans); >> + >> ret = btrfs_setxattr(trans, inode, handler->xattr_name, >> NULL, 0, flags); >> - if (ret) >> - return ret; >> - >> - ret = handler->apply(inode, NULL, 0); >> - ASSERT(ret == 0); >> - >> - return ret; >> + if (!ret) { >> + ret = handler->apply(inode, NULL, 0); >> + ASSERT(ret == 0); >> + } >> + goto out; > > This block would be better in a separate helper, now that it starts the > transaction, eg. > > if (value_len == 0) > return do_reset_property(inode, handler, flags); I was thinking same too. Will fix. >> } >> >> ret = handler->validate(value, value_len); >> if (ret) >> return ret; >> + >> + trans = btrfs_start_transaction(root, 2); >> + if (IS_ERR(trans)) >> + return PTR_ERR(trans); >> + >> ret = btrfs_setxattr(trans, inode, handler->xattr_name, >> value, value_len, flags); >> if (ret) >> - return ret; >> + goto out; >> + >> ret = handler->apply(inode, value, value_len); >> if (ret) { >> + /* Apply failed. Reset the property. */ >> btrfs_setxattr(trans, inode, handler->xattr_name, >> NULL, 0, flags); > > So that's not new code, but I wonder whether this is correct at all. > Reset means to the original value or to empty value? The problem here is > that it's under transaction, so this should be followed by an abort. > > It maybe is possible to safely revert the value without aborting, as the > inode is locked. As I mentioned [1] (above), we have this bug in the original code which is impossible to achieve, because apply() can never as the validate() has passed. So we don't need additional check in apply() for which it may return fail? This works as of now with the compression property. It should be possible to follow the same design for other upcoming property readmirror and encryption. Thanks, Anand >> - return ret; >> - } >> - >> - set_bit(BTRFS_INODE_HAS_PROPS, &BTRFS_I(inode)->runtime_flags); >> - >> - return 0; >> -} >> - >> -int btrfs_set_prop_trans(struct inode *inode, const char *name, >> - const char *value, size_t value_len, int flags) >> -{ >> - struct btrfs_root *root = BTRFS_I(inode)->root; >> - struct btrfs_trans_handle *trans; >> - int ret; >> - >> - trans = btrfs_start_transaction(root, 2); >> - if (IS_ERR(trans)) >> - return PTR_ERR(trans); >> - >> - ret = btrfs_set_prop(trans, inode, name, value, value_len, flags); >> - >> - if (!ret) { >> + } else { >> + set_bit(BTRFS_INODE_HAS_PROPS, &BTRFS_I(inode)->runtime_flags); >> inode_inc_iversion(inode); >> inode->i_ctime = current_time(inode); >> set_bit(BTRFS_INODE_COPY_EVERYTHING, >> @@ -125,6 +116,8 @@ int btrfs_set_prop_trans(struct inode *inode, const char *name, >> ret = btrfs_update_inode(trans, root, inode); >> BUG_ON(ret); >> } >> + >> +out: >> btrfs_end_transaction(trans); >> return ret; >> }
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 28ee9fe6edb4..0eeaf9a68082 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -284,8 +284,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg) binode->flags &= ~BTRFS_INODE_COMPRESS; binode->flags |= BTRFS_INODE_NOCOMPRESS; - ret = btrfs_set_prop_trans(inode, "btrfs.compression", NULL, - 0, 0); + ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0); if (ret && ret != -ENODATA) goto out_drop; } else if (fsflags & FS_COMPR_FL) { @@ -303,14 +302,13 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg) if (!comp || comp[0] == 0) comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB); - ret = btrfs_set_prop_trans(inode, "btrfs.compression", comp, - strlen(comp), 0); + ret = btrfs_set_prop(inode, "btrfs.compression", comp, + strlen(comp), 0); if (ret) goto out_drop; } else { - ret = btrfs_set_prop_trans(inode, "btrfs.compression", NULL, - 0, 0); + ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0); if (ret && ret != -ENODATA) goto out_drop; binode->flags &= ~(BTRFS_INODE_COMPRESS | BTRFS_INODE_NOCOMPRESS); diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c index fb84e76f3b1d..99fa2459ba61 100644 --- a/fs/btrfs/props.c +++ b/fs/btrfs/props.c @@ -56,11 +56,12 @@ static const struct hlist_head *find_prop_handlers_by_hash(const u64 hash) return NULL; } -static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, - const char *name, const char *value, size_t value_len, - int flags) +int btrfs_set_prop(struct inode *inode, const char *name, const char *value, + size_t value_len, int flags) { + struct btrfs_root *root = BTRFS_I(inode)->root; const struct prop_handler *handler; + struct btrfs_trans_handle *trans; int ret; if (btrfs_root_readonly(BTRFS_I(inode)->root)) @@ -74,50 +75,40 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, return -EINVAL; if (value_len == 0) { + /* Its called to reset the property */ + trans = btrfs_start_transaction(root, 2); + if (IS_ERR(trans)) + return PTR_ERR(trans); + ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0, flags); - if (ret) - return ret; - - ret = handler->apply(inode, NULL, 0); - ASSERT(ret == 0); - - return ret; + if (!ret) { + ret = handler->apply(inode, NULL, 0); + ASSERT(ret == 0); + } + goto out; } ret = handler->validate(value, value_len); if (ret) return ret; + + trans = btrfs_start_transaction(root, 2); + if (IS_ERR(trans)) + return PTR_ERR(trans); + ret = btrfs_setxattr(trans, inode, handler->xattr_name, value, value_len, flags); if (ret) - return ret; + goto out; + ret = handler->apply(inode, value, value_len); if (ret) { + /* Apply failed. Reset the property. */ btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0, flags); - return ret; - } - - set_bit(BTRFS_INODE_HAS_PROPS, &BTRFS_I(inode)->runtime_flags); - - return 0; -} - -int btrfs_set_prop_trans(struct inode *inode, const char *name, - const char *value, size_t value_len, int flags) -{ - struct btrfs_root *root = BTRFS_I(inode)->root; - struct btrfs_trans_handle *trans; - int ret; - - trans = btrfs_start_transaction(root, 2); - if (IS_ERR(trans)) - return PTR_ERR(trans); - - ret = btrfs_set_prop(trans, inode, name, value, value_len, flags); - - if (!ret) { + } else { + set_bit(BTRFS_INODE_HAS_PROPS, &BTRFS_I(inode)->runtime_flags); inode_inc_iversion(inode); inode->i_ctime = current_time(inode); set_bit(BTRFS_INODE_COPY_EVERYTHING, @@ -125,6 +116,8 @@ int btrfs_set_prop_trans(struct inode *inode, const char *name, ret = btrfs_update_inode(trans, root, inode); BUG_ON(ret); } + +out: btrfs_end_transaction(trans); return ret; } diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h index b1a6b233b774..72bca072f9ba 100644 --- a/fs/btrfs/props.h +++ b/fs/btrfs/props.h @@ -10,8 +10,8 @@ void __init btrfs_props_init(void); -int btrfs_set_prop_trans(struct inode *inode, const char *name, - const char *value, size_t value_len, int flags); +int btrfs_set_prop(struct inode *inode, const char *name, + const char *value, size_t value_len, int flags); int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path); diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c index 58579a4d2f22..abc150a56f8b 100644 --- a/fs/btrfs/xattr.c +++ b/fs/btrfs/xattr.c @@ -371,7 +371,7 @@ static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler, size_t size, int flags) { name = xattr_full_name(handler, name); - return btrfs_set_prop_trans(inode, name, value, size, flags); + return btrfs_set_prop(inode, name, value, size, flags); } static const struct xattr_handler btrfs_security_xattr_handler = {
When the property fails to pass the prop_handlers::validate() check, the thread should exit with no changes in the kernel, but as we are starting the transaction too early, we have just updated the generation even if there is no change. For example: btrfs prop get /btrfs compression compression=lzo sync btrfs in dump-super /dev/sdb | grep "^generation" generation 32 Try to set an incomplete compression type btrfs prop set /btrfs compression zli ERROR: failed to set compression for /btrfs: Invalid argument sync Set failed but generation is incremented btrfs in dump-super /dev/sdb | grep "^generation" generation 33 <-- Fix it by collapsing btrfs_set_prop_trans() into btrfs_set_prop(), which provides flexibility to start the transaction after the prop_handlers::validate() has been checked. As of now if the prop_handlers::apply() fails then we still increment the generation, but if there is strong prop_handlers::validate() then the prop_handlers::apply() can never fail. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/ioctl.c | 10 ++++------ fs/btrfs/props.c | 59 +++++++++++++++++++++++++------------------------------- fs/btrfs/props.h | 4 ++-- fs/btrfs/xattr.c | 2 +- 4 files changed, 33 insertions(+), 42 deletions(-)