2020-08-17 01:43:31

by Davidlohr Bueso

Subject: [PATCH -next 0/2] tasklist_lock vs get/set priority syscalls

Hi,

This is a (late) update on trying to reduce some of the scope of the tasklist_lock
for get/setpriority(2) as well as the block io equivalent. This version addresses
Oleg's previous concerns and incorporates his feedback.

Changes from v1:
https://lore.kernel.org/lkml/[email protected]/

- only take the lock for PGID cases.
- drop bogus PF_EXITING checks (and live with the small exit race).
- add patch for IOPRIO_WHO_PGRP.

Please consider for v5.10.

Thanks!

Davidlohr Bueso (2):
kernel/sys: only take tasklist_lock for get/setpriority(PRIO_PGRP)
block: fix ioprio_get/set(IOPRIO_WHO_PGRP) vs setuid(2)

block/ioprio.c | 4 ++++
kernel/sys.c | 16 ++++++++--------
2 files changed, 12 insertions(+), 8 deletions(-)

--
2.26.2

2020-08-17 01:45:06

by Davidlohr Bueso

Subject: [PATCH 2/2] block: fix ioprio_get/set(IOPRIO_WHO_PGRP) vs setuid(2)

do_each_pid_thread(PIDTYPE_PGID) can race with a concurrent
change_pid(PIDTYPE_PGID) that can move the task from one hlist
to another while iterating. Serialize ioprio_get/set to take
the tasklist_lock in this case.

Fixes: d69b78ba1de (ioprio: grab rcu_read_lock in sys_ioprio_{set,get}())
Cc: Greg Thelen <[email protected]>
Signed-off-by: Davidlohr Bueso <[email protected]>
---
block/ioprio.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/block/ioprio.c b/block/ioprio.c
index 77bcab11dce5..4ede2da961bb 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -119,11 +119,13 @@ SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
pgrp = task_pgrp(current);
else
pgrp = find_vpid(who);
+ read_lock(&tasklist_lock);
do_each_pid_thread(pgrp, PIDTYPE_PGID, p) {
ret = set_task_ioprio(p, ioprio);
if (ret)
break;
} while_each_pid_thread(pgrp, PIDTYPE_PGID, p);
+ read_unlock(&tasklist_lock);
break;
case IOPRIO_WHO_USER:
uid = make_kuid(current_user_ns(), who);
@@ -207,6 +209,7 @@ SYSCALL_DEFINE2(ioprio_get, int, which, int, who)
pgrp = task_pgrp(current);
else
pgrp = find_vpid(who);
+ read_lock(&tasklist_lock);
do_each_pid_thread(pgrp, PIDTYPE_PGID, p) {
tmpio = get_task_ioprio(p);
if (tmpio < 0)
@@ -216,6 +219,7 @@ SYSCALL_DEFINE2(ioprio_get, int, which, int, who)
else
ret = ioprio_best(ret, tmpio);
} while_each_pid_thread(pgrp, PIDTYPE_PGID, p);
+ read_unlock(&tasklist_lock);
break;
case IOPRIO_WHO_USER:
uid = make_kuid(current_user_ns(), who);
--
2.26.2

2020-08-17 14:11:08

by Oleg Nesterov

Subject: Re: [PATCH 2/2] block: fix ioprio_get/set(IOPRIO_WHO_PGRP) vs setuid(2)

On 08/16, Davidlohr Bueso wrote:
>
> --- a/block/ioprio.c
> +++ b/block/ioprio.c
> @@ -119,11 +119,13 @@ SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
> pgrp = task_pgrp(current);
> else
> pgrp = find_vpid(who);
> + read_lock(&tasklist_lock);
> do_each_pid_thread(pgrp, PIDTYPE_PGID, p) {
> ret = set_task_ioprio(p, ioprio);
> if (ret)
> break;
> } while_each_pid_thread(pgrp, PIDTYPE_PGID, p);
> + read_unlock(&tasklist_lock);
> break;
> case IOPRIO_WHO_USER:
> uid = make_kuid(current_user_ns(), who);
> @@ -207,6 +209,7 @@ SYSCALL_DEFINE2(ioprio_get, int, which, int, who)
> pgrp = task_pgrp(current);
> else
> pgrp = find_vpid(who);
> + read_lock(&tasklist_lock);
> do_each_pid_thread(pgrp, PIDTYPE_PGID, p) {
> tmpio = get_task_ioprio(p);
> if (tmpio < 0)
> @@ -216,6 +219,7 @@ SYSCALL_DEFINE2(ioprio_get, int, which, int, who)
> else
> ret = ioprio_best(ret, tmpio);
> } while_each_pid_thread(pgrp, PIDTYPE_PGID, p);
> + read_unlock(&tasklist_lock);

Acked-by: Oleg Nesterov <[email protected]>

2020-08-17 14:15:04

by Jens Axboe

Subject: Re: [PATCH 2/2] block: fix ioprio_get/set(IOPRIO_WHO_PGRP) vs setuid(2)

On 8/16/20 5:31 PM, Davidlohr Bueso wrote:
> do_each_pid_thread(PIDTYPE_PGID) can race with a concurrent
> change_pid(PIDTYPE_PGID) that can move the task from one hlist
> to another while iterating. Serialize ioprio_get/set to take
> the tasklist_lock in this case.

LGTM:

Reviewed-by: Jens Axboe <[email protected]>

--
Jens Axboe

2020-09-04 22:57:59

by Davidlohr Bueso

Subject: Re: [PATCH -next 0/2] tasklist_lock vs get/set priority syscalls

On Sun, 16 Aug 2020, Davidlohr Bueso wrote:

>Hi,
>
>This is a (late) update on trying to reduce some of the scope of the tasklist_lock
>for get/setpriority(2) as well as the block io equivalent. This version addresses
>Oleg's previous concerns and incorporates his feedback.
>
>Changes from v1:
>https://lore.kernel.org/lkml/[email protected]/
>
> - only take the lock for PGID cases.
> - drop bogus PF_EXITING checks (and live with the small exit race).
> - add patch for IOPRIO_WHO_PGRP.
>
>Please consider for v5.10.

Andrew, unless you have any objections, could you please pick these up?

Thanks,
Davidlohr