linux-kernel - Re: [PATCH] x86/resctrl: Annotate __get_mem_config_intel() as _

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: <20240913194155.GA4188687@thelio-3990X>
Date: Fri, 13 Sep 2024 12:41:55 -0700
From: Nathan Chancellor <nathan@...nel.org>
To: Reinette Chatre <reinette.chatre@...el.com>
Cc: Fenghua Yu <fenghua.yu@...el.com>, x86@...nel.org,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	linux-kernel@...r.kernel.org, llvm@...ts.linux.dev,
	patches@...ts.linux.dev
Subject: Re: [PATCH] x86/resctrl: Annotate __get_mem_config_intel() as __init

Hi Reinette,

On Thu, Sep 12, 2024 at 03:33:09PM -0700, Reinette Chatre wrote:
> Apologies for the delay.

No worries, this is not super high priority (except when the section
mismatch warning is elevated to an error but that does not happen in too
many real world configurations).

> On 8/22/24 5:12 PM, Nathan Chancellor wrote:
> > After a recent LLVM change [1] that deduces __cold on functions that
> > only call cold code (such as __init functions), there is a section
> > mismatch warning from __get_mem_config_intel(), which got moved to
> > .text.unlikely. as a result of that optimization:
> > 
> >    WARNING: modpost: vmlinux: section mismatch in reference: __get_mem_config_intel+0x77 (section: .text.unlikely.) -> thread_throttle_mode_init (section: .init.text)
> > 
> > Mark __get_mem_config_intel() as __init as well since it is only called
> > from __init code, which clears up the warning.
> 
> It looks to me as though __rdt_get_mem_config_amd() may need the same __init
> treatment

It certainly looks like __init would be appropriate for
__rdt_get_mem_config_amd(), although there is no current risk of a
modpost warning like __get_mem_config_intel() because it does not call
any __init functions, which is really what triggered this warning.

> it is not clear to me why __get_mem_config_intel() would trigger
> such warning, but not __rdt_get_mem_config_amd()?

Based on my understanding of the LLVM change linked below my comment
here, __get_mem_config_intel() gets implicitly marked as __cold because
it unconditionally calls thread_throttle_mode_init(), which is __cold
through __init. If __get_mem_config_intel() does not get inlined into
its caller (which could happen if a compiler decides not to optimize
__cold code), that call to thread_throttle_mode_init() will appear to
come from the .text section, even though it will really be from
.init.text because __get_mem_config_intel() is only called from __init
functions.

__rdt_get_mem_config_amd() does not call any cold functions so it avoids
this problem altogether.

I can send a v2 with __init added to __rdt_get_mem_config_amd() if you
want, along with the style update you mention below. Just let me know
what you prefer based on my comments above.

Cheers,
Nathan

> > Link: https://github.com/llvm/llvm-project/commit/6b11573b8c5e3d36beee099dbe7347c2a007bf53 [1]
> > Signed-off-by: Nathan Chancellor <nathan@...nel.org>
> > ---
> >   arch/x86/kernel/cpu/resctrl/core.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> > index 1930fce9dfe9..b28646f1d9d6 100644
> > --- a/arch/x86/kernel/cpu/resctrl/core.c
> > +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > @@ -199,7 +199,7 @@ static inline bool rdt_get_mb_table(struct rdt_resource *r)
> >   	return false;
> >   }
> > -static bool __get_mem_config_intel(struct rdt_resource *r)
> > +static bool __init __get_mem_config_intel(struct rdt_resource *r)
> 
> Surely resctrl is not consistent in this regard but I understand from the coding style
> doc that storage class should precede the return type, so perhaps:
> 	static __init bool __get_mem_config_intel(struct rdt_resource *r)
> 
> We may need to follow this recommended style for this to be included.
> 
> >   {
> >   	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
> >   	union cpuid_0x10_3_eax eax;
> > 
> > ---
> > base-commit: 7424fc6b86c8980a87169e005f5cd4438d18efe6
> > change-id: 20240822-x86-restctrl-get_mem_config_intel-init-3af02a5130ba
> > 
> > Best regards,
> 
> Thank you very much.
> 
> Reinette