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

 Support -fsanitizer=address in user space #6941

Closed
wants to merge 4 commits into from

Conversation

behlendorf
Copy link
Contributor

Description

When --enable-asan is provided to configure build all user space components with -fsanitize=address. For kernel support use the Linux KASAN feature instead.

https://github.com/google/sanitizers/wiki/AddressSanitizer

When using gcc version 4.8 any test case which intentionally generates a core dumps will fail when using --enable-asan. The default behavior is to disable core dumps and only newer versions allow this behavior to be controled at run time with the ASAN_OPTIONS environment variable.

Motivation and Context

Primarily motivated and based on @DeHackEd's changes in #6884 to make it possible to use -fsanitizer=address with ztest. Since the support is optionally enabled I've extended it to cover all of user space. The intention is to eventually enable it in buildbot to improve the automated testing.

How Has This Been Tested?

Locally verified on CentOS 7, pushed to the buildbot to verify the build system changes. Additional manual local testing is needed.

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:

  • 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.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

@behlendorf behlendorf added the Status: Work in Progress Not yet ready for general review label Dec 9, 2017
@behlendorf behlendorf force-pushed the ztest-asan branch 2 times, most recently from bc0d4a5 to efc1579 Compare December 11, 2017 21:37
@behlendorf
Copy link
Contributor Author

This PR is working as expected when enabled in the buildbot. In fact, it currently detects at least one leak in ztest shown below which will need to be resolved. That however shouldn't block this PR since the new asan functionality is disabled by default.

@DeHackEd it would be great if you could look these changes over.

==21847==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 120 byte(s) in 3 object(s) allocated from:
    #0 0x7fef87de8850 in malloc (/lib64/libasan.so.4+0xde850)
    #1 0x7fef862a97eb  (/lib64/libzpool.so.2+0x2397eb)

Indirect leak of 24 byte(s) in 3 object(s) allocated from:
    #0 0x7fef87de8850 in malloc (/lib64/libasan.so.4+0xde850)
    #1 0x7fef862a97eb  (/lib64/libzpool.so.2+0x2397eb)

SUMMARY: AddressSanitizer: 144 byte(s) leaked in 6 allocation(s).
/lib64/libasan.so.4(+0x555c0)[0x7fef87d5f5c0]
/usr/sbin/ztest(+0x1691f)[0x5611ac83c91f]
/lib64/libpthread.so.0(+0x12a70)[0x7fef850c8a70]
/lib64/libc.so.6(gsignal+0xcb)[0x7fef84d0869b]
/lib64/libc.so.6(abort+0x141)[0x7fef84d0a3b1]
/lib64/libasan.so.4(+0x10043e)[0x7fef87e0a43e]
/lib64/libasan.so.4(+0x108238)[0x7fef87e12238]
/lib64/libasan.so.4(+0x10c482)[0x7fef87e16482]
/lib64/libc.so.6(__cxa_finalize+0x95)[0x7fef84d0cef5]
/lib64/libasan.so.4(+0x20b03)[0x7fef87d2ab03]

@behlendorf behlendorf removed the Status: Work in Progress Not yet ready for general review label Dec 11, 2017
@DeHackEd
Copy link
Contributor

LGTM, though it feels like the last commit contains a mixture of code cleanup, debuginfo management, and ASAN all in one.

From my older patch I was hitting a refcount memory leak, but for some reason I wasn't getting stack frames below that so I don't know where the actual leak originates. Did you find that one? Still, like you said this isn't a blocker for the time being - just want to make sure this is on the radar.

@behlendorf
Copy link
Contributor Author

though it feels like the last commit contains a mixture of code cleanup, debuginfo management, and ASAN all in one.

Yes, I can split this up if you like I did end up doing a little more clean than originally intended.

I wasn't getting stack frames below that so I don't know where the actual leak originates. Did you find that one?

Yes, I saw this as well and was confused about the lack of a stack frame identifying the leak location. This still needs to be explained and run down. I did not observed this initially on CentOS 7, presumably due to the much older version of the tool chain.

Surprisingly this change also appears to be causing reliably failures for CentOS 6 which I need to run down. I'll see about splitting up this PR in to different PRs so so the more trivial patches can be merged and the root cause the for the CentOS 6 segfaults identified.

DeHackEd and others added 4 commits December 28, 2017 13:27
kmem_alloc(0, ...) in userspace returns a leakable pointer.

Signed-off-by: DHE <git@dehacked.net>
In ztest_verify_dnode_bt the ztest_object_lock must be held in
order to safely verify the unused bonus space.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Resolved unused variable warnings observed after restricting
-Wno-unused-but-set-variable to only libzfs and libzpool.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
When --enable-asan is provided to configure then build all user
space components with fsanitize=address.  For kernel support
use the Linux KASAN feature instead.

https://github.com/google/sanitizers/wiki/AddressSanitizer

When using gcc version 4.8 any test case which intentionally
generates a core dump will fail when using --enable-asan.
The default behavior is to disable core dumps and only newer
versions allow this behavior to be controled at run time with
the ASAN_OPTIONS environment variable.

Additionally, this patch includes some build system cleanup.

* Rules.am updated to set the minimum AM_CFLAGS, AM_CPPFLAGS,
  and AM_LDFLAGS.  Any additional flags should be added on a
  per-Makefile basic.  The --enable-debug and --enable-asan
  options apply to all user space binaries and libraries.

* Compiler checks consolidated in always-compiler-options.m4
  and renamed for consistency.

* -fstack-check compiler flags was moved from DEBUG_STACKFLAGS
  in to DEBUG_CFLAGS for clarity.  Set with --enable-debug.

* Split DEBUG_CFLAGS in to DEBUG_CFLAGS, DEBUG_CPPFLAGS, and
  DEBUG_LDFLAGS.

* Moved default kernel build flags in to module/Makefile.in and
  split in to ZFS_MODULE_CFLAGS and ZFS_MODULE_CPPFLAGS.  These
  flags are set with the standard ccflags-y kbuild mechanism.

* -Wframe-larger-than checks applied only to binaries or
  libraries which include source files which are built in
  both user space and kernel space.  This restriction is
  relaxed for user space only utilities.

* -Wno-unused-but-set-variable applied only to libzfs and
  libzpool.  The remaining warnings are the result of an
  ASSERT using a variable when is always declared.

Signed-off-by: DHE <git@dehacked.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
@codecov
Copy link

codecov bot commented Dec 29, 2017

Codecov Report

Merging #6941 into master will decrease coverage by 0.08%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6941      +/-   ##
==========================================
- Coverage    75.4%   75.31%   -0.09%     
==========================================
  Files         296      296              
  Lines       95483    95489       +6     
==========================================
- Hits        72002    71921      -81     
- Misses      23481    23568      +87
Flag Coverage Δ
#kernel 74.54% <100%> (-0.27%) ⬇️
#user 67.63% <83.33%> (-0.16%) ⬇️

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 390d679...eceb928. Read the comment docs.

behlendorf pushed a commit that referenced this pull request Jan 9, 2018
kmem_alloc(0, ...) in userspace returns a leakable pointer.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: DHE <git@dehacked.net>
Issue #6941
@behlendorf behlendorf closed this in 06401e4 Jan 9, 2018
behlendorf added a commit that referenced this pull request Jan 9, 2018
Resolved unused variable warnings observed after restricting
-Wno-unused-but-set-variable to only libzfs and libzpool.

Reviewed-by: DHE <git@dehacked.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes #6941
@behlendorf behlendorf deleted the ztest-asan branch January 9, 2018 22:21
prometheanfire pushed a commit to prometheanfire/zfs that referenced this pull request Jan 11, 2018
kmem_alloc(0, ...) in userspace returns a leakable pointer.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: DHE <git@dehacked.net>
Issue openzfs#6941
prometheanfire pushed a commit to prometheanfire/zfs that referenced this pull request Jan 11, 2018
In ztest_verify_dnode_bt the ztest_object_lock must be held in
order to safely verify the unused bonus space.

Reviewed-by: DHE <git@dehacked.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#6941
prometheanfire pushed a commit to prometheanfire/zfs that referenced this pull request Jan 11, 2018
Resolved unused variable warnings observed after restricting
-Wno-unused-but-set-variable to only libzfs and libzpool.

Reviewed-by: DHE <git@dehacked.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#6941
prometheanfire pushed a commit to prometheanfire/zfs that referenced this pull request Jan 11, 2018
kmem_alloc(0, ...) in userspace returns a leakable pointer.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: DHE <git@dehacked.net>
Issue openzfs#6941
prometheanfire pushed a commit to prometheanfire/zfs that referenced this pull request Jan 11, 2018
In ztest_verify_dnode_bt the ztest_object_lock must be held in
order to safely verify the unused bonus space.

Reviewed-by: DHE <git@dehacked.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#6941
prometheanfire pushed a commit to prometheanfire/zfs that referenced this pull request Jan 11, 2018
Resolved unused variable warnings observed after restricting
-Wno-unused-but-set-variable to only libzfs and libzpool.

Reviewed-by: DHE <git@dehacked.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#6941
prometheanfire pushed a commit to prometheanfire/zfs that referenced this pull request Jan 17, 2018
kmem_alloc(0, ...) in userspace returns a leakable pointer.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: DHE <git@dehacked.net>
Issue openzfs#6941
prometheanfire pushed a commit to prometheanfire/zfs that referenced this pull request Jan 17, 2018
In ztest_verify_dnode_bt the ztest_object_lock must be held in
order to safely verify the unused bonus space.

Reviewed-by: DHE <git@dehacked.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#6941
prometheanfire pushed a commit to prometheanfire/zfs that referenced this pull request Jan 17, 2018
Resolved unused variable warnings observed after restricting
-Wno-unused-but-set-variable to only libzfs and libzpool.

Reviewed-by: DHE <git@dehacked.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#6941
Nasf-Fan pushed a commit to Nasf-Fan/zfs that referenced this pull request Jan 29, 2018
kmem_alloc(0, ...) in userspace returns a leakable pointer.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: DHE <git@dehacked.net>
Issue openzfs#6941
Nasf-Fan pushed a commit to Nasf-Fan/zfs that referenced this pull request Jan 29, 2018
In ztest_verify_dnode_bt the ztest_object_lock must be held in
order to safely verify the unused bonus space.

Reviewed-by: DHE <git@dehacked.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#6941
Nasf-Fan pushed a commit to Nasf-Fan/zfs that referenced this pull request Jan 29, 2018
Resolved unused variable warnings observed after restricting
-Wno-unused-but-set-variable to only libzfs and libzpool.

Reviewed-by: DHE <git@dehacked.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#6941
Nasf-Fan pushed a commit to Nasf-Fan/zfs that referenced this pull request Feb 13, 2018
kmem_alloc(0, ...) in userspace returns a leakable pointer.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: DHE <git@dehacked.net>
Issue openzfs#6941
Nasf-Fan pushed a commit to Nasf-Fan/zfs that referenced this pull request Feb 13, 2018
In ztest_verify_dnode_bt the ztest_object_lock must be held in
order to safely verify the unused bonus space.

Reviewed-by: DHE <git@dehacked.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#6941
Nasf-Fan pushed a commit to Nasf-Fan/zfs that referenced this pull request Feb 13, 2018
Resolved unused variable warnings observed after restricting
-Wno-unused-but-set-variable to only libzfs and libzpool.

Reviewed-by: DHE <git@dehacked.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#6941
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.

2 participants