linux-kernel - Re: [PATCH 0/6] Boot-time switching between 4- and 5-level paging for 4.15, Part 1

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]

Message-ID: <20171024131227.nchrzazuk4c6r75i@node.shutemov.name>
Date:   Tue, 24 Oct 2017 16:12:27 +0300
From:   "Kirill A. Shutemov" <kirill@...temov.name>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Ingo Molnar <mingo@...hat.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>, x86@...nel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andy Lutomirski <luto@...capital.net>,
        Cyrill Gorcunov <gorcunov@...nvz.org>,
        Borislav Petkov <bp@...e.de>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/6] Boot-time switching between 4- and 5-level paging
 for 4.15, Part 1

On Tue, Oct 24, 2017 at 02:47:41PM +0200, Ingo Molnar wrote:
> > > > > > > Making a variable that 'looks' like a constant macro dynamic in a rare Kconfig 
> > > > > > > scenario is asking for trouble.
> > > > > > 
> > > > > > We expect boot-time page mode switching to be enabled in kernel of next
> > > > > > generation enterprise distros. It shoudn't be that rare.
> > > > > 
> > > > > My point remains even with not-so-rare Kconfig dependency.
> > > > 
> > > > I don't follow how introducing new variable that depends on Kconfig option
> > > > would help with the situation.
> > > 
> > > A new, properly named variable or function (max_physmem_bits or 
> > > max_physmem_bits()) that is not all uppercase would make it abundantly clear that 
> > > it is not a constant but a runtime value.
> > 
> > Would we need to rename every uppercase macros that would depend on
> > max_physmem_bits()? Like MAXMEM.
> 
> MAXMEM isn't used in too many places either - what's the total impact of it?

The impact is not very small. The tree of macros dependent on
MAX_PHYSMEM_BITS:

MAX_PHYSMEM_BITS
  MAXMEM
    KEXEC_SOURCE_MEMORY_LIMIT
    KEXEC_DESTINATION_MEMORY_LIMIT
    KEXEC_CONTROL_MEMORY_LIMIT
  SECTIONS_SHIFT
    ZONEID_SHIFT
      ZONEID_PGSHIFT
      ZONEID_MASK

The total number of users of them is not large. It's doable. But I expect
it to be somewhat ugly, since we're partly in generic code and it would
require some kind of compatibility layer for other archtectures.

Do you want me to rename them all?

> > > > We would end up with inverse situation: people would use MAX_PHYSMEM_BITS
> > > > where the new variable need to be used and we will in the same situation.
> > > 
> > > It should result in sub-optimal resource allocations worst-case, right?
> > 
> > I don't think it's the worst case.
> > 
> > For instance, virt_addr_valid() depends indirectly on it:
> > 
> >   virt_addr_valid()
> >     __virt_addr_valid()
> >       phys_addr_valid()
> >         boot_cpu_data.x86_phys_bits (initialized with MAX_PHYSMEM_BITS)
> > 
> > virt_addr_valid() is used in things like implementation /dev/kmem.
> > 
> > To me it's far more risky than occasional build breakage for
> > CONFIG_X86_5LEVEL=y.
> 
> So why do we have two variables here, one boot_cpu_data.x86_phys_bits and the 
> other MAX_PHYSMEM_BITS - both set once during boot?
> 
> I'm trying to find a clean solution for this all - hiding a boot time dependency 
> into a constant-looking value doesn't feel clean.

We already have plenty of them: PAGE_OFFSET, IA32_PAGE_OFFSET,
VMALLOC_START, VMEMMAP_START, TASK_SIZE, STACK_TOP, FIXADDR_TOP...

I don't understand why you make this one a special.

-- 
 Kirill A. Shutemov