Messages in this thread | | | From | Andrei Vagin <> | Date | Wed, 22 May 2024 00:06:36 -0700 | Subject | Re: [PATCH 2/3] seccomp: release task filters when the task exits |
| |
> 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. The first thread can check that PF_EXITING isn't set for another thread. Then, the second thread calls exit_signals and seccomp_filter_release(), and finally, the first thread sets its seccomp.filter to the second thread. If seccomp_filter_release takes siglock, it will be handled properly.
> > > > 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
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. > >
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.
Thanks, Andrei
| |