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

zfs_ioc_snapshot: check user-prop permissions on snapshotted datasets #9180

Merged
merged 1 commit into from
Aug 27, 2019

Conversation

avg-I
Copy link
Contributor

@avg-I avg-I commented Aug 19, 2019

Previously, the permissions were checked on the pool which was obviously
incorrect.

After this change, zfs_check_userprops() only validates the properties
without any permission checks. The permissions are checked individually
for each snapshotted dataset.

This is a bug fix for #9179.

Signed-off-by: Andriy Gapon avg@FreeBSD.org

@avg-I
Copy link
Contributor Author

avg-I commented Aug 19, 2019

I think that ideally the permission should be checked on newly created snapshots, but I am not sure how to arrange that. Maybe the check should be pushed down to dsl_dataset_snapshot_check.

@ahrens ahrens added the Status: Code Review Needed Ready for review and testing label Aug 20, 2019
@ahrens
Copy link
Member

ahrens commented Aug 20, 2019

ideally the permission should be checked on newly created snapshots

Do you mean we should only check for permission to set the user property if the snapshot is actually created (as opposed to failing to be created with an error)? I don't think that's needed, especially since the snapshot creation is all-or-nothing: if one snapshot can't be created, the whole ioctl fails and no snapshots are created.

In any case, the PR looks good to me. At least aside from the build error:

/var/lib/buildbot/slaves/zfs/Ubuntu_18_04_x86_64__BUILD_/build/zfs/module/zfs/zfs_ioctl.c: In function ‘zfs_check_userprops’:
/var/lib/buildbot/slaves/zfs/Ubuntu_18_04_x86_64__BUILD_/build/zfs/module/zfs/zfs_ioctl.c:2745:6: error: unused variable ‘error’ [-Werror=unused-variable]
  int error = 0;
      ^~~~~

@avg-I
Copy link
Contributor Author

avg-I commented Aug 21, 2019

Matt, thank you for the comment. I will fix the error ASAP.
Regarding the checks... I was thinking that the userprop permission / delegation should be somehow checked on a snapshot, yet to be created, not on its head dataset. That is, I hypothesize that there could be a configuration where setting user properties is not allowed for the head dataset, but it is allowed for its snapshots (or vice versa). But I am not sure if such configuration is actually possible. Maybe I am overthinking this?

@codecov
Copy link

codecov bot commented Aug 21, 2019

Codecov Report

Merging #9180 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9180      +/-   ##
==========================================
+ Coverage   79.19%   79.27%   +0.07%     
==========================================
  Files         400      400              
  Lines      122002   122006       +4     
==========================================
+ Hits        96625    96723      +98     
+ Misses      25377    25283      -94
Flag Coverage Δ
#kernel 79.79% <100%> (-0.01%) ⬇️
#user 67.06% <ø> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4302698...864514e. Read the comment docs.

@ahrens
Copy link
Member

ahrens commented Aug 21, 2019

@avg-I Delegated permissions work on a per-filesystem level. Snapshots can't have their own permissions, they can only inherit from their containing filesystem. So the scenerio you outlined would be possible if there were separate permissions for "set user property on this filesystem" and "set user property on any snapshot of this filesystem", but there could not be a permission of "set user property on this snapshot (but not other snapshots of this filesystem)". At least not with the current architecture. So checking on the filesystem would still be sufficient.

@avg-I
Copy link
Contributor Author

avg-I commented Aug 21, 2019

Matt, thank you for the explanation.

@avg-I
Copy link
Contributor Author

avg-I commented Aug 22, 2019

I think that the latest test failures are not related to this change.
At least, I cannot see a connection.

Tests with results other than PASS that are unexpected:
    FAIL history/history_002_pos (expected PASS)
    FAIL vdev_zaps/vdev_zaps_005_pos (expected PASS)

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.

This looks right to me. It seems you get particularly unlucky with the CI and ran in to several known issue. I think it would be worthwhile to rebase this on today's master branch and send it through again. The CentOS 6 and Fedora 30 failures at least have been resolved.

Previously, the permissions were checked on the pool which was obviously
incorrect.

After this change, zfs_check_userprops() only validates the properties
without any permission checks.  The permissions are checked individually
for each snapshotted dataset.

Signed-off-by: Andriy Gapon <avg@FreeBSD.org>
@avg-I avg-I force-pushed the delegated-zfs-snapshot--o branch from a14f896 to 864514e Compare August 23, 2019 06:20
@behlendorf behlendorf requested a review from ahrens August 26, 2019 20:21
@behlendorf
Copy link
Contributor

The CI failures observed are all for known existing issue. I've resubmitted the failing runs for good measure.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Aug 26, 2019
@behlendorf behlendorf merged commit e6203d2 into openzfs:master Aug 27, 2019
@avg-I avg-I deleted the delegated-zfs-snapshot--o branch September 4, 2019 07:36
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 17, 2019
Previously, the permissions were checked on the pool which was obviously
incorrect.

After this change, zfs_check_userprops() only validates the properties
without any permission checks.  The permissions are checked individually
for each snapshotted dataset.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Matt Ahrens <mahrens@delphix.com>
Signed-off-by: Andriy Gapon <avg@FreeBSD.org>
Closes openzfs#9179 
Closes openzfs#9180
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 18, 2019
Previously, the permissions were checked on the pool which was obviously
incorrect.

After this change, zfs_check_userprops() only validates the properties
without any permission checks.  The permissions are checked individually
for each snapshotted dataset.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Matt Ahrens <mahrens@delphix.com>
Signed-off-by: Andriy Gapon <avg@FreeBSD.org>
Closes openzfs#9179 
Closes openzfs#9180
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 18, 2019
Previously, the permissions were checked on the pool which was obviously
incorrect.

After this change, zfs_check_userprops() only validates the properties
without any permission checks.  The permissions are checked individually
for each snapshotted dataset.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Matt Ahrens <mahrens@delphix.com>
Signed-off-by: Andriy Gapon <avg@FreeBSD.org>
Closes openzfs#9179 
Closes openzfs#9180
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 18, 2019
Previously, the permissions were checked on the pool which was obviously
incorrect.

After this change, zfs_check_userprops() only validates the properties
without any permission checks.  The permissions are checked individually
for each snapshotted dataset.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Matt Ahrens <mahrens@delphix.com>
Signed-off-by: Andriy Gapon <avg@FreeBSD.org>
Closes openzfs#9179 
Closes openzfs#9180
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 19, 2019
Previously, the permissions were checked on the pool which was obviously
incorrect.

After this change, zfs_check_userprops() only validates the properties
without any permission checks.  The permissions are checked individually
for each snapshotted dataset.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Matt Ahrens <mahrens@delphix.com>
Signed-off-by: Andriy Gapon <avg@FreeBSD.org>
Closes openzfs#9179 
Closes openzfs#9180
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 23, 2019
Previously, the permissions were checked on the pool which was obviously
incorrect.

After this change, zfs_check_userprops() only validates the properties
without any permission checks.  The permissions are checked individually
for each snapshotted dataset.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Matt Ahrens <mahrens@delphix.com>
Signed-off-by: Andriy Gapon <avg@FreeBSD.org>
Closes openzfs#9179 
Closes openzfs#9180
tonyhutter pushed a commit that referenced this pull request Sep 26, 2019
Previously, the permissions were checked on the pool which was obviously
incorrect.

After this change, zfs_check_userprops() only validates the properties
without any permission checks.  The permissions are checked individually
for each snapshotted dataset.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Matt Ahrens <mahrens@delphix.com>
Signed-off-by: Andriy Gapon <avg@FreeBSD.org>
Closes #9179
Closes #9180
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants