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 filesystem_limit enforcement #8228

Closed
wants to merge 1 commit into from

Conversation

behlendorf
Copy link
Contributor

@behlendorf behlendorf commented Dec 27, 2018

Motivation and Context

Issue #8226. Observed 'filesystem_limit' behavior differs from that
described in the man page and code.

TODO: Investigate adding test cases for the filesystem_limit feature.

Description

As described in the zfs(8) man page the 'filesystem_limit' does not
apply to users who are allowed to change the property.

This was incorrectly failing because unlike all other operations
the credential was being checked again outside the user context in
the syncing context. Checking the stored user credential in a
different context will always fail on Linux. See the comment above
priv_policy_ns() for details.

The proposed solution is to convert the CRED() to a kcred after the
secpolicy_zfs() checks succeed in the calling context. This allows
the check to be correctly performed a second time in the syncing
context.

To my surprise test cases were apparently never written for this
feature. If there had been some this may have been caught during
the initial port, and we should look in to adding some. For the
moment this change was manually tested.

How Has This Been Tested?

Locally built and manually tested the failure scenario.

[behlendo@centos7]$ sudo zfs create -o filesystem_limit=3 tank/limited
[behlendo@centos7]$ sudo zfs allow -u behlendo create,mount tank/limited
[behlendo@centos7]$ zfs create tank/limited/fs1
[behlendo@centos7]$ zfs create tank/limited/fs2
[behlendo@centos7]$ zfs create tank/limited/fs3
[behlendo@centos7]$ zfs create tank/limited/fs4
cannot create 'tank/limited/fs4': out of space
[behlendo@centos7]$ sudo zfs create tank/limited/fs4
[behlendo@centos7]$ sudo zfs create tank/limited/fs5

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)
  • Documentation (a change to man pages or other documentation)

Checklist:

As described in the zfs(8) man page the 'filesystem_limit' does not
apply to users who are allowed to change the property.

This was incorrectly failing because unlike all other operations
the credential was being checked again outside the user context in
the syncing context.   Checking the stored user credential in a
different context will always fail on Linux.  See the comment above
priv_policy_ns() for details.

The proposed solution is to convert the CRED() to a kcred after the
secpolicy_zfs() checks succeed in the calling context.  This allows
the check to be correctly performed a second time in the syncing
context.

To my surprise test cases were apparently never written for this
features.  If there had been some this may have been caught during
the initial port, and we some look in to adding some.  For the
moment this change was manually tested.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
@behlendorf behlendorf added the Status: Work in Progress Not yet ready for general review label Dec 27, 2018
@loli10K
Copy link
Contributor

loli10K commented Dec 27, 2018

+1 for adding tests, i was investigating #8226 and found another issue with "filesystem_limit" which i am still debugging but was ready to test in a new "group":

diff --git a/tests/runfiles/linux.run b/tests/runfiles/linux.run
index f33a91649..c05225f18 100644
--- a/tests/runfiles/linux.run
+++ b/tests/runfiles/linux.run
@@ -514,6 +514,11 @@ tags = ['functional', 'cp_files']
 tests = ['ctime_001_pos' ]
 tags = ['functional', 'ctime']
 
+[tests/functional/dataset_limits]
+tests = ['filesystem_count', 'filesystem_limit', 'snapshot_count',
+    'snapshot_limit']
+tags = ['functional', 'limits']
+
 [tests/functional/deadman]
 tests = ['deadman_sync', 'deadman_zio']
 pre =

@codecov
Copy link

codecov bot commented Dec 27, 2018

Codecov Report

Merging #8228 into master will decrease coverage by 0.05%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8228      +/-   ##
==========================================
- Coverage   78.56%   78.51%   -0.06%     
==========================================
  Files         379      379              
  Lines      114909   114904       -5     
==========================================
- Hits        90284    90215      -69     
- Misses      24625    24689      +64
Flag Coverage Δ
#kernel 79.02% <87.5%> (+0.05%) ⬆️
#user 67.45% <75%> (-0.04%) ⬇️

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 06f3fc2...3e5b342. Read the comment docs.

@behlendorf
Copy link
Contributor Author

@loli10K since it sounds like you've been looking in to this, and have already written some tests would you mind taking over this PR too. Based on the test runs we had two unexpected failure which need to explained, and we might want to take a slightly more targeted approach than the one suggested here.

    FAIL delegate/zfs_allow_005_pos (expected PASS)
    FAIL delegate/zfs_unallow_005_pos (expected PASS)

@loli10K
Copy link
Contributor

loli10K commented Dec 27, 2018

@behlendorf i am probably a long way from being able to properly fix #8226, the "allow" code is somewhat new to me. I don't mind taking a stab at this though, seems like a good opportunity to study more ZFS code.

The other issue i found is very similar to https://www.illumos.org/issues/6254 and hopefully i should be able to have a working fix this upcoming weekend.

@loli10K loli10K self-assigned this Dec 30, 2018
@loli10K loli10K mentioned this pull request Jan 13, 2019
12 tasks
@loli10K
Copy link
Contributor

loli10K commented Jan 13, 2019

Alternative version is #8280

@behlendorf behlendorf closed this Jan 14, 2019
ghost pushed a commit to zfsonfreebsd/ZoF that referenced this pull request Feb 29, 2020
See issue openzfs#8226: Property filesystem_limit does not work as documented

There have been previous attempts to fix the behavior on Linux, but so
far the issue is still open.  See PRs openzfs#8228, openzfs#8280.

The existing tests pass for the incorrect behavior.  This is a problem
on FreeBSD; we are failing the tests because we implement the feature
correctly.

I have adapted the tests based on the work by @loli10K in openzfs#8280 and
extended the changes to fix the snapshot_limit test as well.

Linux now fails these tests, so entries linking to the issue have been
added to the "maybe" group in zts-report.py.

Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
@ghost ghost mentioned this pull request Feb 29, 2020
12 tasks
ghost pushed a commit to zfsonfreebsd/ZoF that referenced this pull request Feb 29, 2020
See issue openzfs#8226: Property filesystem_limit does not work as documented

There have been previous attempts to fix the behavior on Linux, but so
far the issue is still open.  See PRs openzfs#8228, openzfs#8280.

The existing tests pass for the incorrect behavior.  This is a problem
on FreeBSD; we are failing the tests because we implement the feature
correctly.

I have adapted the tests based on the work by @loli10K in openzfs#8280 and
extended the changes to fix the snapshot_limit test as well.

Linux now fails these tests, so entries linking to the issue have been
added to the "maybe" group in zts-report.py.

Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
behlendorf added a commit that referenced this pull request Mar 4, 2020
See issue #8226: Property filesystem_limit does not work as documented

There have been previous attempts to fix the behavior on Linux, but so
far the issue is still open.  See PRs #8228, #8280.

The existing tests pass for the incorrect behavior.  This is a problem
on FreeBSD; we are failing the tests because we implement the feature
correctly.

I have adapted the tests based on the work by @loli10K in #8280 and
extended the changes to fix the snapshot_limit test as well.

Linux now fails these tests, so entries linking to the issue have been
added to the "maybe" group in zts-report.py.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes #10082
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
See issue openzfs#8226: Property filesystem_limit does not work as documented

There have been previous attempts to fix the behavior on Linux, but so
far the issue is still open.  See PRs openzfs#8228, openzfs#8280.

The existing tests pass for the incorrect behavior.  This is a problem
on FreeBSD; we are failing the tests because we implement the feature
correctly.

I have adapted the tests based on the work by @loli10K in openzfs#8280 and
extended the changes to fix the snapshot_limit test as well.

Linux now fails these tests, so entries linking to the issue have been
added to the "maybe" group in zts-report.py.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ryan Moeller <ryan@iXsystems.com>
Closes openzfs#10082
@behlendorf behlendorf deleted the issue-8226 branch April 19, 2021 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Work in Progress Not yet ready for general review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants