Support -fsanitizer=address on ztest #6884

DeHackEd · 2017-11-18T23:27:49Z

Requires minimum of gcc version 4.8. Only available to ztest
right now, but easily added to other user applications. For
kernel support use the Linux KASAN feature instead.

Signed-off-by: DHE git@dehacked.net

Description

If your compiler supports -fsanitize=address and you configure with --enable-debug most libraries, zdb and ztest will be built with it enabled.

Motivation and Context

By request from @behlendorf in #6877

How Has This Been Tested?

Build testing for the disabled path, full test of executable launch abilities for the debug path.

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.

DeHackEd · 2017-11-18T23:31:15Z

Surprises:

-fsanitize=address adds a lot to the stack sizes of functions. When building with ASAN I raised the stack size from 1024 to 3072. The highest stack size I noticed when building was 2832.
To be useful, both ztest and libzpool at a minimum would require ASAN enabled. In the end it just made sense to enable it for all the libraries and to LDFLAGS for binaries in general.

In a little while I will push a revert to #6877 to see it in action on the buildbots. :)

DeHackEd · 2017-11-18T23:48:05Z

Buildbots failing becuase libasan is not installed by defualt. On CentOS 7 libasan needs to be installed. :/

behlendorf · 2017-11-19T00:27:49Z

@DeHackEd thanks! You can add libasan to the installed dependencies by opening a PR to update this script and tagging @dinatale2 for a review to get it merged.

https://github.com/zfsonlinux/zfs-buildbot/blob/master/scripts/bb-dependencies.sh

dinatale2 · 2017-11-20T16:47:17Z

FYI @DeHackEd openzfs/zfs-buildbot#123 was just merged.

codecov · 2017-11-20T16:53:26Z

Codecov Report

❗ No coverage uploaded for pull request base (master@ed15d54). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #6884   +/-   ##
=========================================
  Coverage          ?   71.67%           
=========================================
  Files             ?      296           
  Lines             ?    95015           
  Branches          ?        0           
=========================================
  Hits              ?    68098           
  Misses            ?    26917           
  Partials          ?        0

Flag	Coverage Δ
#kernel	`70.84% <100%> (?)`
#user	`64.93% <100%> (?)`

Impacted Files	Coverage Δ
module/zfs/spa.c	`74.81% <100%> (ø)`

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 ed15d54...44617d9. Read the comment docs.

DeHackEd · 2017-11-20T16:53:32Z

Thanks. I've pushed a rebase and fix for minor issues in the PR to kick off the buildbots again.

DeHackEd · 2017-11-22T02:27:47Z

Update pushed. This version tries a full link in the configure script to detect systems where libasan is missing or broken and doesn't prevent building.

behlendorf · 2017-11-28T23:57:25Z

module/zfs/spa.c

+	/*
+	 * -fsanitize in userspace will detect kmem_alloc(0, ...)
+	 * as a leak
+	 */


No comment needed, it is a leak and should be fixed.

behlendorf · 2017-11-29T00:01:33Z

module/zfs/spa.c

+	 * as a leak
+	 */
+	if (sav->sav_count > 0)
+		l2cache = kmem_alloc(sav->sav_count * sizeof (void *),


It looks like this same issue can occur on line 1588, I'm surprised it wasn't also flagged.

behlendorf

It's good to see that enabling this functionality in ztest didn't expose any new issues. Since that went smoothly have you by chance looked in to enabling for all of user space for debug builds? We could do that right away by adding it to DEBUG_CFLAGS assuming it doesn't identify a slew of issues which would need to be resolved first.

I'm not sure why but this patch appears to break the ZFS_ABORT functionality, calling abort() no longer generates a core causing several test cases to fail.

behlendorf · 2017-11-29T00:04:48Z

cmd/ztest/Makefile.am

@@ -2,7 +2,7 @@ include $(top_srcdir)/config/Rules.am

 # -Wnoformat-truncation to get rid of compiler warning for unchecked
 #  truncating snprintfs on gcc 7.1.1.
-AM_CFLAGS += $(DEBUG_STACKFLAGS) $(FRAME_LARGER_THAN) $(NO_FORMAT_TRUNCATION)
+AM_CFLAGS += $(DEBUG_STACKFLAGS) $(FRAME_LARGER_THAN) $(NO_FORMAT_TRUNCATION) $(COMPILER_FSANITIZE_ADDRESS)


Let's pull $(COMPILER_FSANITIZE_ADDRESS) down on to it's own line to mind the 80 character limit. Here and elsewhere.

AM_CFLAGS += $(DEBUG_STACKFLAGS) $(FRAME_LARGER_THAN) $(NO_FORMAT_TRUNCATION) AM_CFLAGS += $(COMPILER_FSANITIZE_ADDRESS)

behlendorf · 2017-11-29T00:07:37Z

config/user-compiler-options.m4

+	])
+
+	CFLAGS="$saved_flags"
+        AC_SUBST([FRAME_LARGER_THAN])


No objection to consolidating these, but please fix the whitespace on this line while you're here. Let's also pull in user-no-format-truncation.m4.

behlendorf · 2017-11-29T00:14:10Z

config/user-compiler-options.m4

+
+	AC_COMPILE_IFELSE([AC_LANG_PROGRAM([], [])],
+	[
+		if test -z "$COMPILER_FSANITIZE_ADDRESS"


The AS_IF macro if available for the conditional. However, I think it'd be reasonable to simply bump this unconditionally to 4096 to cover both cases. This only applies to user space where we have headroom, and this was originally added before the kernel build system set -Wframe-larger-than by default.

behlendorf · 2017-11-29T00:30:30Z

config/user-compiler-options.m4

+		CFLAGS="$saved_flags"
+		LDFLAGS="$saved_ldflags"
+	fi
+	AC_SUBST([COMPILER_FSANITIZE_ADDRESS])


I'd suggest either dropping the COMPILER_ prefix, which isn't super useful and would be my preference. Or alternately adding it to NO_FORMAT_TRUNCATION and FRAME_LARGER_THAN for consistency.

behlendorf · 2017-11-29T00:36:51Z

config/zfs-build.m4

@@ -27,7 +27,7 @@ AC_DEFUN([ZFS_AC_DEBUG], [
 	AC_MSG_CHECKING([whether assertion support will be enabled])
 	AC_ARG_ENABLE([debug],
 		[AS_HELP_STRING([--enable-debug],
-		[Enable assertion support @<:@default=no@:>@])],
+		[Enable assertion support and compiler features where available @<:@default=no@:>@])],


The way you've implemented this setting --enable-debug has no effect of this compiler feature. It only defines DEBUG and sets -Werror, so I'm assuming this is an unrelated bit of clarification. In which case, how about the following message instead.

Enable assertions and -Werror @<:@default=no@:>@

Actually it does. -fsanitize is only checked for and hence enabled when a debug build is performed. The whole new m4 block is guarded with

if test "$enable_debug" = "yes"

This allows a normal build to operate without -fsanitize even on systems which support it.

But I agree, this text needs rewording.

Ahh, I see. I glossed over that in my initial reading. What do you think about moving the call points for these debug-only compiler checks in to ZFS_AC_DEBUG_ENABLE. This way these M4 macro's will only run for debug builds where they're needed.

I wanted to keep the stack size check together with this one for the sake of coherence and so that non-debug builds would still use a sane stack size check value. m4 is new to me so I'm sure my style is horrible.

kmem_alloc(0, ...) in userspace returns a leakable pointer. Signed-off-by: DHE <git@dehacked.net>

DeHackEd · 2017-11-29T23:17:52Z

Regarding the memory leak, the l2cache variable is stack-local whereas newvdevs will be assigned to sav->sav_vdevs to be freed later in spa_unload().

That said I have been hitting another leak in the refcount code but I'm not getting much information about where the allocation is coming from yet. We still need that fixed before we push this branch.

Requires minimum of gcc version 4.8. Only available to ztest right now, but easily added to other user applications. For kernel support use the Linux KASAN feature instead. Signed-off-by: DHE <git@dehacked.net>

behlendorf · 2017-12-11T23:43:16Z

Closing, replaced by #6941

DeHackEd changed the title ~~Ztest asan~~ Support -fsanitizer=address on ztest Nov 18, 2017

DeHackEd force-pushed the ztest-asan branch from c719f23 to 8205d6f Compare November 20, 2017 16:52

DeHackEd force-pushed the ztest-asan branch 3 times, most recently from f003d45 to 524c401 Compare November 22, 2017 02:26

behlendorf requested changes Nov 29, 2017

View reviewed changes

DeHackEd force-pushed the ztest-asan branch from 524c401 to f73422d Compare November 29, 2017 00:56

Fix -fsanitize=address memory leak

5f138ee

kmem_alloc(0, ...) in userspace returns a leakable pointer. Signed-off-by: DHE <git@dehacked.net>

DeHackEd force-pushed the ztest-asan branch from f73422d to 2d4f414 Compare November 29, 2017 23:15

Support -fsanitize=address on ztest

44617d9

Requires minimum of gcc version 4.8. Only available to ztest right now, but easily added to other user applications. For kernel support use the Linux KASAN feature instead. Signed-off-by: DHE <git@dehacked.net>

DeHackEd force-pushed the ztest-asan branch from 2d4f414 to 44617d9 Compare November 29, 2017 23:29

behlendorf added the Status: Work in Progress Not yet ready for general review label Nov 30, 2017

behlendorf mentioned this pull request Dec 9, 2017

Support -fsanitizer=address in user space #6941

Closed

13 tasks

behlendorf closed this Dec 11, 2017

behlendorf mentioned this pull request Jan 9, 2018

Support -fsanitize=address with --enable-asan #7027

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support -fsanitizer=address on ztest #6884

Support -fsanitizer=address on ztest #6884

DeHackEd commented Nov 18, 2017

DeHackEd commented Nov 18, 2017

DeHackEd commented Nov 18, 2017

behlendorf commented Nov 19, 2017

dinatale2 commented Nov 20, 2017

codecov bot commented Nov 20, 2017 •

edited

Loading

DeHackEd commented Nov 20, 2017

DeHackEd commented Nov 22, 2017

behlendorf Nov 28, 2017

behlendorf Nov 29, 2017

behlendorf left a comment

behlendorf Nov 29, 2017

behlendorf Nov 29, 2017

behlendorf Nov 29, 2017

behlendorf Nov 29, 2017

behlendorf Nov 29, 2017

DeHackEd Nov 29, 2017

behlendorf Nov 29, 2017

DeHackEd Nov 29, 2017

DeHackEd commented Nov 29, 2017

behlendorf commented Dec 11, 2017

Support -fsanitizer=address on ztest #6884

Support -fsanitizer=address on ztest #6884

Conversation

DeHackEd commented Nov 18, 2017

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

DeHackEd commented Nov 18, 2017

DeHackEd commented Nov 18, 2017

behlendorf commented Nov 19, 2017

dinatale2 commented Nov 20, 2017

codecov bot commented Nov 20, 2017 • edited Loading

Codecov Report

DeHackEd commented Nov 20, 2017

DeHackEd commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behlendorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeHackEd commented Nov 29, 2017

behlendorf commented Dec 11, 2017

codecov bot commented Nov 20, 2017 •

edited

Loading