RFR : 8215962: Support ThreadPriorityPolicy mode 1 for non-root users on linux/bsd
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Jan 2 23:36:18 UTC 2019
On 1/2/19 5:35 PM, David Holmes wrote:
> Hi Dan,
>
> On 3/01/2019 2:50 am, Daniel D. Daugherty wrote:
>> 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;
>> }
>> }
>
> Sorry I disagree. The existing code checks for policy and whether root
> and issues a warning then resets policy.
You have to be careful with the word 'policy' here. You are actually
talking about the 'ThreadPriorityPolicy' option here and not the
security policy associated with the setpriority() call. One thing
the application code cannot do here is reset the underlying
security policy.
> The new code does exactly the same thing except it doesn't reset the
> policy. The placement of the warning was fine before so it is fine now.
Actually I disagree that the placement of the warning was fine before.
As I said, in my original review comment:
> 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 application code should not make the assumption that it knows the
underlying security policy of the setpriority() call. That is a basic
principle of Trusted Systems design. The best you can do is try the
operation and if it fails, then try to issue a possibly helpful message
based on the error that is returned. There are some Trusted Systems
folks that don't believe in trying to interpret errno values either.
Why? Because you can only code the errno values that you know today that
are security policy related. You can't know if some system down the road
will add a new errno value that's security related...
My proposed do_set_native_prio_warning() function should actually take
an errno parameter and it should only issue the warning if the errno is
EACCES or EPERM.
Another problem with the new code is that:
warning("-XX:ThreadPriorityPolicy requires root privilege or the
CAP_SYS_NICE capability.");
will be issued when the user != root and the thread has the
CAP_SYS_NICE capability so we'll be issuing a warning even
though the setpriority() call should succeed. I don't think
a false warning is acceptable.
By moving the warning to where setpriority() has failed, we no longer
would have the problem of a false warning.
> Yes it could go on use (and add a only-warn-once hack) but why force
> such a disruptive change that has no benefit?
As I've pointed out above, I do think it has benefit and it meets
a Trusted System design principle. As for the mechanism to
only-warn-once, I'm sorry you consider it a hack. I consider it
to be a useful way to avoid swamping the warning output with the
same message. I know HotSpot does the same thing in other places
so if it is a hack, then it is in good company. :-)
Dan
>
> Cheers,
> David
>
>> 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