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

Property filesystem_limit does not work as documented #8226

Closed
spaghettopazzia opened this issue Dec 26, 2018 · 3 comments · Fixed by #10545
Closed

Property filesystem_limit does not work as documented #8226

spaghettopazzia opened this issue Dec 26, 2018 · 3 comments · Fixed by #10545
Labels
Type: Defect Incorrect behavior (e.g. crash, hang)

Comments

@spaghettopazzia
Copy link

System information

Type Version/Name
Distribution Name Debian
Distribution Version Jessie
Linux Kernel 3.16.0-4-amd64
Architecture amd64
ZFS Version 0.8.0-rc2
SPL Version 0.8.0-rc2

Describe the problem you're observing

According to the zfs manual filesystem_limit does not apply to users allowed to change the property:

filesystem_limit=count|none
Limits the number of filesystems and volumes that can exist under
this point in the dataset tree. The limit is not enforced if the user is allowed to change the limit. Setting a filesystem_limit to on a descendent of a filesystem that already has a filesystem_limit does not override the ancestor's filesystem_limit, but rather imposes an additional limit. This feature must be enabled to be used (see zpool-features(5)).

I find this information to be wrong.

Describe how to reproduce the problem

root@nymeria:~# zfs create -o filesystem_limit=3 rpool/limited
root@nymeria:~# zfs create rpool/limited/fs1
root@nymeria:~# zfs create rpool/limited/fs2
root@nymeria:~# zfs create rpool/limited/fs3
root@nymeria:~# zfs create rpool/limited/fs4
cannot create 'rpool/limited/fs4': out of space
root@nymeria:~# zfs set filesystem_limit=4 rpool/limited
root@nymeria:~# zfs create rpool/limited/fs4
root@nymeria:~# zfs create rpool/limited/fs5
cannot create 'rpool/limited/fs5': out of space

Include any warning/errors/backtraces from the system logs

None

@devZer0
Copy link

devZer0 commented Sep 17, 2019

for me it looks like it was fixed !?

close?

@loli10K
Copy link
Contributor

loli10K commented Sep 22, 2019

This is not fixed:

root@linux:~# cat /sys/module/zfs/version
0.8.0-282_gafc8f0a6f
root@linux:~# POOLNAME='testpool'
root@linux:~# TMPDIR='/var/tmp'
root@linux:~# mountpoint -q $TMPDIR || mount -t tmpfs tmpfs $TMPDIR
root@linux:~# zpool destroy -f $POOLNAME
root@linux:~# rm -f $TMPDIR/zpool.dat
root@linux:~# truncate -s 128m $TMPDIR/zpool.dat
root@linux:~# zpool create -O mountpoint=none $POOLNAME $TMPDIR/zpool.dat
root@linux:~# zfs create -o filesystem_limit=3 $POOLNAME/limited
root@linux:~# zfs create $POOLNAME/limited/fs1
root@linux:~# zfs create $POOLNAME/limited/fs2
root@linux:~# zfs create $POOLNAME/limited/fs3
root@linux:~# zfs create $POOLNAME/limited/fs4
cannot create 'testpool/limited/fs4': out of space
root@linux:~# zfs set filesystem_limit=4 $POOLNAME/limited
root@linux:~# zfs create $POOLNAME/limited/fs5
root@linux:~# zfs create $POOLNAME/limited/fs6
cannot create 'testpool/limited/fs6': out of space

@behlendorf behlendorf added the Type: Defect Incorrect behavior (e.g. crash, hang) label Sep 25, 2019
@ghost
Copy link

ghost commented Feb 28, 2020

The test for this is also wrong.

ghost pushed a commit to zfsonfreebsd/ZoF that referenced this issue 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 pushed a commit to zfsonfreebsd/ZoF that referenced this issue 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 issue 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
ahrens added a commit to ahrens/zfs that referenced this issue Jul 8, 2020
The filesystem_limit and snapshot_limit properties limit the number of
filesystems or snapshots that can be created below this dataset.
According to the manpage, "The limit is not enforced if the user is
allowed to change the limit."  Two types of users are allowed to change
the limit:

1. Those that have been delegated the `filesystem_limit` or
`snapshot_limit` permission, e.g. with
`zfs allow USER filesystem_limit DATASET`.  This works properly.

2. A user with elevated system priviliges (e.g. root).  This does not
work - the root user will incorrectly get an error when trying to create
a snapshot/filesystem, if it exceeds the `_limit` property.

The problem is that `priv_policy_ns()` does not work if the `cred_t` is
not that of the current process.  This happens when
`dsl_enforce_ds_ss_limits()` is called in syncing context (as part of a
sync task's check func) to determine the permissions of the
corresponding user process.

This commit fixes the issue by passing the `task_struct` (typedef'ed as
a `proc_t`) to syncing context, and then using `has_capability()` to
determine if that process is privileged.  Note that we still need to
pass the `cred_t` to syncing context so that we can check if the user
was delegated this permission with `zfs allow`.

This problem only impacts Linux.  Wrappers are added to FreeBSD but it
continues to use `priv_check_cred()`, which works on arbitrary `cred_t`.

Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
Closes: openzfs#8226
behlendorf pushed a commit that referenced this issue Jul 12, 2020
The filesystem_limit and snapshot_limit properties limit the number of
filesystems or snapshots that can be created below this dataset.
According to the manpage, "The limit is not enforced if the user is
allowed to change the limit."  Two types of users are allowed to change
the limit:

1. Those that have been delegated the `filesystem_limit` or
`snapshot_limit` permission, e.g. with
`zfs allow USER filesystem_limit DATASET`.  This works properly.

2. A user with elevated system privileges (e.g. root).  This does not
work - the root user will incorrectly get an error when trying to create
a snapshot/filesystem, if it exceeds the `_limit` property.

The problem is that `priv_policy_ns()` does not work if the `cred_t` is
not that of the current process.  This happens when
`dsl_enforce_ds_ss_limits()` is called in syncing context (as part of a
sync task's check func) to determine the permissions of the
corresponding user process.

This commit fixes the issue by passing the `task_struct` (typedef'ed as
a `proc_t`) to syncing context, and then using `has_capability()` to
determine if that process is privileged.  Note that we still need to
pass the `cred_t` to syncing context so that we can check if the user
was delegated this permission with `zfs allow`.

This problem only impacts Linux.  Wrappers are added to FreeBSD but it
continues to use `priv_check_cred()`, which works on arbitrary `cred_t`.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
Closes #8226
Closes #10545
jsai20 pushed a commit to jsai20/zfs that referenced this issue 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
jsai20 pushed a commit to jsai20/zfs that referenced this issue Mar 30, 2021
The filesystem_limit and snapshot_limit properties limit the number of
filesystems or snapshots that can be created below this dataset.
According to the manpage, "The limit is not enforced if the user is
allowed to change the limit."  Two types of users are allowed to change
the limit:

1. Those that have been delegated the `filesystem_limit` or
`snapshot_limit` permission, e.g. with
`zfs allow USER filesystem_limit DATASET`.  This works properly.

2. A user with elevated system privileges (e.g. root).  This does not
work - the root user will incorrectly get an error when trying to create
a snapshot/filesystem, if it exceeds the `_limit` property.

The problem is that `priv_policy_ns()` does not work if the `cred_t` is
not that of the current process.  This happens when
`dsl_enforce_ds_ss_limits()` is called in syncing context (as part of a
sync task's check func) to determine the permissions of the
corresponding user process.

This commit fixes the issue by passing the `task_struct` (typedef'ed as
a `proc_t`) to syncing context, and then using `has_capability()` to
determine if that process is privileged.  Note that we still need to
pass the `cred_t` to syncing context so that we can check if the user
was delegated this permission with `zfs allow`.

This problem only impacts Linux.  Wrappers are added to FreeBSD but it
continues to use `priv_check_cred()`, which works on arbitrary `cred_t`.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
Closes openzfs#8226
Closes openzfs#10545
sempervictus pushed a commit to sempervictus/zfs that referenced this issue May 31, 2021
The filesystem_limit and snapshot_limit properties limit the number of
filesystems or snapshots that can be created below this dataset.
According to the manpage, "The limit is not enforced if the user is
allowed to change the limit."  Two types of users are allowed to change
the limit:

1. Those that have been delegated the `filesystem_limit` or
`snapshot_limit` permission, e.g. with
`zfs allow USER filesystem_limit DATASET`.  This works properly.

2. A user with elevated system privileges (e.g. root).  This does not
work - the root user will incorrectly get an error when trying to create
a snapshot/filesystem, if it exceeds the `_limit` property.

The problem is that `priv_policy_ns()` does not work if the `cred_t` is
not that of the current process.  This happens when
`dsl_enforce_ds_ss_limits()` is called in syncing context (as part of a
sync task's check func) to determine the permissions of the
corresponding user process.

This commit fixes the issue by passing the `task_struct` (typedef'ed as
a `proc_t`) to syncing context, and then using `has_capability()` to
determine if that process is privileged.  Note that we still need to
pass the `cred_t` to syncing context so that we can check if the user
was delegated this permission with `zfs allow`.

This problem only impacts Linux.  Wrappers are added to FreeBSD but it
continues to use `priv_check_cred()`, which works on arbitrary `cred_t`.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
Closes openzfs#8226
Closes openzfs#10545
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Defect Incorrect behavior (e.g. crash, hang)
Projects
None yet
4 participants