From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id AB1CCC369C2 for ; Fri, 25 Apr 2025 17:12:21 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 42EE26B000C; Fri, 25 Apr 2025 13:12:19 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3DD376B000D; Fri, 25 Apr 2025 13:12:19 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2A4326B000E; Fri, 25 Apr 2025 13:12:19 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 0CC806B000C for ; Fri, 25 Apr 2025 13:12:19 -0400 (EDT) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id A04FD801F9 for ; Fri, 25 Apr 2025 17:12:20 +0000 (UTC) X-FDA: 83373209640.01.B307F7B Received: from tor.source.kernel.org (tor.source.kernel.org [172.105.4.254]) by imf10.hostedemail.com (Postfix) with ESMTP id 0AE26C0018 for ; Fri, 25 Apr 2025 17:12:18 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=meH8PVrv; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf10.hostedemail.com: domain of kees@kernel.org designates 172.105.4.254 as permitted sender) smtp.mailfrom=kees@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1745601139; a=rsa-sha256; cv=none; b=mn70W6u+r4dF6cr5l7V7woulK66JD4rrew2zp7owiMv6GeHbvRAvtd9hPxDE+pU++g2VAb WWQZSxrzyzo9t5inVtdihTVHugGwTVJJ5mOE75YQJWqeEFM6v0C+ybaeawuFjq7bFYDPvV 58n+Jlay203btaZ46LngH4xpcaGasmw= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1745601139; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=pPl0mSnDy2ULvrl+NlSY6jK2DNDebISCsgcey9ESoAg=; b=cDEHeTZUmhm4WMtUq2zs+TRdO0ZKVSWwHTyGMA4PwFlrLMdBbw9zjVx8bHGM36A+j9DgUq UNEmN+8R1s9h/l9D7YB8sXUFZ20oLDlry7bcFT57Zl+UI/sFs1f6dB9jSFgE2iOxQiKO34 I3X6J6z50wZje9fjUhai5Fl0895Si6U= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=meH8PVrv; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf10.hostedemail.com: domain of kees@kernel.org designates 172.105.4.254 as permitted sender) smtp.mailfrom=kees@kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by tor.source.kernel.org (Postfix) with ESMTP id B881868465; Fri, 25 Apr 2025 17:11:55 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id CE266C4CEE4; Fri, 25 Apr 2025 17:12:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1745601137; bh=3U78nk8+OCaeXgDy4P6JXWxliABBj1zQnA3H/RaZDHw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=meH8PVrvMt3xXKv0+RutK/uTNkhuORnaRPsqFBrurq9b2X7jON8JVhUqynwBfTAb5 BUfHGI11ypsuqUdKEKoq0y0oiRNFZBXK6U9Ba8RopuRE6hco2lCUOge7XxWBiAJW3h Hxo6gpBSBm0tI00doKzBV66QdrnRME3QE+bbhB4F6iytJdf3MSRPRI2/HUrWaBRA1m JCcgQY0qdX1r6/M4p1NyvzLlFg/s+oS9oM4vWfEftg51+HfFIBV8P4ccO7Kvf1izPi DCKR6urteIZU/xLjvpfkl9nXB1B1jmkZ0a86G1z9vsytA/JyT4qbjwoZ8qW1dy6yKY ozJXJgq8V19eQ== Date: Fri, 25 Apr 2025 10:12:14 -0700 From: Kees Cook To: Suren Baghdasaryan Cc: "Liam R. Howlett" , Lorenzo Stoakes , Andrew Morton , Vlastimil Babka , Jann Horn , Pedro Falcato , David Hildenbrand , Alexander Viro , Christian Brauner , Jan Kara , linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/4] mm: perform VMA allocation, freeing, duplication in mm Message-ID: <202504251010.C5CCE66@keescook> References: <0f848d59f3eea3dd0c0cdc3920644222c40cffe6.1745528282.git.lorenzo.stoakes@oracle.com> <51903B43-2BFC-4BA6-9D74-63F79CF890B7@kernel.org> <7212f5f4-f12b-4b94-834f-b392601360a3@lucifer.local> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 0AE26C0018 X-Stat-Signature: pzqx9h6ocbb36rd56wt3jud6p4whuaxb X-Rspam-User: X-HE-Tag: 1745601138-72895 X-HE-Meta: U2FsdGVkX1+e+AuZw0h9gC2ARGEJBJqs+yez9vsutSOBmhHXrc8hefHFbnAptSOWs8uiMIukG1ujMl5BVVvy9JMR/jeKPd9T/9eXikGw2cQqPD9o2HtBinh/oleWdMZyDMw3I1PUdOR/L5LobRlXub0J6JteCw55/FXpfY7fkWaqvpLjthpKpthCJqGGKotmvzkIU54hjHsTuDkiT6HjTgqMDzHqiC9VJukVhu2B1gJGNfIVi/Tc7KMz0+fQnbfCoxpF0bloiQuEyXEARObDNj1Zft8KXU2gIV7WxFRMDCxdGAjfvZc4hYnpQGFI/lxP1LUotT2tkBmE8iebJ9FNnrPcv+1/wbHQkbJLjTXERPeXqPY4BLlGn+exG2qbT9DsriyMHvygwIhnokyo6qscTmimt7IrATbHTqj0eRc5AG3Dh5b0GoZN1k7Bv7G5MroJQ4V+7GjVIrZfx+XlOD3Z875+NsRHIcXXVs2FXoNkyQFJDpkJ0Xo19uKrhjXkpVjFRLnyMFfGIPlTrAz/AdnKmM6ZkYspo1dpkLrTbt9giXbU/RvPNWPc6jjywrQCcDedM4soxixJUQDGivcqgnwgx5e2y3AuLKBsZrCt7xokECdYlNfe3nwvzhv5SJkSfDJk6FKPThfUkrSF01LQI+lIFQWA+SEGTq2q9KqWC2ZgDrLZaCZHzZHuAviJdweujravg5GyRO7FrQtW2BKDpUwzmxT+mQOKNe8rHU6wV5D6zhUFAFeH3g5HLOpRCVSkERqFVd9Lfw15EwwM4xBi3O5X23UeqhSFooff8tZbfeC2/ABeuR0xlfMqzRpft9tHde9iMsqGytxUJLfIgAd9i2fHVWVqrN9t2tO7bis0yzBZMm67AzAWBnHbRu7cv7z+jt5qKRbpoxyTzUdFo0Y1arc+vH//b6I2W7mHqYC8utuSHEysxDpeu3XoHHTIbQCAfN3vgoVNn8CpkKmpHcGu7ar 4xz6TfSt hq+T+3Fm/29r3y/29PdfFo+S3K5x8JxwOh+E/8vF1hgxCYS2unbwP1blUy+WN+kqQvMepqt4xenZuXjjIN9czOKlVy5fGOcM8vCsuGOGRa9vieUZ7rnnRUXX2wN1XXi4514O6HzXbRh/yG15JSQMhRg832b+L+T+xPn8mumLigzovBG59IiCot7g6k6SurTvGLItAhgCgtztiRs9MusjglNzJ2E82gDvzLN0CEmkiYARBUPktbd5Wumj5uE/MSD4YM/lm7ylCtLfrg/j/HTgs8Cza+3h6IsNIBs6bTdF/PAtBcLS5CvWf397NUc6mDP9R35rfbK2JiuzZdTsvYlazh95ASA== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Fri, Apr 25, 2025 at 08:32:48AM -0700, Suren Baghdasaryan wrote: > On Fri, Apr 25, 2025 at 6:55 AM Liam R. Howlett wrote: > > > > * Lorenzo Stoakes [250425 06:40]: > > > On Thu, Apr 24, 2025 at 08:15:26PM -0700, Kees Cook wrote: > > > > > > > > > > > > On April 24, 2025 2:15:27 PM PDT, Lorenzo Stoakes wrote: > > > > >+static void vm_area_init_from(const struct vm_area_struct *src, > > > > >+ struct vm_area_struct *dest) > > > > >+{ > > > > >+ dest->vm_mm = src->vm_mm; > > > > >+ dest->vm_ops = src->vm_ops; > > > > >+ dest->vm_start = src->vm_start; > > > > >+ dest->vm_end = src->vm_end; > > > > >+ dest->anon_vma = src->anon_vma; > > > > >+ dest->vm_pgoff = src->vm_pgoff; > > > > >+ dest->vm_file = src->vm_file; > > > > >+ dest->vm_private_data = src->vm_private_data; > > > > >+ vm_flags_init(dest, src->vm_flags); > > > > >+ memcpy(&dest->vm_page_prot, &src->vm_page_prot, > > > > >+ sizeof(dest->vm_page_prot)); > > > > >+ /* > > > > >+ * src->shared.rb may be modified concurrently when called from > > > > >+ * dup_mmap(), but the clone will reinitialize it. > > > > >+ */ > > > > >+ data_race(memcpy(&dest->shared, &src->shared, sizeof(dest->shared))); > > > > >+ memcpy(&dest->vm_userfaultfd_ctx, &src->vm_userfaultfd_ctx, > > > > >+ sizeof(dest->vm_userfaultfd_ctx)); > > > > >+#ifdef CONFIG_ANON_VMA_NAME > > > > >+ dest->anon_name = src->anon_name; > > > > >+#endif > > > > >+#ifdef CONFIG_SWAP > > > > >+ memcpy(&dest->swap_readahead_info, &src->swap_readahead_info, > > > > >+ sizeof(dest->swap_readahead_info)); > > > > >+#endif > > > > >+#ifdef CONFIG_NUMA > > > > >+ dest->vm_policy = src->vm_policy; > > > > >+#endif > > > > >+} > > > > > > > > I know you're doing a big cut/paste here, but why in the world is this function written this way? Why not just: > > > > > > > > *dest = *src; > > > > > > > > And then do any one-off cleanups? > > > > > > Yup I find it odd, and error prone to be honest. We'll end up with uninitialised > > > state for some fields if we miss them here, seems unwise... > > > > > > Presumably for performance? > > > > > > This is, as you say, me simply propagating what exists, but I do wonder. > > > > Two things come to mind: > > > > 1. How ctors are done. (v3 of Suren's RCU safe patch series, willy made > > a comment.. I think) > > > > 2. Some race that Vlastimil came up with the copy and the RCU safeness. > > IIRC it had to do with the ordering of the setting of things? > > > > Also, looking at it again... > > > > How is it safe to do dest->anon_name = src->anon_name? Isn't that ref > > counted? > > dest->anon_name = src->anon_name is fine here because right after > vm_area_init_from() we call dup_anon_vma_name() which will bump up the > refcount. I don't recall why this is done this way but now looking at > it I wonder if I could call dup_anon_vma_name() directly instead of > this assignment. Might be just an overlooked legacy from the time we > memcpy'd the entire structure. I'll need to double-check. Oh, is "dest" accessible to other CPU threads? I hadn't looked and was assuming this was like process creation where everything gets built in isolation and then attached to the main process tree. I was thinking this was similar. -- Kees Cook