linux-kernel - Re: [PATCH v5 05/13] module: Move latched RB-tree support to a separate file

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: <CANfR36iKsTd8Ave+gt2PUho0Zpcnc7kYJrXUHEG5_TrbH_+3jw@mail.gmail.com>
Date:   Thu, 10 Feb 2022 22:41:24 +0000
From:   Aaron Tomlin <atomlin@...hat.com>
To:     Christophe Leroy <christophe.leroy@...roup.eu>
Cc:     "mcgrof@...nel.org" <mcgrof@...nel.org>,
        "cl@...ux.com" <cl@...ux.com>,
        "pmladek@...e.com" <pmladek@...e.com>,
        "mbenes@...e.cz" <mbenes@...e.cz>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "jeyu@...nel.org" <jeyu@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-modules@...r.kernel.org" <linux-modules@...r.kernel.org>,
        "live-patching@...r.kernel.org" <live-patching@...r.kernel.org>,
        "atomlin@...mlin.com" <atomlin@...mlin.com>,
        "ghalat@...hat.com" <ghalat@...hat.com>,
        "allen.lkml@...il.com" <allen.lkml@...il.com>,
        "void@...ifault.com" <void@...ifault.com>,
        "joe@...ches.com" <joe@...ches.com>,
        "msuchanek@...e.de" <msuchanek@...e.de>,
        "oleksandr@...alenko.name" <oleksandr@...alenko.name>
Subject: Re: [PATCH v5 05/13] module: Move latched RB-tree support to a
 separate file

On Thu 2022-02-10 12:03 +0000, Christophe Leroy wrote:
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 680b31ff57fa..fd6161d78127 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -342,9 +342,9 @@ struct module_layout {
> >   #ifdef CONFIG_MODULES_TREE_LOOKUP
> >   /* Only touch one cacheline for common rbtree-for-core-layout case. */
> >   #define __module_layout_align ____cacheline_aligned
> > -#else
> > +#else /* !CONFIG_MODULES_TREE_LOOKUP */
> >   #define __module_layout_align
> > -#endif
> > +#endif /* CONFIG_MODULES_TREE_LOOKUP */
> Commenting an #else / #endif is only usefull when the block is more than
> one screen or when there are nested #ifdef inside the block.

For me, this is a personal style preference. That being said, fair enough.

> > +#else /* !CONFIG_MODULES_TREE_LOOKUP */
> > +static unsigned long module_addr_min = -1UL, module_addr_max;
>
> This is wrong to put that in a .h.
>

I understand. This was an oversight. I'll move this to kernel/module/main.c
in preparation for your work.

> > +static void mod_tree_insert(struct module *mod) { }
> > +static void mod_tree_remove_init(struct module *mod) { }
> > +static void mod_tree_remove(struct module *mod) { }
> > +static struct module *mod_find(unsigned long addr)
>
> Also keep mod_find() in main.c, or make it a 'static inline'. Otherwise
> it will be duplicated in every file including internal.h

Agreed. This too was an oversight. I'll use the 'inline' keyword here.

> > diff --git a/kernel/module/tree_lookup.c b/kernel/module/tree_lookup.c
> > new file mode 100644
> > index 000000000000..037d6eb2f56f
> > --- /dev/null
> > +++ b/kernel/module/tree_lookup.c
> > @@ -0,0 +1,109 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Modules tree lookup
> > + *
> > + * Copyright (C) 2015 Peter Zijlstra
> > + * Copyright (C) 2015 Rusty Russell
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/rbtree_latch.h>
> > +#include "internal.h"
> > +
> > +/*
> > + * Use a latched RB-tree for __module_address(); this allows us to use
> > + * RCU-sched lookups of the address from any context.
> > + *
> > + * This is conditional on PERF_EVENTS || TRACING because those can really hit
> > + * __module_address() hard by doing a lot of stack unwinding; potentially from
> > + * NMI context.
> > + */
> > +
> > +__always_inline unsigned long __mod_tree_val(struct latch_tree_node *n)
>
> Should be static.
> > +__always_inline unsigned long __mod_tree_size(struct latch_tree_node *n)
>
> Should be static.
> > +__always_inline bool
> > +mod_tree_less(struct latch_tree_node *a, struct latch_tree_node *b)
>
> Should be static.
>
>
> > +__always_inline int
> > +mod_tree_comp(void *key, struct latch_tree_node *n)
>
> Should be static.
>
> > +const struct latch_tree_ops mod_tree_ops = {
> > +    .less = mod_tree_less,
> > +    .comp = mod_tree_comp,
> > +};
>
> Should be static.

Agreed. Only used in kernel/module/tree_lookup.c.


-- 
Aaron Tomlin