linux-kernel - Re: [PATCH 2/4] mm: perform VMA allocation, freeing, duplication in mm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7212f5f4-f12b-4b94-834f-b392601360a3@lucifer.local>
Date: Fri, 25 Apr 2025 11:40:00 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Kees Cook <kees@...nel.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
        Pedro Falcato <pfalcato@...e.de>, David Hildenbrand <david@...hat.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
        Suren Baghdasaryan <surenb@...gle.com>, linux-mm@...ck.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] mm: perform VMA allocation, freeing, duplication in
 mm

On Thu, Apr 24, 2025 at 08:15:26PM -0700, Kees Cook wrote:
>
>
> On April 24, 2025 2:15:27 PM PDT, Lorenzo Stoakes <lorenzo.stoakes@...cle.com> 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.

>
>
> --
> Kees Cook