linux-kernel - Re: [PATCH v10 2/3] mm: add a field to store names for private anonymous memory

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Wed, 6 Oct 2021 19:39:40 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Suren Baghdasaryan <surenb@...gle.com>
Cc:     Colin Cross <ccross@...gle.com>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Michal Hocko <mhocko@...e.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Kees Cook <keescook@...omium.org>,
        Matthew Wilcox <willy@...radead.org>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Johannes Weiner <hannes@...xchg.org>,
        Jonathan Corbet <corbet@....net>,
        Al Viro <viro@...iv.linux.org.uk>,
        Randy Dunlap <rdunlap@...radead.org>,
        Kalesh Singh <kaleshsingh@...gle.com>,
        Peter Xu <peterx@...hat.com>, rppt@...nel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Catalin Marinas <catalin.marinas@....com>,
        vincenzo.frascino@....com,
        Chinwen Chang (張錦文) 
        <chinwen.chang@...iatek.com>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Jann Horn <jannh@...gle.com>, apopple@...dia.com,
        John Hubbard <jhubbard@...dia.com>,
        Yu Zhao <yuzhao@...gle.com>, Will Deacon <will@...nel.org>,
        fenghua.yu@...el.com, thunder.leizhen@...wei.com,
        Hugh Dickins <hughd@...gle.com>, feng.tang@...el.com,
        Jason Gunthorpe <jgg@...pe.ca>, Roman Gushchin <guro@...com>,
        Thomas Gleixner <tglx@...utronix.de>, krisman@...labora.com,
        chris.hyser@...cle.com, Peter Collingbourne <pcc@...gle.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Jens Axboe <axboe@...nel.dk>, legion@...nel.org,
        Rolf Eike Beer <eb@...ix.com>,
        Cyrill Gorcunov <gorcunov@...il.com>,
        Pavel Machek <pavel@....cz>,
        Muchun Song <songmuchun@...edance.com>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Thomas Cedeno <thomascedeno@...gle.com>, sashal@...nel.org,
        cxfcosmos@...il.com, Rasmus Villemoes <linux@...musvillemoes.dk>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-fsdevel@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-mm <linux-mm@...ck.org>,
        kernel-team <kernel-team@...roid.com>
Subject: Re: [PATCH v10 2/3] mm: add a field to store names for private
 anonymous memory

On Mon, 4 Oct 2021 09:21:42 -0700 Suren Baghdasaryan <surenb@...gle.com> wrote:

> > > > The name pointers are not shared between vmas even if they contain the
> > > > same name. The name pointer is stored in a union with fields that are
> > > > only used on file-backed mappings, so it does not increase memory usage.
> > > >
> > > > The patch is based on the original patch developed by Colin Cross, more
> > > > specifically on its latest version [1] posted upstream by Sumit Semwal.
> > > > It used a userspace pointer to store vma names. In that design, name
> > > > pointers could be shared between vmas. However during the last upstreaming
> > > > attempt, Kees Cook raised concerns [2] about this approach and suggested
> > > > to copy the name into kernel memory space, perform validity checks [3]
> > > > and store as a string referenced from vm_area_struct.
> > > > One big concern is about fork() performance which would need to strdup
> > > > anonymous vma names. Dave Hansen suggested experimenting with worst-case
> > > > scenario of forking a process with 64k vmas having longest possible names
> > > > [4]. I ran this experiment on an ARM64 Android device and recorded a
> > > > worst-case regression of almost 40% when forking such a process. This
> > > > regression is addressed in the followup patch which replaces the pointer
> > > > to a name with a refcounted structure that allows sharing the name pointer
> > > > between vmas of the same name. Instead of duplicating the string during
> > > > fork() or when splitting a vma it increments the refcount.
> > >
> > > Generally, the patch adds a bunch of code which a lot of users won't
> > > want.  Did we bust a gut to reduce this impact?  Was a standalone
> > > config setting considered?
> >
> > I didn't consider a standalone config for this feature because when
> > not used it has no memory impact at runtime. As for the image size, I
> > built Linus' ToT with and without this patchset with allmodconfig and

allnoconfig would be more interesting.  People who want small kernels
won't be using allmodconfig!

> > the sizes are:
> > Without the patchset:
> > $ size vmlinux
> >    text    data     bss     dec     hex filename
> > 40763556 58424519 29016228 128204303 7a43e0f vmlinux
> >
> > With the patchset:
> > $ size vmlinux
> >    text    data     bss     dec     hex filename
> > 40765068 58424671 29016228 128205967 7a4448f vmlinux
> >
> > The increase seems quite small, so I'm not sure if it warrants a
> > separate config option.
> 
> Andrew, do you still think we need a separate CONFIG option? I fixed
> the build issue when CONFIG_ADVISE_SYSCALLS=n and would like to post
> the update but if you want to have a separate config then I can post
> that together with the fix. Please let me know.

I don't see much downside to the standalone option.  More complexity
for developers/testers, I guess.  But such is life?