Page MenuHomePhabricator

A workaround for getting process memory map taking too long for sanitizers on FreeBSD
AbandonedPublic

Authored by kutuzov.viktor.84 on May 12 2014, 1:31 PM.

Details

Summary

This patch implements a workaround for the issue with sysctl(KERN_PROC_VMMAP) calls on FreeBSD:

http://www.freebsd.org/cgi/query-pr.cgi?pr=188911

The patch should be reverted back as soon as the bug is fixed.

Since the patch relies on reading '/dev/kmem' which is not accessible for normal users by default, the following command granting reading access to all users shall be executed before running tests:

sudo chmod a+r /dev/kmem

Diff Detail

Event Timeline

kutuzov.viktor.84 retitled this revision from to A workaround for getting process memory map taking too long for sanitizers on FreeBSD.
kutuzov.viktor.84 updated this object.
kutuzov.viktor.84 edited the test plan for this revision. (Show Details)
kutuzov.viktor.84 added reviewers: kcc, samsonov.
kutuzov.viktor.84 added subscribers: Unknown Object (MLST), emaste.
kcc edited edge metadata.May 13 2014, 2:19 AM

this is too much ifdef-ed code.
Given the size of this patch I think it's better to create lib/sanitizer_common/sanitizer_procmaps_freebsd.cc and move all freebsd-specific logic there
(and remove all freebsd logic from lib/sanitizer_common/sanitizer_procmaps_linux.cc)

kutuzov.viktor.84 edited edge metadata.

FreeBSD-specific things moved to newly added lib/sanitizer_common/sanitizer_procmaps_freebsd.cc .

kcc added a comment.May 13 2014, 6:51 AM

So, we have some linux-specific and some common code in parts in sanitizer_procmaps_linux.cc
Maybe split sanitizer_procmaps_linux.cc even further (create sanitizer_procmaps_common.cc and remove all SANITIZER_FREEBSD from sanitizer_procmaps_linux.cc)?
Not sure how this will look. Can you try?

kcc added a comment.May 13 2014, 6:57 AM

BTW, the whole FREEBSD-specific part is gobbledygook for me.
I don't mind it as long as it is completely under #if SANITIZER_FREEBSD,
but maybe you can CC some FreeBSD expert just in case.

Maybe split sanitizer_procmaps_linux.cc even further (create sanitizer_procmaps_common.cc and remove all SANITIZER_FREEBSD from sanitizer_procmaps_linux.cc)?
Not sure how this will look. Can you try?

Sure. Will do.

BTW, the whole FREEBSD-specific part is gobbledygook for me.
I don't mind it as long as it is completely under #if SANITIZER_FREEBSD,
but maybe you can CC some FreeBSD expert just in case.

Ed, if you know other FreeBSD maintainers regstered here, can you please add them to CC?

Thanks!

Two changes:

  • common Linux/FreeBSD parts moved out to sanitizer_procmaps_common.cc and
  • the workaround code now ignore the header map entry that correspond to the whole available memory range.

With this patch we pass x86-64 Asan unit tests on FreeBSD.

kcc added a comment.May 14 2014, 12:01 AM

Looks good.
I'd like to hear from some one else regarding the FreeBSD part.

theraven edited edge metadata.Jun 23 2014, 2:41 AM

The functionality for getting process mappings on FreeBSD is exposed via libprocstat. Is there a reason for this code not to use this library?

The functionality for getting process mappings on FreeBSD is exposed via libprocstat. Is there a reason for this code not to use this library?

That library relies on sysctl(KERN_PROC_VMMAP) so we would need to workaround it anyway.

I'm also not happy with requiring read access to /dev/kmem. This is a serious security hole. If the only issue is performance, then I think I would rather live with it being slow than insecure and fix it upstream.

I'm also not happy with requiring read access to /dev/kmem. This is a serious security hole. If the only issue is performance, then I think I would rather live with it being slow than insecure and fix it upstream.

Well, it's not just somewhat slower than the sysctl() version; the problem is that the call consumes so much time that it makes impossible to run the tests in practice.

This patch is a temporary solution intended to allow us to continue the work on adding FreeBSD support for sanitizers. It's OK if we do not put this patch to the mainline, but then the FreeBSD builbot will further skip running tests unless the issue with sysctl() is fixed and backported to v9.2 .

The response from the FreeBSD core group is that they wouldn't like this patch to be committed due to security reasons, but rather elaborate a kernel fix.

The related activity can be seen at:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=188911

Once we get a working fix, the FreeBSD bot will be patched respectively.

The linked FreeBSD bug now has a patch attached. Please test and let us know if it fixes the issue.

The kernel patch provided by Konstantin Belousov against FreeBSD HEAD is confirmed to solve the issue with sysctl(KERN_PROC_VMMAP) on Asan unit tests. Similar patch for stable/9 is expected to be prepared in a couple of weeks.

kutuzov.viktor.84 abandoned this revision.Aug 8 2014, 2:00 AM

Here's the patch for FreeBSD 9.2:

Once the kernel is patched, new kernel state named 'kern.proc_vmmap_skip_resident_count' should be available. That state must be set to 1 before running Asan tests:

sysctl kern.proc_vmmap_skip_resident_count=1

Note that the patch is committed to stable/9 as r269090 so there is no need to apply the patch to subsequent releases.

Also, a more sophisticated version of the patch (not applicable to v9 sources) is committed to HEAD as r268466. That patch differs from the stable/9 patch so that it makes the Asan tests passing in reasonable/usual amount of time regardless of the value of the kern.proc_vmmap_skip_resident_count state.