Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix i_flags issue caused by 64c688d #5486

Merged
merged 5 commits into from
Dec 19, 2016
Merged

Fix i_flags issue caused by 64c688d #5486

merged 5 commits into from
Dec 19, 2016

Conversation

tuxoko
Copy link
Contributor

@tuxoko tuxoko commented Dec 14, 2016

Description

Fix zfs_xvattr_set to set S_IMMUTABLE and S_APPEND flags correctly.

Reinstate zfs_set_inode_flags and use it when zfs_xvatter_set and also when
setting up inode in zfs_znode_alloc and zfs_rezget.

Fix wrong operator in xvattr.h

Motivation and Context

#5470
#5469

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • Change has been approved by a ZFS on Linux member.

Chunwei Chen added 2 commits December 14, 2016 14:48
Fix zfs_xvattr_set to set S_IMMUTABLE and S_APPEND flags correctly.

Reinstate zfs_set_inode_flags and use it when zfs_xvatter_set and also when
setting up inode in zfs_znode_alloc and zfs_rezget.

Signed-off-by: Chunwei Chen <[email protected]>
Signed-off-by: Chunwei Chen <[email protected]>
@mention-bot
Copy link

@tuxoko, thanks for your PR! By analyzing the history of the files in this pull request, we identified @lorddoskias, @behlendorf and @dweeezil to be potential reviewers.

@tuxoko tuxoko force-pushed the xvap branch 2 times, most recently from c7a64c5 to 05100ec Compare December 15, 2016 19:47
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor

@dweeezil dweeezil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Presumably the use of inode_set_flags() can be addressed separately.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, while this fixes the existing issue it appears things still aren't quite right. Users without CAP_LINUX_IMMUTABLE were able to clear immutable and append. @tuxoko behlendorf/zfs@c1f4a1b is a proposed fix for the issue, can you apply it to the top of this stack for review.

$ touch testfile
$ sudo chattr +i testfile 
$ chattr -i testfile # Should fail

TODO

  • Add a basic test cases for immutable and append
  • Use inode_set_flags()

@behlendorf
Copy link
Contributor

@dweeezil we could definitely do it in a separate PR but it sure would be nice to handle it right now as well as add a few test cases.

The fchange in zpl_ioctl_setflags was for detecting flag change. However it
was incorrect and would always fail to detect a flag change from set to unset,
causing users without CAP_LINUX_IMMUTABLE to be able to unset flags.

Signed-off-by: Chunwei Chen <[email protected]>
@tuxoko
Copy link
Contributor Author

tuxoko commented Dec 16, 2016

@behlendorf
That is not a regression, it's a bug introduced with the chattr support...

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm fine with the alternate fix.

@tuxoko
Copy link
Contributor Author

tuxoko commented Dec 16, 2016

Is zfstest file run with root? If so how do I drop privilege?

@behlendorf
Copy link
Contributor

@tuxoko you can use the user_run function to drop privilege. The userquota/userspace_001_pos.ksh is a pretty good example. You'll need to make sure to add/remove the test user in setup.ksh/cleanup.ksh respectively.

Chunwei Chen added 2 commits December 16, 2016 13:54
Signed-off-by: Chunwei Chen <[email protected]>
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @tuxoko thank you for adding the configure check and test cases. I'll get this merged.

@behlendorf behlendorf merged commit a3823f4 into openzfs:master Dec 19, 2016
@tuxoko tuxoko deleted the xvap branch March 3, 2017 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants