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

user namespace fix & test for #6800 #7270

Merged
merged 1 commit into from
Mar 7, 2018
Merged

Conversation

Blub
Copy link
Contributor

@Blub Blub commented Mar 5, 2018

This is the first part of the previous PR #6865 with only the patches relevant to fix #6800 separated out.

It may be reasonable to squash the first two commits, but for reviewing purposes I left them separate for now.
The first patch also adds HAVE_CRED_USER_NS which I ended up not using (see the second patch's comment about HAVE_KUID_HAS_MAPPING). Once the rest of the PR is good enough to merge I can remove that.

PS: Sorry for taking so long...

@Blub
Copy link
Contributor Author

Blub commented Mar 5, 2018

Looks like I forgot to add user_namespace_common.kshlib to dist_pkgdata_SCRIPTS in the user namespace test's Makefile.am.

@Blub Blub force-pushed the userns-part-1 branch 2 times, most recently from f718daa to 84b1956 Compare March 5, 2018 20:45
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.

@Blub thanks for splitting this in to a separate PR. I'll give it a good review tomorrow and do some manual testing to verify it. In the meanwhile the bots did determine that the new test case is failing on CentOS 7 (only) and that needs to be investigated.

When you do refresh this please go ahead and squash all three of these commits. You'll also need to rebase on master to resolve the Kernel.org build failure.

@codecov
Copy link

codecov bot commented Mar 6, 2018

Codecov Report

Merging #7270 into master will decrease coverage by 0.03%.
The diff coverage is 54.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7270      +/-   ##
==========================================
- Coverage   76.44%   76.41%   -0.04%     
==========================================
  Files         327      328       +1     
  Lines      103922   104006      +84     
==========================================
+ Hits        79447    79480      +33     
- Misses      24475    24526      +51
Flag Coverage Δ
#kernel 76.22% <58.82%> (+0.19%) ⬆️
#user 65.62% <53.33%> (-0.25%) ⬇️

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 639b189...021c942. Read the comment docs.

@Blub
Copy link
Contributor Author

Blub commented Mar 6, 2018

The failed test uses centos 7 which, while compiling user namespace support into the kernel (and therfore enabling CONFIG_USER_NS & friends) needs boot parameters to actually enable it. (see [1])

So the question is: should I try to detect this in the test suite or will you adapt the build bot?

[1] https://discuss.linuxcontainers.org/t/centos-7-kernel-514-693-cannot-start-any-nodes-after-update/641/16?u=brauner

@stgraber
Copy link

stgraber commented Mar 6, 2018

From https://discuss.linuxcontainers.org/t/lxd-on-centos-7/1250

The instructions to enable the right bits are:

grubby --args="user_namespace.enable=1" --update-kernel="$(grubby --default-kernel)"
grubby --args="namespace.unpriv_enable=1" --update-kernel="$(grubby --default-kernel)"
echo "user.max_user_namespaces=3883" > /etc/sysctl.d/99-userns.conf
reboot

After the reboot, the kernel will be properly configured for use of user namespaces.
This is only needed on Centos/RHEL kernels, all other distros behave as one would expect :)

@behlendorf
Copy link
Contributor

@Blub let me update the CentOS 7 bots. @stgraber thanks for the concise instructions!

@behlendorf
Copy link
Contributor

@Blub alright the CentOS 7 bots have been updated.

The only issue I ran in to was that the new test case fails when run in-tree due to the constrained path set by the ZFS Test Suite (the bots test with packages which is why they missed it). Using full paths for touch and chmod let's us avoid the issue, how about updating the test case like this.

Please go ahead and squash and rebase this on master then force update the PR.

diff --git a/tests/zfs-tests/tests/functional/user_namespace/user_namespace_001.ksh b/tests/zfs-tests/tests/functional/user_namespace/user_namespace_
index 3d95a1d..6be30ab 100755
--- a/tests/zfs-tests/tests/functional/user_namespace/user_namespace_001.ksh
+++ b/tests/zfs-tests/tests/functional/user_namespace/user_namespace_001.ksh
@@ -51,9 +51,12 @@ log_onexit cleanup
 
 log_assert "Check root in user namespaces"
 
+TOUCH=$(readlink -e $(which touch))
+CHMOD=$(readlink -e $(which chmod))
+
 for i in ${files[*]}; do
-       log_must touch $TESTDIR/$i
-       log_must chmod 0644 $TESTDIR/$i
+       log_must $TOUCH $TESTDIR/$i
+       log_must $CHMOD 0644 $TESTDIR/$i
 done
 
 log_must chown 0:0 $TESTDIR/rroot_rroot
@@ -62,25 +65,25 @@ log_must chown $ROOT_UID:$OTHER_UID $TESTDIR/uroot_other
 log_must chown $OTHER_UID:$ROOT_UID $TESTDIR/uother_uroot
 log_must chown $OTHER_UID:$OTHER_UID $TESTDIR/uother_uother
 
-log_mustnot user_ns_exec chmod 02755 $TESTDIR/rroot_rroot
+log_mustnot user_ns_exec $CHMOD 02755 $TESTDIR/rroot_rroot
 log_mustnot test -g $TESTDIR/rroot_rroot
 
-log_must user_ns_exec chmod 02755 $TESTDIR/uroot_uroot
+log_must user_ns_exec $CHMOD 02755 $TESTDIR/uroot_uroot
 log_must test -g $TESTDIR/uroot_uroot
 
-log_must user_ns_exec chmod 02755 $TESTDIR/uroot_other
+log_must user_ns_exec $CHMOD 02755 $TESTDIR/uroot_other
 log_must test -g $TESTDIR/uroot_other
 
-log_must user_ns_exec chmod 02755 $TESTDIR/uother_uroot
+log_must user_ns_exec $CHMOD 02755 $TESTDIR/uother_uroot
 log_must test -g $TESTDIR/uother_uroot
 
-log_must user_ns_exec chmod 02755 $TESTDIR/uother_uother
+log_must user_ns_exec $CHMOD 02755 $TESTDIR/uother_uother
 log_must test -g $TESTDIR/uother_uother
 
-log_mustnot user_ns_exec touch $TESTDIR/rroot_rroot
-log_must chmod 0666 $TESTDIR/rroot_rroot
+log_mustnot user_ns_exec $TOUCH $TESTDIR/rroot_rroot
+log_must $CHMOD 0666 $TESTDIR/rroot_rroot
 for i in ${files[*]}; do
-       log_must user_ns_exec touch $TESTDIR/$i
+       log_must user_ns_exec $TOUCH $TESTDIR/$i
 done
 
 log_pass "Check root in user namespaces"

Change file related checks to use user namespaces and make
sure involved uids/gids are mappable in the current
namespace.

Note that checks without file ownership information will
still not take user namespaces into account, as some of
these should be handled via 'zfs allow' (otherwise root in a
user namespace could issue commands such as `zpool export`).

This also adds an initial user namespace regression test
for the setgid bit loss, with a user_ns_exec helper usable
in further tests.

Additionally, configure checks for the required user
namespace related features are added for:
  * ns_capable
  * kuid/kgid_has_mapping()
  * user_ns in cred_t

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Closes openzfs#6800
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.

Looks good! Thanks!

@behlendorf behlendorf merged commit 0e85048 into openzfs:master Mar 7, 2018
@behlendorf
Copy link
Contributor

Merged and added to the list for the 0.7.7 release. Thanks!

tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Mar 12, 2018
Change file related checks to use user namespaces and make
sure involved uids/gids are mappable in the current
namespace.

Note that checks without file ownership information will
still not take user namespaces into account, as some of
these should be handled via 'zfs allow' (otherwise root in a
user namespace could issue commands such as `zpool export`).

This also adds an initial user namespace regression test
for the setgid bit loss, with a user_ns_exec helper usable
in further tests.

Additionally, configure checks for the required user
namespace related features are added for:
  * ns_capable
  * kuid/kgid_has_mapping()
  * user_ns in cred_t

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Closes openzfs#6800 
Closes openzfs#7270
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Mar 13, 2018
This is a squashed patchset for zfs-0.7.7.  The individual commits are
in the tonyhutter:zfs-0.7.7-hutter branch.  I squashed the commits so
that buildbot wouldn't have to run against each one, and because
github/builbot seem to have a maximum limit of 30 commits they can
test from a PR.

- Fix MMP write frequency for large pools openzfs#7205 openzfs#7289
- Handle zio_resume and mmp => off openzfs#7286
- Fix zfs-kmod builds when using rpm >= 4.14 openzfs#7284
- zdb and inuse tests don't pass with real disks openzfs#6939 openzfs#7261
- Take user namespaces into account in policy checks openzfs#6800 openzfs#7270
- Detect long config lock acquisition in mmp openzfs#7212
- Linux 4.16 compat: get_disk_and_module() openzfs#7264
- Change checksum & IO delay ratelimit values openzfs#7252
- Increment zil_itx_needcopy_bytes properly openzfs#6988 openzfs#7176
- Fix some typos openzfs#7237
- Fix zpool(8) list example to match actual format openzfs#7244
- Add SMART self-test results to zpool status -c openzfs#7178
- Add scrub after resilver zed script openzfs#4662 openzfs#7086
- Fix free memory calculation on v3.14+ openzfs#7170
- Report duration and error in mmp_history entries openzfs#7190
- Do not initiate MMP writes while pool is suspended openzfs#7182
- Linux 4.16 compat: use correct *_dec_and_test()
- Allow modprobe to fail when called within systemd openzfs#7174
- Add SMART attributes for SSD and NVMe openzfs#7183 openzfs#7193
- Correct count_uberblocks in mmp.kshlib openzfs#7191
- Fix config issues: frame size and headers openzfs#7169
- Clarify zinject(8) explanation of -e openzfs#7172
- OpenZFS 8857 - zio_remove_child() panic due to already destroyed parent zio openzfs#7168
- 'zfs receive' fails with "dataset is busy" openzfs#7129 openzfs#7154
- contrib/initramfs: add missing conf.d/zfs openzfs#7158
- mmp should use a fixed tag for spa_config locks openzfs#6530 openzfs#7155
- Handle zap_add() failures in mixed case mode openzfs#7011 openzfs#7054
- Fix zdb -ed on objset for exported pool openzfs#7099 openzfs#6464
- Fix zdb -E segfault openzfs#7099
- Fix zdb -R decompression openzfs#7099 openzfs#4984
- Fix racy assignment of zcb.zcb_haderrors openzfs#7099
- Fix zle_decompress out of bound access openzfs#7099
- Fix zdb -c traverse stop on damaged objset root openzfs#7099
- Linux 4.11 compat: avoid refcount_t name conflict openzfs#7148
- Linux 4.16 compat: inode_set_iversion() openzfs#7148
- OpenZFS 8966 - Source file zfs_acl.c, function zfs_aclset_common contains a use after end of the lifetime of a local variable openzfs#7141
- Remove deprecated zfs_arc_p_aggressive_disable openzfs#7135
- Fix default libdir for Debian/Ubuntu openzfs#7083 openzfs#7101
- Bug fix in qat_compress.c for vmalloc addr check openzfs#7125
- Fix systemd_ RPM macros usage on Debian-based distributions openzfs#7074 openzfs#7100
- Emit an error message before MMP suspends pool openzfs#7048
- ZTS: Fix create-o_ashift test case openzfs#6924 openzfs#6977
- Fix --with-systemd on Debian-based distributions (openzfs#6963) openzfs#6591 openzfs#6963
- Remove vn_rename and vn_remove dependency openzfs/spl#648 openzfs#6753
- Add support for "--enable-code-coverage" option openzfs#6670
- Make "-fno-inline" compile option more accessible openzfs#6605
- Add configure option to enable gcov analysis openzfs#6642
- Implement --enable-debuginfo to force debuginfo openzfs#2734
- Make --enable-debug fail when given bogus args openzfs#2734

Signed-off-by: Tony Hutter <hutter2@llnl.gov>
Requires-spl: refs/pull/690/head
@tonyhutter tonyhutter mentioned this pull request Mar 13, 2018
13 tasks
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Mar 13, 2018
This is a squashed patchset for zfs-0.7.7.  The individual commits are
in the tonyhutter:zfs-0.7.7-hutter branch.  I squashed the commits so
that buildbot wouldn't have to run against each one, and because
github/builbot seem to have a maximum limit of 30 commits they can
test from a PR.

- Fix MMP write frequency for large pools openzfs#7205 openzfs#7289
- Handle zio_resume and mmp => off openzfs#7286
- Fix zfs-kmod builds when using rpm >= 4.14 openzfs#7284
- zdb and inuse tests don't pass with real disks openzfs#6939 openzfs#7261
- Take user namespaces into account in policy checks openzfs#6800 openzfs#7270
- Detect long config lock acquisition in mmp openzfs#7212
- Linux 4.16 compat: get_disk_and_module() openzfs#7264
- Change checksum & IO delay ratelimit values openzfs#7252
- Increment zil_itx_needcopy_bytes properly openzfs#6988 openzfs#7176
- Fix some typos openzfs#7237
- Fix zpool(8) list example to match actual format openzfs#7244
- Add SMART self-test results to zpool status -c openzfs#7178
- Add scrub after resilver zed script openzfs#4662 openzfs#7086
- Fix free memory calculation on v3.14+ openzfs#7170
- Report duration and error in mmp_history entries openzfs#7190
- Do not initiate MMP writes while pool is suspended openzfs#7182
- Linux 4.16 compat: use correct *_dec_and_test()
- Allow modprobe to fail when called within systemd openzfs#7174
- Add SMART attributes for SSD and NVMe openzfs#7183 openzfs#7193
- Correct count_uberblocks in mmp.kshlib openzfs#7191
- Fix config issues: frame size and headers openzfs#7169
- Clarify zinject(8) explanation of -e openzfs#7172
- OpenZFS 8857 - zio_remove_child() panic due to already destroyed
  parent zio openzfs#7168
- 'zfs receive' fails with "dataset is busy" openzfs#7129 openzfs#7154
- contrib/initramfs: add missing conf.d/zfs openzfs#7158
- mmp should use a fixed tag for spa_config locks openzfs#6530 openzfs#7155
- Handle zap_add() failures in mixed case mode openzfs#7011 openzfs#7054
- Fix zdb -ed on objset for exported pool openzfs#7099 openzfs#6464
- Fix zdb -E segfault openzfs#7099
- Fix zdb -R decompression openzfs#7099 openzfs#4984
- Fix racy assignment of zcb.zcb_haderrors openzfs#7099
- Fix zle_decompress out of bound access openzfs#7099
- Fix zdb -c traverse stop on damaged objset root openzfs#7099
- Linux 4.11 compat: avoid refcount_t name conflict openzfs#7148
- Linux 4.16 compat: inode_set_iversion() openzfs#7148
- OpenZFS 8966 - Source file zfs_acl.c, function zfs_aclset_common
  contains a use after end of the lifetime of a local variable openzfs#7141
- Remove deprecated zfs_arc_p_aggressive_disable openzfs#7135
- Fix default libdir for Debian/Ubuntu openzfs#7083 openzfs#7101
- Bug fix in qat_compress.c for vmalloc addr check openzfs#7125
- Fix systemd_ RPM macros usage on Debian-based distributions openzfs#7074
  openzfs#7100
- Emit an error message before MMP suspends pool openzfs#7048
- ZTS: Fix create-o_ashift test case openzfs#6924 openzfs#6977
- Fix --with-systemd on Debian-based distributions (openzfs#6963) openzfs#6591 openzfs#6963
- Remove vn_rename and vn_remove dependency openzfs/spl#648 openzfs#6753
- Add support for "--enable-code-coverage" option openzfs#6670
- Make "-fno-inline" compile option more accessible openzfs#6605
- Add configure option to enable gcov analysis openzfs#6642
- Implement --enable-debuginfo to force debuginfo openzfs#2734
- Make --enable-debug fail when given bogus args openzfs#2734

Signed-off-by: Tony Hutter <hutter2@llnl.gov>
Requires-spl: refs/pull/690/head
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Mar 13, 2018
Change file related checks to use user namespaces and make
sure involved uids/gids are mappable in the current
namespace.

Note that checks without file ownership information will
still not take user namespaces into account, as some of
these should be handled via 'zfs allow' (otherwise root in a
user namespace could issue commands such as `zpool export`).

This also adds an initial user namespace regression test
for the setgid bit loss, with a user_ns_exec helper usable
in further tests.

Additionally, configure checks for the required user
namespace related features are added for:
  * ns_capable
  * kuid/kgid_has_mapping()
  * user_ns in cred_t

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Closes openzfs#6800 
Closes openzfs#7270
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Mar 13, 2018
This is a squashed patchset for zfs-0.7.7.  The individual commits are
in the tonyhutter:zfs-0.7.7-hutter branch.  I squashed the commits so
that buildbot wouldn't have to run against each one, and because
github/builbot seem to have a maximum limit of 30 commits they can
test from a PR.

- Fix MMP write frequency for large pools openzfs#7205 openzfs#7289
- Handle zio_resume and mmp => off openzfs#7286
- Fix zfs-kmod builds when using rpm >= 4.14 openzfs#7284
- zdb and inuse tests don't pass with real disks openzfs#6939 openzfs#7261
- Take user namespaces into account in policy checks openzfs#6800 openzfs#7270
- Detect long config lock acquisition in mmp openzfs#7212
- Linux 4.16 compat: get_disk_and_module() openzfs#7264
- Change checksum & IO delay ratelimit values openzfs#7252
- Increment zil_itx_needcopy_bytes properly openzfs#6988 openzfs#7176
- Fix some typos openzfs#7237
- Fix zpool(8) list example to match actual format openzfs#7244
- Add SMART self-test results to zpool status -c openzfs#7178
- Add scrub after resilver zed script openzfs#4662 openzfs#7086
- Fix free memory calculation on v3.14+ openzfs#7170
- Report duration and error in mmp_history entries openzfs#7190
- Do not initiate MMP writes while pool is suspended openzfs#7182
- Linux 4.16 compat: use correct *_dec_and_test()
- Allow modprobe to fail when called within systemd openzfs#7174
- Add SMART attributes for SSD and NVMe openzfs#7183 openzfs#7193
- Correct count_uberblocks in mmp.kshlib openzfs#7191
- Fix config issues: frame size and headers openzfs#7169
- Clarify zinject(8) explanation of -e openzfs#7172
- OpenZFS 8857 - zio_remove_child() panic due to already destroyed
  parent zio openzfs#7168
- 'zfs receive' fails with "dataset is busy" openzfs#7129 openzfs#7154
- contrib/initramfs: add missing conf.d/zfs openzfs#7158
- mmp should use a fixed tag for spa_config locks openzfs#6530 openzfs#7155
- Handle zap_add() failures in mixed case mode openzfs#7011 openzfs#7054
- Fix zdb -ed on objset for exported pool openzfs#7099 openzfs#6464
- Fix zdb -E segfault openzfs#7099
- Fix zdb -R decompression openzfs#7099 openzfs#4984
- Fix racy assignment of zcb.zcb_haderrors openzfs#7099
- Fix zle_decompress out of bound access openzfs#7099
- Fix zdb -c traverse stop on damaged objset root openzfs#7099
- Linux 4.11 compat: avoid refcount_t name conflict openzfs#7148
- Linux 4.16 compat: inode_set_iversion() openzfs#7148
- OpenZFS 8966 - Source file zfs_acl.c, function zfs_aclset_common
  contains a use after end of the lifetime of a local variable openzfs#7141
- Remove deprecated zfs_arc_p_aggressive_disable openzfs#7135
- Fix default libdir for Debian/Ubuntu openzfs#7083 openzfs#7101
- Bug fix in qat_compress.c for vmalloc addr check openzfs#7125
- Fix systemd_ RPM macros usage on Debian-based distributions openzfs#7074
  openzfs#7100
- Emit an error message before MMP suspends pool openzfs#7048
- ZTS: Fix create-o_ashift test case openzfs#6924 openzfs#6977
- Fix --with-systemd on Debian-based distributions (openzfs#6963) openzfs#6591 openzfs#6963
- Remove vn_rename and vn_remove dependency openzfs/spl#648 openzfs#6753
- Fix "--enable-code-coverage" debug build openzfs#6674
- Update codecov.yml openzfs#6669
- Add support for "--enable-code-coverage" option openzfs#6670
- Make "-fno-inline" compile option more accessible openzfs#6605
- Add configure option to enable gcov analysis openzfs#6642
- Implement --enable-debuginfo to force debuginfo openzfs#2734
- Make --enable-debug fail when given bogus args openzfs#2734

Signed-off-by: Tony Hutter <hutter2@llnl.gov>
Requires-spl: refs/pull/690/head
tonyhutter pushed a commit that referenced this pull request Mar 19, 2018
Change file related checks to use user namespaces and make
sure involved uids/gids are mappable in the current
namespace.

Note that checks without file ownership information will
still not take user namespaces into account, as some of
these should be handled via 'zfs allow' (otherwise root in a
user namespace could issue commands such as `zpool export`).

This also adds an initial user namespace regression test
for the setgid bit loss, with a user_ns_exec helper usable
in further tests.

Additionally, configure checks for the required user
namespace related features are added for:
  * ns_capable
  * kuid/kgid_has_mapping()
  * user_ns in cred_t

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Closes #6800
Closes #7270
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.

setgid bit in chmod dropped inconsistently
3 participants