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 <nbori...@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 <dste...@suse.com>
