Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Jens Axboe <axboe@kernel.dk>
Cc:     io-uring@vger.kernel.org, torvalds@linux-foundation.org,
        linux-kernel@vger.kernel.org, oleg@redhat.com
References: <20210320153832.1033687-1-axboe@kernel.dk>
Date:   Sat, 20 Mar 2021 11:26:32 -0500
In-Reply-To: <20210320153832.1033687-1-axboe@kernel.dk> (Jens Axboe's message
        of "Sat, 20 Mar 2021 09:38:30 -0600")
Message-ID: <m14kh5aj0n.fsf@fess.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCHSET 0/2] PF_IO_WORKER signal tweaks
Precedence: bulk

Jens Axboe <axboe@kernel.dk> writes:

> Hi,
>
> Been trying to ensure that we do the right thing wrt signals and
> PF_IO_WORKER threads, and I think there are two cases we need to handle
> explicitly:
>
> 1) Just don't allow signals to them in general. We do mask everything
>    as blocked, outside of SIGKILL, so things like wants_signal() will
>    never return true for them. But it's still possible to send them a
>    signal via (ultimately) group_send_sig_info(). This will then deliver
>    the signal to the original io_uring owning task, and that seems a bit
>    unexpected. So just don't allow them in general.
>
> 2) STOP is done a bit differently, and we should not allow that either.
>
> Outside of that, I've been looking at same_thread_group(). This will
> currently return true for an io_uring task and it's IO workers, since
> they do share ->signal. From looking at the kernel users of this, that
> actually seems OK for the cases I checked. One is accounting related,
> which we obviously want, and others are related to permissions between
> tasks. FWIW, I ran with the below and didn't observe any ill effects,
> but I'd like someone to actually think about and verify that PF_IO_WORKER
> same_thread_group() usage is sane.

They are helper threads running in kernel space.  Allowing the ordinary
threads not to block.  Why would same_thread_group be a problem?

I don't hate this set of patches.   But I also don't see much
explanation why the changes are the right thing semantically.

That makes me uneasy.  Because especially the SIGSTOP changes feels like
it is the wrong thing semantically.  The group_send_sig_info change
simply feels like it is unnecessary.

Like we are maybe playing whak-a-mole with symptoms rather than make
certain these are the right fixes for the symptoms.

Eric

> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
> index 3f6a0fcaa10c..a580bc0f8aa3 100644
> --- a/include/linux/sched/signal.h
> +++ b/include/linux/sched/signal.h
> @@ -667,10 +667,17 @@ static inline bool thread_group_leader(struct task_struct *p)
>  	return p->exit_signal >= 0;
>  }
>  
> +static inline
> +bool same_thread_group_account(struct task_struct *p1, struct task_struct *p2)
> +{
> +	return p1->signal == p2->signal
> +}
> +
>  static inline
>  bool same_thread_group(struct task_struct *p1, struct task_struct *p2)
>  {
> -	return p1->signal == p2->signal;
> +	return same_thread_group_account(p1, p2) &&
> +			!((p1->flags | p2->flags) & PF_IO_WORKER);
>  }
>  
>  static inline struct task_struct *next_thread(const struct task_struct *p)
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index 5f611658eeab..625110cacc2a 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -307,7 +307,7 @@ void thread_group_cputime(struct task_struct *tsk, struct task_cputime *times)
>  	 * those pending times and rely only on values updated on tick or
>  	 * other scheduler action.
>  	 */
> -	if (same_thread_group(current, tsk))
> +	if (same_thread_group_account(current, tsk))
>  		(void) task_sched_runtime(current);
>  
>  	rcu_read_lock();