linux-kernel - Re: [PATCH RT] locking: Make spinlock_t and rwlock

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]

Date:   Fri, 22 Nov 2019 19:01:40 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        linux-rt-users <linux-rt-users@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Paul E. McKenney" <paulmck@...nel.org>
Subject: Re: [PATCH RT] locking: Make spinlock_t and rwlock_t a RCU section
 on RT

On 2019-11-19 09:21:49 [-0500], Steven Rostedt wrote:
> On Tue, 19 Nov 2019 09:46:40 +0100
> Sebastian Andrzej Siewior <bigeasy@...utronix.de> wrote:
> 
> > On !RT a locked spinlock_t and rwlock_t disables preemption which
> > implies a RCU read section. There is code that relies on that behaviour.
> > 
> > Add an explicit RCU read section on RT while a sleeping lock (a lock
> > which would disables preemption on !RT) acquired.
> 
> I know that there was some work to merge the RCU flavors of
> rcu_read_lock and rcu_read_lock_sched, I'm assuming this depends on
> that behavior. That is, a synchronize_rcu() will wait for all CPUs to
> schedule and all grace periods to finish, which means that those using
> rcu_read_lock() and those using all CPUs to schedule can be
> interchangeable. That is, on !RT, it's likely that rcu_read_lock()
> waiters will end up waiting for all CPUs to schedule, and on RT, this
> makes it where those waiting for all CPUs to schedule, will also wait
> for all rcu_read_lock()s grace periods to finish. If that's the case,
> then this change is fine. But it depends on that being the case, which
> it wasn't in older kernels, and we need to be careful about backporting
> this.

Let me give you an example how I got into this:

do_sigaction() acquires p->sighand->siglock and then iterates over list
via for_each_thread() which is a list_for_each_entry_rcu(). No RCU lock
is held, just the siglock.
On removal side, __unhash_process() removes a task from the list but
while doing so it holds the siglock and tasklist_lock. So it is
perfectly fine.
Later, we have:
|do_exit()
| -> exit_notify()
|   -> write_lock_irq(&tasklist_lock);
|   -> forget_original_parent()
|      -> find_child_reaper()
|        -> find_alive_thread()
|           -> for_each_thread()

find_alive_thread() does the for_each_thread() and checks PF_EXITING.
it might be enough for not operating on "removed" task_struct. It
dereferences task_struct->flags while looking for PF_EXITING. At this
point only tasklist_lock is acquired.
I have *no* idea if the whole synchronisation based on siglock/
PF_EXITING/ tasklist_lock is enough and RCU simply doesn't matter. It
seems so.

I am a little worried if this construct here (or somewhere else) assumes
that holding one of those locks, which disable preemption, is the same
as rcu_read_lock() (or rcu_read_lock_sched()).

> -- Steve

Sebastian