Received-SPF: pass (google.com: domain of linux-kernel+bounces-186060-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 22 May 2024 12:35:51 +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: <20240522103531.GA28435@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>
 <20240516130913.GB19105@redhat.com>
 <CAEWA0a7NGbv32RG3DE=RwuvEUZsjQsfAB=i1f82Rr5oj8drOPA@mail.gmail.com>
 <CAEWA0a6Z+jGKtz1Bdc2tP4YwR6u1zeEhoYY1hYZqdxCZMcgjcg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAEWA0a6Z+jGKtz1Bdc2tP4YwR6u1zeEhoYY1hYZqdxCZMcgjcg@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)

On 05/22, Andrei Vagin wrote:
>
> > On Thu, May 16, 2024 at 6:10 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > 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.
> > >                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> PF_EXITING is set without holding ->siglock if tsk->signal has the
> SIGNAL_GROUP_EXIT flag. I think it can be a case when one thread is in
> seccomp_sync_threads and others are exiting.

Yes, I forgot this.

> > > Hmm. I thought we have something smp_mb__after_unlock(), but it seems we
> > > don't. So we can't add a fast-path
>
> We have smp_mb__after_unlock_lock in include/linux/rcupdate.h.

This is another thing.

But sorry for confusion, this doesn't really matter, we could you a plain mb().
I mean, I was thinking about something like

	seccomp_filter_release:

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

		spin_lock_irq(siglock);
		orig = tsk->seccomp.filter;
		...

but then seccomp_sync_threads() should do something like


		orig = READ_ONCE(thread->seccomp.filter);

		smp_store_release(&thread->seccomp.filter,
				  caller->seccomp.filter);

		smp_mb(); // pairs with mb() in seccomp_filter_release()

		if (READ_ONCE(thread->flags) & PF_EXITING) {
			WRITE_ONCE(thread->seccomp.filter, orig);
			continue;
		}
		__seccomp_filter_release(orig);

		...

too subtle even _if_ correct, and I am not sure at all this would be correct.

> > > 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.
> > >
>
> You are right, this check isn't required in seccomp_can_sync_threads, but
> I decided that it is better to be consistent with seccomp_sync_threads.

OK, agreed.

Oleg.