sched_setscheduler: fix? policy checks
PATCH 4/4 sched_setscheduler: fix? policy checks
I am not sure this patch is correct: I can't understand what the current code does, and I don't know what it was supposed to do.
The comment says:
* can't change policy, except between SCHED_NORMAL
* and SCHED_BATCH:
The code:
if (((policy != SCHED_NORMAL && p->policy != SCHED_BATCH) && (policy != SCHED_BATCH && p->policy != SCHED_NORMAL)) &&
But this is equivalent to:
if ( (is_rt_policy(policy) && has_rt_policy(p)) &&
which means something different.
Probably, it was supposed to be:
if ( !(policy == SCHED_NORMAL && p->policy == SCHED_BATCH) && !(policy == SCHED_BATCH && p->policy == SCHED_NORMAL)
this matches the comment, but strange: it doesn't allow to _drop_ the realtime priority when rlim[RLIMIT_RTPRIO] == 0.
I think the right check would be:
if (is_rt_policy(policy) && !has_rt_policy(p) && !rlim_rtprio) return -EPERM;
but this is covered by the "/* can't increase priority */" check below, because
is_rt_policy(policy) => ->sched_priority > 0 !has_rt_policy(p) => p->rt_priority == 0
So I think we can just remove this code.
kernel/sched.c renamed from kernel/sched.c~4_policy +13 -19
@@ -4079,27 +4079,21 @@ recheck:* Allow unprivileged RT tasks to decrease priority: */ if (!capable (CAP_SYS_NICE )) { -unsigned long rlim_rtprio; -unsigned long flags; +if (is_rt_policy (policy)) { +unsigned long rlim_rtprio; +unsigned long flags; -if (!lock_task_sighand (p, &flags)) -return -ESRCH ; - rlim_rtprio = p->signal->rlim[RLIMIT_RTPRIO ].rlim_cur; -unlock_task_sighand (p, &flags); +if (!lock_task_sighand (p, &flags)) +return -ESRCH ; + rlim_rtprio = p->signal->rlim[RLIMIT_RTPRIO ].rlim_cur; +unlock_task_sighand (p, &flags); + +/* can't increase priority */ +if (param->sched_priority > p->rt_priority && + param->sched_priority > rlim_rtprio) +return -EPERM ; + } -/* -* can't change policy, except between SCHED_NORMAL -* and SCHED_BATCH: -*/ -if (((policy !=SCHED_NORMAL && p->policy !=SCHED_BATCH ) && - (policy !=SCHED_BATCH && p->policy !=SCHED_NORMAL )) && - !rlim_rtprio) -return -EPERM ; -/* can't increase priority */ -if (is_rt_policy (policy) && - param->sched_priority > p->rt_priority && - param->sched_priority > rlim_rtprio) -return -EPERM ;/* can't change other user's priorities */ if ((current->euid != p->euid) && (current->euid != p->uid))
Quoted 2 lines · Oleg Nesterov
I am not sure this patch is correct:
Surely it is not correct. Another attempt ...
[PATCH 4/4] sched_setscheduler: fix? policy checks
I am not sure this patch is correct: I can't understand what the current code does, and I don't know what it was supposed to do.
The comment says:
* can't change policy, except between SCHED_NORMAL
* and SCHED_BATCH:
The code:
if (((policy != SCHED_NORMAL && p->policy != SCHED_BATCH) && (policy != SCHED_BATCH && p->policy != SCHED_NORMAL)) &&
But this is equivalent to:
if ( (is_rt_policy(policy) && has_rt_policy(p)) &&
which means something different. We can't _decrease_ the current ->rt_priority with such a check (if rlim[RLIMIT_RTPRIO] == 0).
Probably, it was supposed to be:
if ( !(policy == SCHED_NORMAL && p->policy == SCHED_BATCH) && !(policy == SCHED_BATCH && p->policy == SCHED_NORMAL)
this matches the comment, but strange: it doesn't allow to _drop_ the realtime priority when rlim[RLIMIT_RTPRIO] == 0.
I think the right check would be:
/* can't set/change rt policy */ if (is_rt_policy(policy) && policy != p->policy && !rlim_rtprio) return -EPERM;
kernel/sched.c renamed from kernel/sched.c~4_policy +17 -19
@@ -4079,27 +4079,25 @@ recheck:* Allow unprivileged RT tasks to decrease priority: */ if (!capable (CAP_SYS_NICE )) { -unsigned long rlim_rtprio; -unsigned long flags; +if (is_rt_policy (policy)) { +unsigned long rlim_rtprio; +unsigned long flags; -if (!lock_task_sighand (p, &flags)) -return -ESRCH ; - rlim_rtprio = p->signal->rlim[RLIMIT_RTPRIO ].rlim_cur; -unlock_task_sighand (p, &flags); +if (!lock_task_sighand (p, &flags)) +return -ESRCH ; + rlim_rtprio = p->signal->rlim[RLIMIT_RTPRIO ].rlim_cur; +unlock_task_sighand (p, &flags); + +/* can't set/change the rt policy */ +if (policy != p->policy && !rlim_rtprio) +return -EPERM ; + +/* can't increase priority */ +if (param->sched_priority > p->rt_priority && + param->sched_priority > rlim_rtprio) +return -EPERM ; + } -/* -* can't change policy, except between SCHED_NORMAL -* and SCHED_BATCH: -*/ -if (((policy !=SCHED_NORMAL && p->policy !=SCHED_BATCH ) && - (policy !=SCHED_BATCH && p->policy !=SCHED_NORMAL )) && - !rlim_rtprio) -return -EPERM ; -/* can't increase priority */ -if (is_rt_policy (policy) && - param->sched_priority > p->rt_priority && - param->sched_priority > rlim_rtprio) -return -EPERM ;/* can't change other user's priorities */ if ((current->euid != p->euid) && (current->euid != p->uid))