History

kernel/sys.c: only take tasklist_lock for get/setpriority(PRIO_PGRP)

PRIO_PGRP needs the tasklist_lock mainly to serialize vs setpgid(2), to protect against any concurrent change_pid(PIDTYPE_PGID) that can move the task from one hlist to another while iterating. However, the remaining can only rely only on RCU: PRIO_PROCESS only does the task lookup and never iterates over tasklist and we already have an rcu-aware stable pointer. PRIO_USER is already racy vs setuid(2) so with creds being rcu protected, we can end up seeing stale data. When removing the tasklist_lock there can be a race with (i) fork but this is benign as the child's nice is inherited and the new task is not observable by the user yet either, hence the return semantics do not differ. And (ii) a race with exit, which is a small window and can cause us to miss a task which was removed from the list and it had the highest nice. Similarly change the buggy do_each_thread/while_each_thread combo in PRIO_USER for the rcu-safe for_each_process_thread flavor, which doesn't make use of next_thread/p->thread_group. [akpm@linux-foundation.org: coding style fixes]

Davidlohr Bueso committed 4 years ago

Commit 7f8ca0e1 parent d6986ce1 file · +8 −8Browse files

Unified · Split

kernel/sys.c+8−8
@@ -220,7 +220,6 @@ SYSCALL_DEFINE3(setpriority, int, which, int, who, int, niceval) · 219 unmodified lines
220220 niceval = MAX_NICE;
221221
222222 rcu_read_lock();
223− read_lock(&tasklist_lock);
224223 switch (which) {
225224 case PRIO_PROCESS:
226225 if (who)
@@ -235,9 +234,11 @@ SYSCALL_DEFINE3(setpriority, int, which, int, who, int, niceval) · 8 unmodified lines
235234 pgrp = find_vpid(who);
236235 else
237236 pgrp = task_pgrp(current);
237+ read_lock(&tasklist_lock);
238238 do_each_pid_thread(pgrp, PIDTYPE_PGID, p) {
239239 error = set_one_prio(p, niceval, error);
240240 } while_each_pid_thread(pgrp, PIDTYPE_PGID, p);
241+ read_unlock(&tasklist_lock);
241242 break;
242243 case PRIO_USER:
243244 uid = make_kuid(cred->user_ns, who);
@@ -249,16 +250,15 @@ SYSCALL_DEFINE3(setpriority, int, which, int, who, int, niceval) · 5 unmodified lines
249250 if (!user)
250251 goto out_unlock; /* No processes for this user */
251252 }
252− do_each_thread(g, p) {
253+ for_each_process_thread(g, p) {
253254 if (uid_eq(task_uid(p), uid) && task_pid_vnr(p))
254255 error = set_one_prio(p, niceval, error);
255− } while_each_thread(g, p);
256+ }
256257 if (!uid_eq(uid, cred->uid))
257258 free_uid(user); /* For find_user() */
258259 break;
259260 }
260261 out_unlock:
261− read_unlock(&tasklist_lock);
262262 rcu_read_unlock();
263263 out:
264264 return error;
@@ -283,7 +283,6 @@ SYSCALL_DEFINE2(getpriority, int, which, int, who) · 18 unmodified lines
283283 return -EINVAL;
284284
285285 rcu_read_lock();
286− read_lock(&tasklist_lock);
287286 switch (which) {
288287 case PRIO_PROCESS:
289288 if (who)
@@ -301,11 +300,13 @@ SYSCALL_DEFINE2(getpriority, int, which, int, who) · 11 unmodified lines
301300 pgrp = find_vpid(who);
302301 else
303302 pgrp = task_pgrp(current);
303+ read_lock(&tasklist_lock);
304304 do_each_pid_thread(pgrp, PIDTYPE_PGID, p) {
305305 niceval = nice_to_rlimit(task_nice(p));
306306 if (niceval > retval)
307307 retval = niceval;
308308 } while_each_pid_thread(pgrp, PIDTYPE_PGID, p);
309+ read_unlock(&tasklist_lock);
309310 break;
310311 case PRIO_USER:
311312 uid = make_kuid(cred->user_ns, who);
@@ -317,19 +318,18 @@ SYSCALL_DEFINE2(getpriority, int, which, int, who) · 5 unmodified lines
317318 if (!user)
318319 goto out_unlock; /* No processes for this user */
319320 }
320− do_each_thread(g, p) {
321+ for_each_process_thread(g, p) {
321322 if (uid_eq(task_uid(p), uid) && task_pid_vnr(p)) {
322323 niceval = nice_to_rlimit(task_nice(p));
323324 if (niceval > retval)
324325 retval = niceval;
325326 }
326− } while_each_thread(g, p);
327+ }
327328 if (!uid_eq(uid, cred->uid))
328329 free_uid(user); /* for find_user() */
329330 break;
330331 }
331332 out_unlock:
332− read_unlock(&tasklist_lock);
333333 rcu_read_unlock();
334334
335335 return retval;