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 build with KASAN #12232

Merged
merged 1 commit into from
Jun 26, 2021
Merged

Fix build with KASAN #12232

merged 1 commit into from
Jun 26, 2021

Conversation

rincebrain
Copy link
Contributor

@rincebrain rincebrain commented Jun 12, 2021

Motivation and Context

Presumably since the zstd commit landed, no longer builds with KASAN.

Description

The stock zstd code expects some helpers from ASAN if ADDRESS_SANITIZER
is defined. This works fine in userland, but in kernel, KASAN also
results in that getting defined, but does not provide those helpers. So let's make some empty
substitutes for that case.

How Has This Been Tested?

I ran through ZTS with this applied on a kernel on Debian buster with KASAN (though not without failures; that's another day).
I also built it on a kernel without KASAN to make certain the defines seemed right.

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)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@rincebrain
Copy link
Contributor Author

Fascinating, am I just blind, and didn't notice the build reporting problems, or did something strange ensue?

I had occasion to rollback my KASAN VM, reapply this patch to master, and rebuild, and lo, the symbols were missing, because ADDRESS_SANITIZER is a define from earlier in the zstd library code.

How this built (and loaded, and ran through ZTS) for me before will remain a mystery to me, since I did do the prodding I documented in the testing section above. Perhaps I didn't clobber the installed old modules?

Anyway, I just looted the test from lib/zstd.c, in the interest of not winding up in a strange position where it gets defined one place and not the other, or the obnoxiously long #if defined(...) && (defined(...) || defined(...)) that would result from collapsing it.

To ensure no mysterious excitement again, I did a completely clean checkout, applied this patch, built it, verified it wasn't already loaded, and loaded the zzstd module, on my KASAN system. No risk of fun this time.

The stock zstd code expects some helpers from ASAN if present.
This works fine in userland, but in kernel, KASAN also gets detected,
and lacks those helpers. So let's make some empty substitutes for
that case.

Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Jun 13, 2021
module/zstd/zfs_zstd.c Show resolved Hide resolved
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 26, 2021
@behlendorf behlendorf merged commit 2084e9f into openzfs:master Jun 26, 2021
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 29, 2021
The stock zstd code expects some helpers from ASAN if present.
This works fine in userland, but in kernel, KASAN also gets detected,
and lacks those helpers. So let's make some empty substitutes for
that case.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes openzfs#12232
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants