Received-SPF: pass (google.com: domain of linux-kernel+bounces-181091-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 16 May 2024 15:09:14 +0200
From: Oleg Nesterov <oleg@redhat.com>
To: Andrei Vagin <avagin@google.com>
Cc: Kees Cook <keescook@chromium.org>,
	Tycho Andersen <tandersen@netflix.com>,
	Andy Lutomirski <luto@amacapital.net>,
	Will Drewry <wad@chromium.org>, Jens Axboe <axboe@kernel.dk>,
	Christian Brauner <brauner@kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] seccomp: release task filters when the task exits
Message-ID: <20240516130913.GB19105@redhat.com>
References: <20240514175551.297237-1-avagin@google.com>
 <20240514175551.297237-3-avagin@google.com>
 <20240515125113.GC6821@redhat.com>
 <CAEWA0a5dBvRwGAnztL56i=JV-WGGiaTd-GdJYdOxZmq1c+bdpg@mail.gmail.com>
 <20240516093427.GA19105@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240516093427.GA19105@redhat.com>
User-Agent: Mutt/1.5.24 (2015-08-30)

On 05/16, Oleg Nesterov wrote:
>
> On 05/15, Andrei Vagin wrote:
> >
> > seccomp_sync_threads and seccomp_can_sync_threads should be considered too.
>
> Yes. But we only need to consider them in the multi-thread case, right?
> In this case exit_signals() sets PF_EXITING under ->siglock, so they can't
> miss this flag, seccomp_filter_release() doesn't need to take siglock.
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Ah, no. seccomp_filter_release() does need to take ->siglock even if we
forget about proc_pid_seccomp_cache().

Without siglock

	orig = tsk->seccomp.filter;

can leak into the critical section in exit_signals() (spin_unlock is the
one-way barrier) and this LOAD can be reordered with "flags |= PF_EXITING".

Hmm. I thought we have something smp_mb__after_unlock(), but it seems we
don't. So we can't add a fast-path

	if (!tsk->seccomp.filter)
		return;

check at the start of seccomp_filter_release().


Cough... Now that I look at seccomp_can_sync_threads() I think it too
doesn't need the PF_EXITING check.

If it is called before seccomp_filter_release(), this doesn't really
differ from the case when it is called before do_exit/exit_signals.

If it is called after seccomp_filter_release(), then is_ancestor()
must be true.

But perhaps I missed something, I won't insist, up to you.

> > If we check PF_EXITING in all of them, we don't need to take ->siglock in
> > seccomp_filter_release. Does it sound right?
>
> The problem is a single-threaded exiting task. In this case exit_signals()
> sets PF_EXITING lockless. This means that in this case
>
> 	- proc_pid_seccomp_cache() can't rely on the PF_EXITING check
> 	  but it can be safely removed.
>
> 	- seccomp_filter_release() needs to take ->siglock to avoid the
> 	  race with proc_pid_seccomp_cache().
>
> And this chunk from your patch
>
> 	 static void __seccomp_filter_orphan(struct seccomp_filter *orig)
> 	 {
> 	+       lockdep_assert_held(&current->sighand->siglock);
> 	+
>
> looks unnecessary too, seccomp_filter_release() can just do
>
> 	spin_lock_irq(siglock);
> 	orig = tsk->seccomp.filter;
> 	tsk->seccomp.filter = NULL;
> 	spin_unlock_irq(siglock);
>
> 	__seccomp_filter_release(orig);
>
> Right?
>
> Oleg.