RFR : 8215962: Support ThreadPriorityPolicy mode 1 for non-root users on linux/bsd

[David Holmes]

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. 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. Yes it could go on use (and 
add a only-warn-once hack) but why force such a disruptive change that 
has no benefit?

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
>