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

[Oleg Nesterov]

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
>