| Message ID | e5e58875bd15551d0386552d3f9fa9ee8bc183a2.1643015752.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Allocate module text and data separately | expand |
On Mon, Jan 24, 2022 at 09:22:15AM +0000, Christophe Leroy wrote:
> +static inline bool within_range(unsigned long addr, void *base, unsigned int size)
Please avoid the overly long line.
.. But given that this function only has a single caller I see no
point in factoring it out anyway.
Le 24/01/2022 à 13:32, Christoph Hellwig a écrit : > On Mon, Jan 24, 2022 at 09:22:15AM +0000, Christophe Leroy wrote: >> +static inline bool within_range(unsigned long addr, void *base, unsigned int size) > > Please avoid the overly long line. > > .. But given that this function only has a single caller I see no > point in factoring it out anyway. Patch 2 brings a second caller. Having it in patch 1 reduces churn in patch 2. Is it the wrong way to do ? Christophe
On Mon, Jan 24, 2022 at 09:22:15AM +0000, Christophe Leroy wrote: > within_module_core() and within_module_init() are doing the exact same > test, one on core_layout, the second on init_layout. > > In preparation of increasing the complexity of that verification, > refactor it into a single function called within_module_layout(). > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > include/linux/module.h | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/include/linux/module.h b/include/linux/module.h > index c9f1200b2312..33b4db8f5ca5 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -565,18 +565,27 @@ bool __is_module_percpu_address(unsigned long addr, unsigned long *can_addr); > bool is_module_percpu_address(unsigned long addr); > bool is_module_text_address(unsigned long addr); > > +static inline bool within_range(unsigned long addr, void *base, unsigned int size) > +{ > + return addr >= (unsigned long)base && addr < (unsigned long)base + size; > +} There's also 'within' at least in arch/x86/mm/pat/set_memory.c and surely tons of open-coded "address within" code. Should it live in, say, include/linux/range.h? > + > +static inline bool within_module_layout(unsigned long addr, > + const struct module_layout *layout) > +{ > + return within_range(addr, layout->base, layout->size); > +} > + > static inline bool within_module_core(unsigned long addr, > const struct module *mod) > { > - return (unsigned long)mod->core_layout.base <= addr && > - addr < (unsigned long)mod->core_layout.base + mod->core_layout.size; > + return within_module_layout(addr, &mod->core_layout); > } > > static inline bool within_module_init(unsigned long addr, > const struct module *mod) > { > - return (unsigned long)mod->init_layout.base <= addr && > - addr < (unsigned long)mod->init_layout.base + mod->init_layout.size; > + return within_module_layout(addr, &mod->init_layout); > } > > static inline bool within_module(unsigned long addr, const struct module *mod) > -- > 2.33.1 >
Le 24/01/2022 à 13:32, Christoph Hellwig a écrit : > On Mon, Jan 24, 2022 at 09:22:15AM +0000, Christophe Leroy wrote: >> +static inline bool within_range(unsigned long addr, void *base, unsigned int size) > > Please avoid the overly long line. > > .. But given that this function only has a single caller I see no > point in factoring it out anyway. I finally decided to drop this change from the series as it brings little added value. Thanks Christophe
Le 26/01/2022 à 22:36, Mike Rapoport a écrit : > On Mon, Jan 24, 2022 at 09:22:15AM +0000, Christophe Leroy wrote: >> within_module_core() and within_module_init() are doing the exact same >> test, one on core_layout, the second on init_layout. >> >> In preparation of increasing the complexity of that verification, >> refactor it into a single function called within_module_layout(). >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> include/linux/module.h | 17 +++++++++++++---- >> 1 file changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/include/linux/module.h b/include/linux/module.h >> index c9f1200b2312..33b4db8f5ca5 100644 >> --- a/include/linux/module.h >> +++ b/include/linux/module.h >> @@ -565,18 +565,27 @@ bool __is_module_percpu_address(unsigned long addr, unsigned long *can_addr); >> bool is_module_percpu_address(unsigned long addr); >> bool is_module_text_address(unsigned long addr); >> >> +static inline bool within_range(unsigned long addr, void *base, unsigned int size) >> +{ >> + return addr >= (unsigned long)base && addr < (unsigned long)base + size; >> +} > > There's also 'within' at least in arch/x86/mm/pat/set_memory.c and surely > tons of open-coded "address within" code. > > Should it live in, say, include/linux/range.h? > include/linux/range.h has functions that work with struct ranges. It might be an alternative, to be investigated a bit more. At the time being, this change finally brings little added value so I drop the two first patches from the series. Thanks Christophe
diff --git a/include/linux/module.h b/include/linux/module.h index c9f1200b2312..33b4db8f5ca5 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -565,18 +565,27 @@ bool __is_module_percpu_address(unsigned long addr, unsigned long *can_addr); bool is_module_percpu_address(unsigned long addr); bool is_module_text_address(unsigned long addr); +static inline bool within_range(unsigned long addr, void *base, unsigned int size) +{ + return addr >= (unsigned long)base && addr < (unsigned long)base + size; +} + +static inline bool within_module_layout(unsigned long addr, + const struct module_layout *layout) +{ + return within_range(addr, layout->base, layout->size); +} + static inline bool within_module_core(unsigned long addr, const struct module *mod) { - return (unsigned long)mod->core_layout.base <= addr && - addr < (unsigned long)mod->core_layout.base + mod->core_layout.size; + return within_module_layout(addr, &mod->core_layout); } static inline bool within_module_init(unsigned long addr, const struct module *mod) { - return (unsigned long)mod->init_layout.base <= addr && - addr < (unsigned long)mod->init_layout.base + mod->init_layout.size; + return within_module_layout(addr, &mod->init_layout); } static inline bool within_module(unsigned long addr, const struct module *mod)
within_module_core() and within_module_init() are doing the exact same test, one on core_layout, the second on init_layout. In preparation of increasing the complexity of that verification, refactor it into a single function called within_module_layout(). Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- include/linux/module.h | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)