Re: [PATCH v2] fs/fcntl: fix SOFTIRQ-unsafe lock order in fasync signaling

[Oleg Nesterov]

Nobody replied ;)

In case it was not clear, I believe the commit 00633c4683828
("fs/fcntl: fix SOFTIRQ-unsafe lock order in fasync signaling")
backported to stable trees is wrong.

Right now the fix is not clear to me, I don't even fully understand
the changelog:

    The deadlock is caused by the rwlock writer fairness mechanism:
    1. CPU 0 (process context) holds read_lock(&tasklist_lock) in do_wait().
    2. CPU 1 (process context) attempts write_lock(&tasklist_lock) in
       fork() or exit() and spins, which blocks all new readers.

see below ...

    3. CPU 0 is interrupted by a softirq (e.g., TCP URG packet reception).
    4. The softirq calls send_sigurg() and attempts to acquire
       read_lock(&tasklist_lock), deadlocking because CPU 1 is waiting.

but queued_read_lock_slowpath() does not block the reader if
in_interrupt() == T ?

Mingyu, could you provide more details? How was this problem reported?

So far I only sent the doc patch to explain that we can't simply avoid tasklist:

	[PATCH] pid: document that do_each_pid_task() needs tasklist_lock
	https://lore.kernel.org/all/asuaU8H_5MbO3-cW@xxxxxxxxxx/

Oleg.

On 10/04, Oleg Nesterov wrote:
>
> Hi,
>
> On 05/23, w15303746062@xxxxxxx wrote:
> >
> > Since PID hashing and do_each_pid_task() traversals are already
> > RCU-protected, the read_lock on tasklist_lock is no longer strictly
> > required for safe traversal. Fix this by replacing tasklist_lock with
> > rcu_read_lock(),
>
> I don't think we can simply avoid tasklist_lock. Let me quote my old email,
>
> 	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.
>
> And... I need to recheck, but perhaps signal_struct->multiprocess logic
> is another reason why the lockless do_each_pid_task() is wrong...
>
> See also the recent discussion
> https://lore.kernel.org/all/20261003182224.2171b574@pumpkin/
>
> Oleg.
>
> > --- a/fs/fcntl.c
> > +++ b/fs/fcntl.c
> > @@ -929,11 +929,11 @@ void send_sigio(struct fown_struct *fown, int fd, int band)
> >  			send_sigio_to_task(p, fown, fd, band, type);
> >  		rcu_read_unlock();
> >  	} else {
> > -		read_lock(&tasklist_lock);
> > +		rcu_read_lock();
> >  		do_each_pid_task(pid, type, p) {
> >  			send_sigio_to_task(p, fown, fd, band, type);
> >  		} while_each_pid_task(pid, type, p);
> > -		read_unlock(&tasklist_lock);
> > +		rcu_read_unlock();
> >  	}
> >   out_unlock_fown:
> >  	read_unlock_irqrestore(&fown->lock, flags);
> > @@ -975,11 +975,11 @@ int send_sigurg(struct file *file)
> >  			send_sigurg_to_task(p, fown, type);
> >  		rcu_read_unlock();
> >  	} else {
> > -		read_lock(&tasklist_lock);
> > +		rcu_read_lock();
> >  		do_each_pid_task(pid, type, p) {
> >  			send_sigurg_to_task(p, fown, type);
> >  		} while_each_pid_task(pid, type, p);
> > -		read_unlock(&tasklist_lock);
> > +		rcu_read_unlock();
> >  	}
> >   out_unlock_fown:
> >  	read_unlock_irqrestore(&fown->lock, flags);
> > --
> > 2.34.1
> >