From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_MSPIKE_H2 autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 23292 invoked from network); 6 Oct 2022 19:51:12 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 6 Oct 2022 19:51:12 -0000 Received: (qmail 9885 invoked by uid 550); 6 Oct 2022 19:51:09 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 9850 invoked from network); 6 Oct 2022 19:51:08 -0000 Date: Thu, 6 Oct 2022 15:50:54 -0400 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20221006195053.GX29905@brightrain.aerifal.cx> References: <033092a85a52d9f8e8d73a235e2381ae@ispras.ru> <6cf9cc5562e17610392c0f9c2cc68316@ispras.ru> <20221006192042.GW29905@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="0lnxQi9hkpPO77W3" Content-Disposition: inline In-Reply-To: <20221006192042.GW29905@brightrain.aerifal.cx> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] Re: MT fork and key_lock in pthread_key_create.c --0lnxQi9hkpPO77W3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Oct 06, 2022 at 03:20:42PM -0400, Rich Felker wrote: > On Thu, Oct 06, 2022 at 10:02:11AM +0300, Alexey Izbyshev wrote: > > On 2022-10-06 09:37, Alexey Izbyshev wrote: > > >Hi, > > > > > >I noticed that fork() doesn't take key_lock that is used to protect > > >the global table of thread-specific keys. I couldn't find mentions of > > >this lock in the MT fork discussion in the mailing list archive. Was > > >this lock overlooked? > > > > > >Also, I looked at how __aio_atfork() handles a similar case with > > >maplock, and it seems wrong. It takes the read lock and then simply > > >unlocks it both in the parent and in the child. But if there were > > >other holders of the read lock at the time of fork(), the lock won't > > >end up in the unlocked state in the child. It should probably be > > >completely nulled-out in the child instead. > > > > > Looking at aio further, I don't understand how it's supposed to work > > with MT fork at all. __aio_atfork() is called in _Fork() when the > > allocator locks are already held. Meanwhile another thread could be > > stuck in __aio_get_queue() holding maplock in exclusive mode while > > trying to allocate, resulting in deadlock. > > Indeed, this is messy and I don't think it makes sense to be doing > this at all. The child is just going to throw away the state so the > parent shouldn't need to synchronize at all, but if we walk the > multi-level map[] table in the child after async fork, it's possible > that the contents seen are inconsistent, even that the pointers are > only half-written or something. > > I see a few possible solutions: > > 1. Just set map = 0 in the child and leak the memory. This is not > going to matter unless you're doing multiple generations of fork > with aio anyway. > > 2. The same, but be a little bit smarter. pthread_rwlock_tryrdlock in > the child, and if it succeeds, we know the map is consistent so we > can just zero it out the same as now. Still "leaks" but only on > contention to expand the map. > > 3. Getting a little smarter still: move the __aio_atfork for the > parent side from _Fork to fork, outside of the critical section > where malloc lock is held. Then proceed as in (2). Now, the > tryrdlock is guaranteed to succeed in the child. Leak is only > possible when _Fork is used (in which case the child context is an > async signal one, and thus calling any aio_* that would allocate > map[] again is UB -- note that in this case, the only reason we > have to do anything at all in the child is to prevent close from > interacting with aio). > > After writing them out, 3 seems like the right choice. Proposed patch attached. --0lnxQi9hkpPO77W3 Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="aio_atfork.diff" diff --git a/src/aio/aio.c b/src/aio/aio.c index fa24f6b6..4c3379e1 100644 --- a/src/aio/aio.c +++ b/src/aio/aio.c @@ -401,11 +401,25 @@ void __aio_atfork(int who) if (who<0) { pthread_rwlock_rdlock(&maplock); return; + } else if (!who) { + pthread_rwlock_unlock(&maplock); + return; } - if (who>0 && map) for (int a=0; a<(-1U/2+1)>>24; a++) + if (pthread_rwlock_tryrdlock(&maplock)) { + /* Obtaining lock may fail if _Fork was called nor via + * fork. In this case, no further aio is possible from + * child and we can just null out map so __aio_close + * does not attempt to do anything. */ + map = 0; + return; + } + if (map) for (int a=0; a<(-1U/2+1)>>24; a++) if (map[a]) for (int b=0; b<256; b++) if (map[a][b]) for (int c=0; c<256; c++) if (map[a][b][c]) for (int d=0; d<256; d++) map[a][b][c][d] = 0; - pthread_rwlock_unlock(&maplock); + /* Re-initialize the rwlock rather than unlocking since there + * may have been more than one reference on it in the parent. + * We are not a lock holder anyway; the thread in the parent was. */ + pthread_rwlock_init(&maplock, 0); } diff --git a/src/process/_Fork.c b/src/process/_Fork.c index da063868..fb0fdc2c 100644 --- a/src/process/_Fork.c +++ b/src/process/_Fork.c @@ -14,7 +14,6 @@ pid_t _Fork(void) pid_t ret; sigset_t set; __block_all_sigs(&set); - __aio_atfork(-1); LOCK(__abort_lock); #ifdef SYS_fork ret = __syscall(SYS_fork); @@ -32,7 +31,7 @@ pid_t _Fork(void) if (libc.need_locks) libc.need_locks = -1; } UNLOCK(__abort_lock); - __aio_atfork(!ret); + if (!ret) __aio_atfork(1); __restore_sigs(&set); return __syscall_ret(ret); } diff --git a/src/process/fork.c b/src/process/fork.c index ff71845c..80e804b1 100644 --- a/src/process/fork.c +++ b/src/process/fork.c @@ -36,6 +36,7 @@ static volatile int *const *const atfork_locks[] = { static void dummy(int x) { } weak_alias(dummy, __fork_handler); weak_alias(dummy, __malloc_atfork); +weak_alias(dummy, __aio_atfork); weak_alias(dummy, __ldso_atfork); static void dummy_0(void) { } @@ -50,6 +51,7 @@ pid_t fork(void) int need_locks = libc.need_locks > 0; if (need_locks) { __ldso_atfork(-1); + __aio_atfork(-1); __inhibit_ptc(); for (int i=0; i