RFR : 8215962: Support ThreadPriorityPolicy mode 1 for non-root users on linux/bsd
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Jan 2 16:50:19 UTC 2019
On 1/2/19 4:11 AM, Baesken, Matthias wrote:
> Hello , please review the following patch .
>
> Currently, when ThreadPriorityPolicy is set to 1 (so called "Aggressive mode"), on linux and bsd(+Mac) a root-user-check (geteuid() != 0)) is done.
> See for example the coding in jdk/src/hotspot/os/linux/os_linux.cpp int prio_init().
>
> However the root-user-check has a few drawbacks:
> - it blocks the capabilities feature available on current Linux distros (CAP_SYS_NICE capability) that can be used to allow setting lower niceness also for non-root
> - setting a higher "niceness" (lower priority) is not possible on Linux for non-root because of the geteuid check
>
> We had a discussion about this in "ThreadPriorityPolicy settings for non-root users" , with this suggestion :
>
> https://mail.openjdk.java.net/pipermail/hotspot-dev/2018-December/035986.html
>
> ....
>
>> Just drop the root check for ThreadPriorityPolicy=1 and let the underlying system
>> permissions control success or failure.
> I did the change in this webrev :
>
>
> Bug/webrev :
>
> https://bugs.openjdk.java.net/browse/JDK-8215962
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8215962.0/
General
- Please update Copyright years to 2019 before pushing.
src/hotspot/share/runtime/globals.hpp
L2045: " Linux/BSD/Mac this policy requires root
privilege or "\
Typo: "Mac" should be "macOS".
L2046: " extended
capabilites.") \
Typo: "capabilites" -> "capabilities."
Please consider: "an extended capability." since it is normal
for only a single capability to be required for a specific
policy override.
src/hotspot/os/bsd/os_bsd.cpp
L2260: // CAP_SYS_NICE capabilities
Typo: "capabilities" -> "capability." (note the added period)
L2306: if (ThreadPriorityPolicy == 1) {
L2307: // root and threads with capability CAP_SYS_NICE can
raise the thread priority
L2308: // however testing the CAP_SYS_NICE capability would
require libcap.so
L2309: if (geteuid() != 0) {
L2310: if (!FLAG_IS_DEFAULT(ThreadPriorityPolicy)) {
L2311: warning("-XX:ThreadPriorityPolicy requires root
privilege or CAP_SYS_NICE capability on Bsd");
L2312: }
L2313: }
L2314: }
Sorry this whole block is the wrong thing to do. It makes the
assumption that it "knows" the underlying security policy and
tries to provide a "helpful" message in anticipation that the
underlying security policy will reject the operation.
The "(ThreadPriorityPolicy == 1) {" if-block needs to be deleted.
If you want to output a helpful warning, then you need to do it in
the code that will actually get a policy failure:
static void do_set_native_prio_warning() {
static bool has_warned = false;
if (!has_warned) {
warning("-XX:ThreadPriorityPolicy requires root privilege or
the CAP_SYS_NICE capability.");
has_warned = true;
}
}
L2321: OSReturn os::set_native_priority(Thread* thread, int newpri) {
L2322: if (!UseThreadPriorities || ThreadPriorityPolicy == 0)
return OS_OK;
L2323:
L2324: #ifdef __OpenBSD__
L2325: // OpenBSD pthread_setprio starves low priority threads
L2326: return OS_OK;
L2327: #elif defined(__FreeBSD__)
L2328: int ret =
pthread_setprio(thread->osthread()->pthread_id(), newpri);
// Note the __FreeBSD__ branch here is broken; it is missing the return
sequence.
if (ret != 0) {
do_set_native_prio_warning();
return OS_ERR;
}
return OS_OK;
L2329: #elif defined(__APPLE__) || defined(__NetBSD__)
L2330: struct sched_param sp;
L2331: int policy;
L2332:
L2333: if
(pthread_getschedparam(thread->osthread()->pthread_id(), &policy, &sp)
!= 0) {
do_set_native_prio_warning();
L2334: return OS_ERR;
L2335: }
L2336:
L2337: sp.sched_priority = newpri;
L2338: if
(pthread_setschedparam(thread->osthread()->pthread_id(), policy, &sp) !=
0) {
do_set_native_prio_warning();
L2339: return OS_ERR;
L2340: }
L2341:
L2342: return OS_OK;
L2343: #else
L2344: int ret = setpriority(PRIO_PROCESS,
thread->osthread()->thread_id(), newpri);
if (ret != 0) {
do_set_native_prio_warning();
return OS_ERR;
}
return OS_OK; // replace L2345 with this line.
L2345: return (ret == 0) ? OS_OK : OS_ERR;
L2346: #endif
L2347: }
src/hotspot/os/linux/os_linux.cpp
L4080: // CAP_SYS_NICE capabilities
Typo: "capabilities" -> "capability." (note the added period)
The same comment about prio_init() applies here:
L4103-L4111:
The "(ThreadPriorityPolicy == 1) {" if-block needs to be deleted.
static void do_set_native_prio_warning() {
static bool has_warned = false;
if (!has_warned) {
warning("-XX:ThreadPriorityPolicy requires root privilege or
the CAP_SYS_NICE capability.");
has_warned = true;
}
}
But the changes to os::set_native_priority() are much simpler:
L4118: OSReturn os::set_native_priority(Thread* thread, int newpri) {
L4119: if (!UseThreadPriorities || ThreadPriorityPolicy == 0)
return OS_OK;
L4120:
L4121: int ret = setpriority(PRIO_PROCESS,
thread->osthread()->thread_id(), newpri);
if (ret != 0) {
do_set_native_prio_warning();
return OS_ERR;
}
return OS_OK; // replace L4122 with this line.
L4122: return (ret == 0) ? OS_OK : OS_ERR;
L4123: }
In both os/bsd/os_bsd.cpp and os/linux/os_linux.cpp, the
os::get_native_priority() code allows for the possibility
of getting an error condition for getpriority(). I don't
think we need a do_get_native_prio_warning() function here
since the only threads we should be querying belong to the
Java process so they should not fail the policy check.
Dan
>
>
> Best Regards , Matthias
More information about the hotspot-dev
mailing list