| Message ID | 20190321155706.16686-1-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] btrfs: Defer setting new inode mode until after do_set_acl succeeds | expand |
On 21/03/2019 16:57, Nikolay Borisov wrote: [...] > * Fixed logic of when i_mode is set, v1 was causing an uninitialised mode > to be assigned to inode->i_mode resulting in fs consistency problems. Fix this > by introducing a variable which is set to true when the i_mode needs to be > changed. I know this variable could be eliminated by simply initialising > mode = inode->i_mode and always assigning it in the if (!ret) branch but I > find this somewhat subtle and rather be explicit with the boolean variable. [...] > + if (!ret) { > + if (change_mode) > + inode->i_mode = mode; But what about initializing mode by inode->i_mode *and* adding a comment that this is either the saved state or the new one, depending on posix_acl_update_mode()'s outcome?
On Thu, Mar 21, 2019 at 05:57:06PM +0200, Nikolay Borisov wrote: > Currently a reference to inode->i_mode is passed directly to > posix_acl_update_mode when setting an ACL which results in the inode's > mode always being changed. In case of errors (e.g. in do_set_acl or > even starting a transaction) the old mode needs to be re-assigned to > ->i_mode. This mode recovery is done only in case do_set_acl fails, > which leads to buggy behavior in case btrfs_start_transaction fails. > > Fix it by simply setting the new mode to a temporary variable which is > assigned to inode->i_mode's only when do_set_acl succeeds. This covers > both failure cases explained above. > > Fixes: db0f220e98eb ("btrfs: start transaction in btrfs_set_acl") > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- > > Changes since v1: > * Eliminated unrealted newline change (Johannes) > * Fixed logic of when i_mode is set, v1 was causing an uninitialised mode > to be assigned to inode->i_mode resulting in fs consistency problems. Fix this > by introducing a variable which is set to true when the i_mode needs to be > changed. I know this variable could be eliminated by simply initialising > mode = inode->i_mode and always assigning it in the if (!ret) branch but I > find this somewhat subtle and rather be explicit with the boolean variable. Agreed. I folded the missing mode reset to the patch that introduced it as the Fixes: reference would not work, and adjusted your patch to simplify it again. Changelog adapted accordingly, only that it's not a faix but simplification. Please check it once it's pushed to misc-next. Reviewed-by: David Sterba <dsterba@suse.com>
diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c index b722866e1442..ac12a4530540 100644 --- a/fs/btrfs/acl.c +++ b/fs/btrfs/acl.c @@ -112,14 +112,16 @@ static int do_set_acl(struct btrfs_trans_handle *trans, struct inode *inode, int btrfs_set_acl(struct inode *inode, struct posix_acl *acl, int type) { int ret; - umode_t old_mode = inode->i_mode; + umode_t mode; + bool change_mode = false; struct btrfs_trans_handle *trans; struct btrfs_root *root = BTRFS_I(inode)->root; if (type == ACL_TYPE_ACCESS && acl) { - ret = posix_acl_update_mode(inode, &inode->i_mode, &acl); + ret = posix_acl_update_mode(inode, &mode, &acl); if (ret) return ret; + change_mode = true; } trans = btrfs_start_transaction(root, 2); @@ -127,9 +129,9 @@ int btrfs_set_acl(struct inode *inode, struct posix_acl *acl, int type) return PTR_ERR(trans); ret = do_set_acl(trans, inode, acl, type); - if (ret) { - inode->i_mode = old_mode; - } else { + if (!ret) { + if (change_mode) + inode->i_mode = mode; inode_inc_iversion(inode); inode->i_ctime = current_time(inode); set_bit(BTRFS_INODE_COPY_EVERYTHING, &BTRFS_I(inode)->runtime_flags);
Currently a reference to inode->i_mode is passed directly to posix_acl_update_mode when setting an ACL which results in the inode's mode always being changed. In case of errors (e.g. in do_set_acl or even starting a transaction) the old mode needs to be re-assigned to ->i_mode. This mode recovery is done only in case do_set_acl fails, which leads to buggy behavior in case btrfs_start_transaction fails. Fix it by simply setting the new mode to a temporary variable which is assigned to inode->i_mode's only when do_set_acl succeeds. This covers both failure cases explained above. Fixes: db0f220e98eb ("btrfs: start transaction in btrfs_set_acl") Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- Changes since v1: * Eliminated unrealted newline change (Johannes) * Fixed logic of when i_mode is set, v1 was causing an uninitialised mode to be assigned to inode->i_mode resulting in fs consistency problems. Fix this by introducing a variable which is set to true when the i_mode needs to be changed. I know this variable could be eliminated by simply initialising mode = inode->i_mode and always assigning it in the if (!ret) branch but I find this somewhat subtle and rather be explicit with the boolean variable. fs/btrfs/acl.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)