Uh oh!
There was an error while loading. Please reload this page.
Use exec_attr to gain proc_lock_memory priv on illumos - #104292
Conversation
Or else define _STRUCTURED_PROC = 1 as described in sys/procfs.h Also untangle from the NetBSD ifdef section.
| // PAL_InitializeCoreCLR / InitializeFlushProcessWriteBuffers | ||
| // | ||
| // This privilege is added to the "inheritable" set via the exec_attr(5) | ||
| // by adding a file like: /etc/security/exec_attr.d/dotnet containing: |
There was a problem hiding this comment.
Expecting user to create this entry feels like a similar workaround as asking them to run sudo usermod -K defaultpriv='basic,proc_lock_memory' $(whoami) (current workaround). I think we should find a better way to avoid calling mlock(), as we do for macOS and other membarrier() supported platforms.
We were discussing about fork et al. based solution, did it not work?
There was a problem hiding this comment.
I am looking at better options, but want to get through self hosting before I invest more time in that.
The exec_attr method is a bit easier for me to deal with for testing, but I guess I could just keep this change locally for now.
There was a problem hiding this comment.
Lets keep this patch in local branch. I don't think it is a good idea to trade one semi-finished solution with another of a kind. We had a long discussion here: #36632.
There was a problem hiding this comment.
OK, let's just close this then.
There was a problem hiding this comment.
Alternatively, we can mark it as "draft" (there is a button visible for you on this page). In case we can't avoid mlock() this solution is better for packaging as you explained on IRC.
| } | ||
| #endif // __APPLE__ | ||
| #if defined(__sun) |
There was a problem hiding this comment.
Might be able to use TARGET_SUNOS here too. (All __sun in this file)
There was a problem hiding this comment.
I think it's ok to leave it as __sun if nearby context has the similar usage.
| #endif | ||
| #include <sys/procfs.h> | ||
| #ifdef UNDEF_KERNEL | ||
| #undef _KERNEL |
There was a problem hiding this comment.
There are three places where we had to use this workaround for a reason (I think some missing definition or behavior difference):
$ git grep -n "undef _KERNEL"
src/coreclr/pal/src/init/pal.cpp:86:#undef _KERNEL
src/coreclr/pal/src/thread/thread.cpp:45:#undef _KERNEL
src/native/libs/System.Native/pal_io.c:69:#undef _KERNELMaybe we should revisit them as well if you are sure that _KERNEL is not needed now?
Please make sure cross-build scenario is working with this change, otherwise it gets very complicated as we don't have the illumos SDK.
There was a problem hiding this comment.
The other locations should have the same fix. All are due to the mistake of using <sys/procfs.h> where just <procfs.h> should be used. Would it be easier to have that change in a separate PR and fix all three places? I'm happy to try that and put up a PR.
There was a problem hiding this comment.
Sounds good. Note that I'm not a maintainer. So I'm just giving what "I think" makes sense, which may or may not be the final word when maintainers will review these changes. :)
On Solaris and illumos, calling
mlock(2)requiresproc_lock_memoryprivilege.Without that privilege,
mlock()returns EPERM.We can use fine-grained
privileges(7)on these systems to grant this privilege.See the comments in the code on how one sets that up.
This is helpful until we find a way to get rid of the
mlock()page strategy,either implementing somethings like
membarrier(2)or something else.Investigation of that is underway but may take a while.
While I'm here, fix the procfs.h include. Applications should use <procfs.h> not <sys/procfs.h>
That's why the temporary
#define _KERNELmess was there. With that fix, the mess is cleaned up.