From owner-dev-commits-src-main@freebsd.org Fri May 21 11:51:53 2021 Return-Path: Delivered-To: dev-commits-src-main@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 51922657E30; Fri, 21 May 2021 11:51:53 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4FmlMD69fKz3J9l; Fri, 21 May 2021 11:51:52 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.16.1/8.16.1) with ESMTPS id 14LBpcrV087325 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Fri, 21 May 2021 14:51:41 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua 14LBpcrV087325 Received: (from kostik@localhost) by tom.home (8.16.1/8.16.1/Submit) id 14LBpcZa087324; Fri, 21 May 2021 14:51:38 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Fri, 21 May 2021 14:51:38 +0300 From: Konstantin Belousov To: Hans Petter Selasky Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: b764a426534f - main - There is a window where threads are removed from the process list and where the thread destructor is invoked. Catch that window by waiting for all task_struct allocations to be returned before freeing the UMA zone in the LinuxKPI. Else UMA may fail to release the zone due to concurrent access and panic: Message-ID: References: <202105211121.14LBLHI2026834@gitrepo.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <202105211121.14LBLHI2026834@gitrepo.freebsd.org> X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FORGED_GMAIL_RCVD,FREEMAIL_FROM, NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on tom.home X-Rspamd-Queue-Id: 4FmlMD69fKz3J9l X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: dev-commits-src-main@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for the main branch of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 21 May 2021 11:51:53 -0000 On Fri, May 21, 2021 at 11:21:17AM +0000, Hans Petter Selasky wrote: > The branch main has been updated by hselasky: > > URL: https://cgit.FreeBSD.org/src/commit/?id=b764a426534f2f5f86d6625288c74dafdbc94d2b > > commit b764a426534f2f5f86d6625288c74dafdbc94d2b > Author: Hans Petter Selasky > AuthorDate: 2021-05-21 11:17:42 +0000 > Commit: Hans Petter Selasky > CommitDate: 2021-05-21 11:18:41 +0000 > > There is a window where threads are removed from the process list and where > the thread destructor is invoked. Catch that window by waiting for all > task_struct allocations to be returned before freeing the UMA zone in the > LinuxKPI. Else UMA may fail to release the zone due to concurrent access > and panic: > > panic() - Bad link element prev->next != elm > zone_release() > bucket_drain() > bucket_free() > zone_dtor() > zone_free_item() > uma_zdestroy() > linux_current_uninit() > > This failure can be triggered by loading and unloading the LinuxKPI module > in a loop: > > while true > do > kldload linuxkpi > kldunload linuxkpi > done > > Discussed with: kib@ No, it was not discussed, with me. It contains parts of my half-done patches. And I disagree with what the global counting you added there, both on principle and on implementation. > MFC after: 1 week > Sponsored by: Mellanox Technologies // NVIDIA Networking > --- > sys/compat/linuxkpi/common/src/linux_current.c | 35 ++++++++++++++++++++++---- > 1 file changed, 30 insertions(+), 5 deletions(-) > > diff --git a/sys/compat/linuxkpi/common/src/linux_current.c b/sys/compat/linuxkpi/common/src/linux_current.c > index 9bae7ee92e49..51e396081c04 100644 > --- a/sys/compat/linuxkpi/common/src/linux_current.c > +++ b/sys/compat/linuxkpi/common/src/linux_current.c > @@ -45,6 +45,7 @@ extern u_int first_msi_irq, num_msi_irqs; > > static eventhandler_tag linuxkpi_thread_dtor_tag; > > +static atomic_t linux_current_allocs; > static uma_zone_t linux_current_zone; > static uma_zone_t linux_mm_zone; > > @@ -146,6 +147,10 @@ linux_alloc_current(struct thread *td, int flags) > /* free mm_struct pointer, if any */ > uma_zfree(linux_mm_zone, mm); > > + /* keep track of number of allocations */ > + if (atomic_add_return(1, &linux_current_allocs) == INT_MAX) > + panic("linux_alloc_current: Refcount too high!"); > + > return (0); > } > > @@ -173,6 +178,10 @@ linux_free_current(struct task_struct *ts) > { > mmput(ts->mm); > uma_zfree(linux_current_zone, ts); > + > + /* keep track of number of allocations */ > + if (atomic_sub_return(1, &linux_current_allocs) < 0) > + panic("linux_free_current: Negative refcount!"); > } > > static void > @@ -271,10 +280,6 @@ SYSCTL_INT(_compat_linuxkpi, OID_AUTO, task_struct_reserve, > static void > linux_current_init(void *arg __unused) > { > - lkpi_alloc_current = linux_alloc_current; > - linuxkpi_thread_dtor_tag = EVENTHANDLER_REGISTER(thread_dtor, > - linuxkpi_thread_dtor, NULL, EVENTHANDLER_PRI_ANY); > - > TUNABLE_INT_FETCH("compat.linuxkpi.task_struct_reserve", > &lkpi_task_resrv); > if (lkpi_task_resrv == 0) { > @@ -298,6 +303,12 @@ linux_current_init(void *arg __unused) > UMA_ALIGN_PTR, 0); > uma_zone_reserve(linux_mm_zone, lkpi_task_resrv); > uma_prealloc(linux_mm_zone, lkpi_task_resrv); > + > + atomic_thread_fence_seq_cst(); > + > + lkpi_alloc_current = linux_alloc_current; > + linuxkpi_thread_dtor_tag = EVENTHANDLER_REGISTER(thread_dtor, > + linuxkpi_thread_dtor, NULL, EVENTHANDLER_PRI_ANY); > } > SYSINIT(linux_current, SI_SUB_EVENTHANDLER, SI_ORDER_SECOND, > linux_current_init, NULL); > @@ -309,6 +320,10 @@ linux_current_uninit(void *arg __unused) > struct task_struct *ts; > struct thread *td; > > + lkpi_alloc_current = linux_alloc_current_noop; > + > + atomic_thread_fence_seq_cst(); > + > sx_slock(&allproc_lock); > FOREACH_PROC_IN_SYSTEM(p) { > PROC_LOCK(p); > @@ -321,8 +336,18 @@ linux_current_uninit(void *arg __unused) > PROC_UNLOCK(p); > } > sx_sunlock(&allproc_lock); > + > + /* > + * There is a window where threads are removed from the > + * process list and where the thread destructor is invoked. > + * Catch that window by waiting for all task_struct > + * allocations to be returned before freeing the UMA zone. > + */ > + while (atomic_read(&linux_current_allocs) != 0) > + pause("W", 1); > + > EVENTHANDLER_DEREGISTER(thread_dtor, linuxkpi_thread_dtor_tag); > - lkpi_alloc_current = linux_alloc_current_noop; > + > uma_zdestroy(linux_current_zone); > uma_zdestroy(linux_mm_zone); > }