linux-kernel - Re: [PATCH] include/linux/module.h: mark init/cleanup_module aliases 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]

Date:   Thu, 31 Jan 2019 17:48:13 +0100
From:   Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To:     Jessica Yu <jeyu@...nel.org>
Cc:     Laura Abbott <labbott@...hat.com>,
        Martin Sebor <msebor@....gnu.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] include/linux/module.h: mark init/cleanup_module aliases
 as __cold

Hi Jessica,

On Thu, Jan 31, 2019 at 3:22 PM Jessica Yu <jeyu@...nel.org> wrote:
>
> Hi Miguel, sorry for the delay!

No worries! :)

> The module init functions are only called once from do_init_module().
> Does the __cold attribute just assume it is unlikely to be executed,
> or just that it is infrequently called (which would be true for the
> module init functions since they're just called once)?

That was exactly my concern :-) Martin can provide way better details
than me, but as far as I understand it, it is the paths that end up
calling __cold functions that are treated as unlikely to happen. For
instance, if f() has a few branches and calls a cold g() in one of
them, that branch is understood to be rarely executed and f() will be
laid out assuming the other branches are more likely.

Then there is the other aspect of __cold, in the definition of the
function. There, it affects how it is compiled and where it is placed,
etc.

Therefore, I assume the current situation is the correct one: we want
to callers to *not* see __cold, but we want the init function to be
compiled as __cold.

Now, the alias is not seen by other TUs (i.e. they only see the extern
declaration), so it does not matter whether the alias is cold or not
(except for the warning), as far as I understand.

> In any case, module init functions are normally annotated with __init,
> so they get the __cold attribute anyway. I'm wondering why not just
> annotate the alias with __init instead, instead of cherry picking
> attributes to silence the warnings? That way the alias and the actual
> module init function would always have the same declaration/attributes.
> Would this work to silence the warnings or am I missing something?

We could do indeed do that too (Martin actually proposed a solution
with the new copy attribute, which would do something like that).

I chose to only add __cold to avoid any problems derived from the rest
of the attributes, since I don't know how they behave or what are the
implications (if any) of putting them into the alias (and not into the
extern declaration).

Cheers,
Miguel