linux-kernel - Re: [PATCH v2 1/2] x86/ioremap: introduce helper to implement xxx_is_setup

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: <Z0Q-TAbXPSwFXWPI@gmail.com>
Date: Mon, 25 Nov 2024 10:07:24 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Tom Lendacky <thomas.lendacky@....com>
Cc: Baoquan He <bhe@...hat.com>, linux-kernel@...r.kernel.org, bp@...en8.de,
	x86@...nel.org
Subject: Re: [PATCH v2 1/2] x86/ioremap: introduce helper to implement
 xxx_is_setup_data()


* Tom Lendacky <thomas.lendacky@....com> wrote:

> On 11/20/24 02:25, Ingo Molnar wrote:
> > 
> > * Tom Lendacky <thomas.lendacky@....com> wrote:
> > 
> >>>  /*
> >>>   * Examine the physical address to determine if it is boot data by checking
> >>>   * it against the boot params setup_data chain.
> >>>   */
> >>> -static bool memremap_is_setup_data(resource_size_t phys_addr,
> >>> -				   unsigned long size)
> >>> +static bool __ref __memremap_is_setup_data(resource_size_t phys_addr,
> >>
> >> Oh, I see why the __ref is needed now, because this calls an __init
> >> function based on the early bool.
> >>
> >> While this nicely consolidates the checking, I'll let the x86
> >> maintainers decide whether they like that an __init function is calling
> >> a non __init function.
> > 
> > So why would it be a problem? Only non-__init calling __init is a bug, 
> > because __init functions cease to exist after early bootup. Also, 
> > calling certain kernel subsystems too early, before they are 
> > initialized, is a bug as well.
> 
> I brought it up because that is what could happen if the wrong boolean
> value is supplied to the helper function. The helper function is marked
> non-__init but calls a __init function if the boolean value is true, hence
> the need for the __ref tagging.

Oh, so I misunderstood your point, because you typoed the direction:

  > >> While this nicely consolidates the checking, I'll let the x86 
  > >> maintainers decide whether they like that an __init function is 
  > >> calling a non __init function.

The problem is the inverse: that a non-__init generic facility may be 
calling an __init function if the wrong flag is supplied. As you wrote 
a sentence earlier, but I only responded to this paragraph :-/

So yeah, that's a fragility indeed - which happens sometimes when 
generic MM facilities share code (I think 
mm/sparse.c::section_deactivate() is similar), but I tend to agree that 
this pattern could perhaps be improved:

+                       if (early)
+                               early_memunmap(data, SD_SIZE);
+                       else
+                               memunmap(data);

Could we perhaps un-__init early_memunmap(), and call memunmap() if 
it's in a late context? (Also early_memremap_decrypted().)

That way this code could just use early_memunmap() and 
early_memremap_decrypted() and skip the boolean complication?

> But, I don't anticipate that this helper will be called by anything 
> else than what is currently calling it and the proper boolean values 
> are set on those calls.
> 
> I just wanted to raise awareness. I'm ok with using __ref, just 
> wanted to make sure everyone else is, too.

It's a fair argument I misunderstood :-)

Thanks,

	Ingo