| Message ID | YG7Q5C4Rb5dx5GFx@hirez.programming.kicks-ass.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix sys_ioprio_set(.which=IOPRIO_WHO_PGRP) task iteration | expand |
On 4/8/21 3:46 AM, Peter Zijlstra wrote: > > do_each_pid_thread() { } while_each_pid_thread() is a double loop and > thus break doesn't work as expected. Also, it should be used under > tasklist_lock because otherwise we can race against change_pid() for > PGID/SID. Applied, thanks.
On 04/08, Jens Axboe wrote: > > On 4/8/21 3:46 AM, Peter Zijlstra wrote: > > > > do_each_pid_thread() { } while_each_pid_thread() is a double loop and > > thus break doesn't work as expected. Also, it should be used under > > tasklist_lock because otherwise we can race against change_pid() for > > PGID/SID. > > Applied, thanks. Agreed, but can't resist. We can move the "out" label up and avoid the extra read_unlock(tasklist_lock). IOW, something like below on top of this patch. Quite possibly this won't change the generated code, gcc is smart enough, but this makes the code a bit more readable. Oleg. --- x/block/ioprio.c~ 2021-04-09 12:00:28.066145563 +0200 +++ x/block/ioprio.c 2021-04-09 12:02:01.817849618 +0200 @@ -123,11 +123,10 @@ read_lock(&tasklist_lock); do_each_pid_thread(pgrp, PIDTYPE_PGID, p) { ret = set_task_ioprio(p, ioprio); - if (ret) { - read_unlock(&tasklist_lock); - goto out; - } + if (ret) + goto out_pgrp; } while_each_pid_thread(pgrp, PIDTYPE_PGID, p); +out_pgrp: read_unlock(&tasklist_lock); break; @@ -159,7 +158,6 @@ ret = -EINVAL; } -out: rcu_read_unlock(); return ret; }
--- a/block/ioprio.c +++ b/block/ioprio.c @@ -119,11 +119,17 @@ SYSCALL_DEFINE3(ioprio_set, int, which, 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; + if (ret) { + read_unlock(&tasklist_lock); + goto out; + } } while_each_pid_thread(pgrp, PIDTYPE_PGID, p); + read_unlock(&tasklist_lock); + break; case IOPRIO_WHO_USER: uid = make_kuid(current_user_ns(), who); @@ -153,6 +159,7 @@ SYSCALL_DEFINE3(ioprio_set, int, which, ret = -EINVAL; } +out: rcu_read_unlock(); return ret; }
do_each_pid_thread() { } while_each_pid_thread() is a double loop and thus break doesn't work as expected. Also, it should be used under tasklist_lock because otherwise we can race against change_pid() for PGID/SID. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- block/ioprio.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)