linux-kernel - Re: [PATCH 1/2] kernel/sys: only rely on rcu for getpriority(2)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200512165824.t6ktwllqlvkiingv@linux-p48b>
Date:   Tue, 12 May 2020 09:58:24 -0700
From:   Davidlohr Bueso <dave@...olabs.net>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     akpm@...ux-foundation.org, peterz@...radead.org,
        paulmck@...nel.org, tglx@...utronix.de,
        linux-kernel@...r.kernel.org, Davidlohr Bueso <dbueso@...e.de>
Subject: Re: [PATCH 1/2] kernel/sys: only rely on rcu for getpriority(2)

On Tue, 12 May 2020, Oleg Nesterov wrote:

>On 05/12, Davidlohr Bueso wrote:
>>
>> On Tue, 12 May 2020, Oleg Nesterov wrote:
>>
>> >do_each_pid_task(PIDTYPE_PGID) can race with change_pid(PIDTYPE_PGID)
>> >which moves the task from one hlist to another. Yes, it is safe in
>> >that task_struct can't go away. But still this is not right because
>> >do_each_pid_task() can scan the wrong (2nd) hlist.
>>
>> Hmm I didn't think about this case, I guess this is also busted in
>> ioprio_get(2) then.
>
>agreed...
>
>> >
>> >could you explain in details why do you think this PF_EXITING check
>> >makes any sense?
>>
>> My logic was that if the task with the highest prio exited while we
>> were iterating the list, it would not be necessarily seen with rcu
>> and the syscall would return the highest prio of a task that exited;
>> and checking against PF_EXITING was a way to ignore such scenarios
>> as we were going to race with it anyway.
>
>Sorry, still can't understand. The PF_EXITING flag is not protected by
>tasklist_lock or rcu_lock.

Sorry for not making my idea clear, perhaps it's complete garbage.

Right, but setting the flag is an indication that the tasklist_lock
will be taken and removed from the list, and therefore we could
optimistically avoid considering that task altogether instead of
relying on the old copy of the list. It's not perfect, but it does
reduce the window in which getpriority() can return a stale value(?).

At least this is how I justify it. Otoh this also opens a window in
where the lockless version can ignore highest prio task when the locked
version would otherwise consider it. So it might not be worth it.

>
>OK, if nothing else. Suppose that a prgp has a single process P, this
>proces has already exited but its parent didn't do wait().
>
>Currently getpriority() returns task_nice(P). With the PF_EXITING check
>it will return -ESRCH. Hmm?

Yes, that would need fixing but you don't seem to be buying the idea
in the first place.

Thanks,
Davidlohr