Re: [PATCH v2] mm: memory-failure: use rcu lock instead of tasklist_lock when collect_procs()

[Matthew Wilcox]

On Tue, Aug 22, 2023 at 11:41:41AM +0800, Tong Tiangen wrote:
> 在 2023/8/22 2:33, Matthew Wilcox 写道:
> > On Mon, Aug 21, 2023 at 05:13:12PM +0800, Tong Tiangen wrote:
> > > We can see that CPU1 waiting for CPU0 respond IPI，CPU0 waiting for CPU2
> > > unlock tasklist_lock, CPU2 waiting for CPU1 unlock page->ptl. As a result,
> > > softlockup is triggered.
> > > 
> > > For collect_procs_anon(), we will not modify the tasklist, but only perform
> > > read traversal. Therefore, we can use rcu lock instead of spin lock
> > > tasklist_lock, from this, we can break the softlock chain above.
> > 
> > The only thing that's giving me pause is that there's no discussion
> > about why this is safe.  "We're not modifying it" isn't really enough
> > to justify going from read_lock() to rcu_read_lock().  When you take a
> > normal read_lock(), writers are not permitted and so you see an atomic
> > snapshot of the list.  With rcu_read_lock() you can see inconsistencies.
> 
> Hi Matthew:
> 
> When rcu_read_lock() is used, the task list can be modified during the
> iteration, but cannot be seen during iteration. After the iteration is
> complete, the task list can be updated in the RCU mechanism. Therefore, the
> task list used by iteration can also be considered as a snapshot.

No, that's not true!  You are not iterating a snapshot of the list,
you're iterating the live list.  It will change under you.  RCU provides
you with some guarantees about that list.  See Documentation/RCU/listRCU.rst

> > For example, if new tasks can be added to the tasklist, they may not
> > be seen by an iteration.  Is this OK?
> 
> The newly added tasks does not access the HWPoison page, because the
> HWPoison page has been isolated from the
> buddy(memory_failure()->take_page_off_buddy()). Therefore, it is safe to see
> the newly added task during the iteration and not be seen by iteration.
> 
> Tasks may be removed from the
> > tasklist after they have been seen by the iteration.  Is this OK?
> 
> Task be seen during iteration are deleted from the task list after
> iteration, it's task_struct is not released because reference counting is
> added in __add_to_kill(). Therefore, the subsequent processing of
> kill_procs() is not affected (sending signals to the task deleted from task
> list). so i think it's safe too.

I don't know this code, but it seems unsafe to me.  Look:

collect_procs_anon:
        for_each_process(tsk) {
                struct task_struct *t = task_early_kill(tsk, force_early);
                        add_to_kill_anon_file(t, page, vma, to_kill);

add_to_kill_anon_file:
        __add_to_kill(tsk, p, vma, to_kill, 0, FSDAX_INVALID_PGOFF);

__add_to_kill:
        get_task_struct(tsk);

static inline struct task_struct *get_task_struct(struct task_struct *t)
{
        refcount_inc(&t->usage);
        return t;
}

/**
 * refcount_inc - increment a refcount
 * @r: the refcount to increment
 *
 * Similar to atomic_inc(), but will saturate at REFCOUNT_SATURATED and WARN.
 *
 * Provides no memory ordering, it is assumed the caller already has a
 * reference on the object.
 *
 * Will WARN if the refcount is 0, as this represents a possible use-after-free
 * condition.
 */

I don't see anything that prevents that refcount_inc from seeing a zero
refcount.  Usually that would be prevented by tasklist_lock, right?

Andrew, I think this patch is bad and needs to be dropped.