Received-SPF: pass (google.com: domain of linux-kernel+bounces-185825-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
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>
In-Reply-To: <20240516130913.GB19105@redhat.com>
From: Andrei Vagin <avagin@google.com>
Date: Tue, 21 May 2024 23:49:27 -0700
Message-ID: <CAEWA0a7NGbv32RG3DE=RwuvEUZsjQsfAB=i1f82Rr5oj8drOPA@mail.gmail.com>
Subject: Re: [PATCH 2/3] seccomp: release task filters when the task exits
To: Oleg Nesterov <oleg@redhat.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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, May 16, 2024 at 6:10=E2=80=AFAM Oleg Nesterov <oleg@redhat.com> wro=
te:
>
> On 05/16, Oleg Nesterov wrote:
> >
> > On 05/15, Andrei Vagin wrote:
> > >
> > > seccomp_sync_threads and seccomp_can_sync_threads should be considere=
d 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 ca=
n'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 =3D 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 |=3D PF_EXITI=
NG".
>
> 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.

>
>         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 ->sigloc=
k in
> > > seccomp_filter_release. Does it sound right?
> >
> > The problem is a single-threaded exiting task. In this case exit_signal=
s()
> > 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 =3D tsk->seccomp.filter;
> >       tsk->seccomp.filter =3D NULL;
> >       spin_unlock_irq(siglock);
> >
> >       __seccomp_filter_release(orig);
> >
> > Right?
> >
> > Oleg.
>