Received-SPF: pass (google.com: domain of linux-kernel+bounces-180852-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 11:34:27 +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: <20240516093427.GA19105@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>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAEWA0a5dBvRwGAnztL56i=JV-WGGiaTd-GdJYdOxZmq1c+bdpg@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)

(add lkml)

On 05/15, Andrei Vagin wrote:
>
> On Wed, May 15, 2024 at 5:52 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Let me repeat I forgot everything about seccomp, but let me ask
> > a couple of questions...
>
> It seems you still remember something:). Thank you for the feedback.

Just I am still remember how to use grep ;)

> > > @@ -2126,6 +2137,11 @@ static struct seccomp_filter *get_nth_filter(struct task_struct *task,
> > >        */
> > >       spin_lock_irq(&task->sighand->siglock);
> > >
> > > +     if (task->flags & PF_EXITING) {
> > > +             spin_unlock_irq(&task->sighand->siglock);
> > > +             return ERR_PTR(-EINVAL);
> > > +     }
> >
> > Why do we need the PF_EXITING check here?
> >
> > This looks unnecessary even if get_nth_filter() could race with the
> > exiting task, but this doesn't matter.
> >
> > This race is not possible, get_nth_filter() is only called from ptrace()
> > paths, but the tracee can't stop in TASK_TRACED after exit_signals() which
> > sets PF_EXITING.
>
> If we rely on using seccomp_get_filter only from ptrace, you are right.

Plus it too does __get_seccomp_filter/__get_seccomp_filter, so I guess it
should be safe without this check even if it could be used outside of ptrace.
Just like proc_pid_seccomp_cache(), see below.

> > > @@ -2494,6 +2510,11 @@ int proc_pid_seccomp_cache(struct seq_file *m, struct pid_namespace *ns,
> > >       if (!lock_task_sighand(task, &flags))
> > >               return -ESRCH;
> > >
> > > +     if (thread->flags & PF_EXITING) {
> > > +             unlock_task_sighand(task, &flags);
> > > +             return 0;
> >
> > Again, do we really need this check?
> >
> > It can race with the exiting task and (without this check) do
> > __get_seccomp_filter(f) right before seccomp_filter_release()
> > takes sighand->siglock. But why is it bad?
>
> I think you are right, this check isn't required.
>
> >
> > OTOH. I guess proc_pid_seccomp_cache() is the only reason why
> > seccomp_filter_release() takes ->siglock with your patch?
>
> 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.

> 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.