arch/arm/Kconfig: enable ARM_MODULE_PLTS when LOCKDEP=y

Message ID 20180129234900.11121-1-anders.roxell@linaro.org
State New
Headers show
Series
  • arch/arm/Kconfig: enable ARM_MODULE_PLTS when LOCKDEP=y
Related show

Commit Message

Anders Roxell Jan. 29, 2018, 11:49 p.m.
While testing multi_v7_defconfig with LOCKDEP enabled, the kernel
fails to load simple modules, as reported by kselftest:

[   34.107620] test_printf: section 4 reloc 2 sym 'memset': relocation
28 out of range (0xbf046044 -> 0xc109f720)
selftests: printf.sh [FAIL]

The problem that is seen when LOCKDEP is enabled without
ARM_MODULE_PLTS, is that LOCKDEP eats so much memory that the top of the
kernel gets out of reach from the bottom of the module area.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Anders Roxell <anders.roxell@linaro.org>

---
 arch/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

-- 
2.11.0

Comments

Russell King - ARM Linux Jan. 29, 2018, 11:57 p.m. | #1
On Tue, Jan 30, 2018 at 12:49:00AM +0100, Anders Roxell wrote:
> While testing multi_v7_defconfig with LOCKDEP enabled, the kernel

> fails to load simple modules, as reported by kselftest:

> 

> [   34.107620] test_printf: section 4 reloc 2 sym 'memset': relocation

> 28 out of range (0xbf046044 -> 0xc109f720)

> selftests: printf.sh [FAIL]

> 

> The problem that is seen when LOCKDEP is enabled without

> ARM_MODULE_PLTS, is that LOCKDEP eats so much memory that the top of the

> kernel gets out of reach from the bottom of the module area.


This really doesn't follow IMHO - enabling various features can cause
this, and we're not going to end up stuffing the Kconfig full of these
select statements each time we find a combination of Kconfig symbols
that cause it.

lockdep isn't that special - I can (and do) build kernels with lockdep
enabled, with modules, and I do not run into this problem.  So it's
not as simple as you make out in this commit description.

It's likely that you have either a fairly full kernel configuration (it
must be to place memset() more than 16MB) or you are not placing the
kernel at 0xc0008000 due to memory reservations in the low memory.

> Suggested-by: Arnd Bergmann <arnd@arndb.de>


I guess this was discussed privately with Arnd, since there's no record
of the discussion on the lists - which is even more reason why the
commit message needs to describe better why you need this change.

> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>

> ---

>  arch/arm/Kconfig | 1 +

>  1 file changed, 1 insertion(+)

> 

> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig

> index 51c8df561077..a339157497c6 100644

> --- a/arch/arm/Kconfig

> +++ b/arch/arm/Kconfig

> @@ -20,6 +20,7 @@ config ARM

>  	select ARCH_USE_BUILTIN_BSWAP

>  	select ARCH_USE_CMPXCHG_LOCKREF

>  	select ARCH_WANT_IPC_PARSE_VERSION

> +	select ARM_MODULE_PLTS if LOCKDEP

>  	select BUILDTIME_EXTABLE_SORT if MMU

>  	select CLONE_BACKWARDS

>  	select CPU_PM if (SUSPEND || CPU_IDLE)

> -- 

> 2.11.0

> 


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
kbuild test robot Jan. 31, 2018, 11:15 a.m. | #2
Hi Anders,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v4.15 next-20180126]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Anders-Roxell/arch-arm-Kconfig-enable-ARM_MODULE_PLTS-when-LOCKDEP-y/20180131-115917
config: arm-moxart_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All warnings (new ones prefixed by >>):

warning: (ARM) selects ARM_MODULE_PLTS which has unmet direct dependencies (MODULES)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Arnd Bergmann Jan. 31, 2018, 11:25 a.m. | #3
On Tue, Jan 30, 2018 at 12:57 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Jan 30, 2018 at 12:49:00AM +0100, Anders Roxell wrote:

>> While testing multi_v7_defconfig with LOCKDEP enabled, the kernel

>> fails to load simple modules, as reported by kselftest:

>>

>> [   34.107620] test_printf: section 4 reloc 2 sym 'memset': relocation

>> 28 out of range (0xbf046044 -> 0xc109f720)

>> selftests: printf.sh [FAIL]

>>

>> The problem that is seen when LOCKDEP is enabled without

>> ARM_MODULE_PLTS, is that LOCKDEP eats so much memory that the top of the

>> kernel gets out of reach from the bottom of the module area.

>

> This really doesn't follow IMHO - enabling various features can cause

> this, and we're not going to end up stuffing the Kconfig full of these

> select statements each time we find a combination of Kconfig symbols

> that cause it.

>

> lockdep isn't that special - I can (and do) build kernels with lockdep

> enabled, with modules, and I do not run into this problem.  So it's

> not as simple as you make out in this commit description.

>

> It's likely that you have either a fairly full kernel configuration (it

> must be to place memset() more than 16MB) or you are not placing the

> kernel at 0xc0008000 due to memory reservations in the low memory.

>

>> Suggested-by: Arnd Bergmann <arnd@arndb.de>

>

> I guess this was discussed privately with Arnd, since there's no record

> of the discussion on the lists - which is even more reason why the

> commit message needs to describe better why you need this change.


I don't remember the original discussion exactly, but it's clear that
the addition of LOCKDEP to the build is what caused the size to
grow dramatically and prevent module loading.

There are two alternative ways to do this without forcing
ARM_MODULE_PLTS on all the time (which also triggered
the 0day bot warning):

a) we could make ARM_MODULE_PLTS default to 'y' when
LOCKDEP is anbled, making it a more reasonable default while
also letting users turn it off when the lockdep-enabled kernel
is still small enough

b) The Kconfig fragment that Anders uses which turns on LOCKDEP
could simply enable ARM_MODULE_PLTS as well. That would
be the easiest solution for him. I assume he already does it, but
it's likely that others run into the same issue with kselftest testing.

       Arnd
Russell King - ARM Linux Jan. 31, 2018, 11:46 a.m. | #4
On Wed, Jan 31, 2018 at 12:25:33PM +0100, Arnd Bergmann wrote:
> On Tue, Jan 30, 2018 at 12:57 AM, Russell King - ARM Linux

> <linux@armlinux.org.uk> wrote:

> > On Tue, Jan 30, 2018 at 12:49:00AM +0100, Anders Roxell wrote:

> >> While testing multi_v7_defconfig with LOCKDEP enabled, the kernel

> >> fails to load simple modules, as reported by kselftest:

> >>

> >> [   34.107620] test_printf: section 4 reloc 2 sym 'memset': relocation

> >> 28 out of range (0xbf046044 -> 0xc109f720)

> >> selftests: printf.sh [FAIL]

> >>

> >> The problem that is seen when LOCKDEP is enabled without

> >> ARM_MODULE_PLTS, is that LOCKDEP eats so much memory that the top of the

> >> kernel gets out of reach from the bottom of the module area.

> >

> > This really doesn't follow IMHO - enabling various features can cause

> > this, and we're not going to end up stuffing the Kconfig full of these

> > select statements each time we find a combination of Kconfig symbols

> > that cause it.

> >

> > lockdep isn't that special - I can (and do) build kernels with lockdep

> > enabled, with modules, and I do not run into this problem.  So it's

> > not as simple as you make out in this commit description.

> >

> > It's likely that you have either a fairly full kernel configuration (it

> > must be to place memset() more than 16MB) or you are not placing the

> > kernel at 0xc0008000 due to memory reservations in the low memory.

> >

> >> Suggested-by: Arnd Bergmann <arnd@arndb.de>

> >

> > I guess this was discussed privately with Arnd, since there's no record

> > of the discussion on the lists - which is even more reason why the

> > commit message needs to describe better why you need this change.

> 

> I don't remember the original discussion exactly, but it's clear that

> the addition of LOCKDEP to the build is what caused the size to

> grow dramatically and prevent module loading.


As I say above, it does not follow "lockdep is enabled" therefore "we
need module plts" so I don't accept this argument.

Yes, if you have a lot of features built in, then its entirely possible
that enabling lockdep will cause the kernel to overflow the non-plt
boundary.  It will also be true that enabling a few more other features
will also cause the kernel to overflow the non-plt boundary as well.
Should we add those other features to a Kconfig expression too?  At
what point do we stop adding these?

> There are two alternative ways to do this without forcing

> ARM_MODULE_PLTS on all the time (which also triggered

> the 0day bot warning):


Yes, 0day is pointing out yet again what a silly idea it is to select
symbols that (a) have dependencies and (b) are user visible, something
that I've long disagreed about.

> a) we could make ARM_MODULE_PLTS default to 'y' when

> LOCKDEP is anbled, making it a more reasonable default while

> also letting users turn it off when the lockdep-enabled kernel

> is still small enough


As I've said, I don't believe "LOCKDEP" therefore need "MODULE_PLTS"
follows.  It's just a symptom of an already large kernel.  I suspect
without lockdep, Ander's kernel is already approaching the
problematical 16MB mark.

Another option that causes the kernel to grow by a few megabytes is
the kernel protection options (ronx etc).  I suspect if Anders built
a kernel that had lockdep enabled and ronx etc disabled, that there
would be no need for module plts.  Should we turn on module plts if
lockdep or ronx is enabled?

I don't think there's a _sane_ solution to this other than defaulting
ARM_MODULE_PLTS to 'y' without any of these dependencies, and if people
want to turn it off, they still can if they're sure they won't run into
this situation.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
kbuild test robot Jan. 31, 2018, 12:12 p.m. | #5
Hi Anders,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.15 next-20180126]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Anders-Roxell/arch-arm-Kconfig-enable-ARM_MODULE_PLTS-when-LOCKDEP-y/20180131-115917
config: arm-moxart_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   arch/arm/kernel/module-plts.c: In function 'in_init':
>> arch/arm/kernel/module-plts.c:36:23: error: dereferencing pointer to incomplete type 'const struct module'

     return loc - (u32)mod->init_layout.base < mod->init_layout.size;
                          ^~
   arch/arm/kernel/module-plts.c: In function 'get_module_plt':
>> arch/arm/kernel/module-plts.c:41:56: error: dereferencing pointer to incomplete type 'struct module'

     struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core :
                                                           ^~

vim +36 arch/arm/kernel/module-plts.c

7d485f64 Ard Biesheuvel 2014-11-24  33  
b7ede5a1 Ard Biesheuvel 2017-02-22  34  static bool in_init(const struct module *mod, unsigned long loc)
b7ede5a1 Ard Biesheuvel 2017-02-22  35  {
b7ede5a1 Ard Biesheuvel 2017-02-22 @36  	return loc - (u32)mod->init_layout.base < mod->init_layout.size;
b7ede5a1 Ard Biesheuvel 2017-02-22  37  }
b7ede5a1 Ard Biesheuvel 2017-02-22  38  
7d485f64 Ard Biesheuvel 2014-11-24  39  u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val)
7d485f64 Ard Biesheuvel 2014-11-24  40  {
b7ede5a1 Ard Biesheuvel 2017-02-22 @41  	struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core :
b7ede5a1 Ard Biesheuvel 2017-02-22  42  							  &mod->arch.init;
b7ede5a1 Ard Biesheuvel 2017-02-22  43  
b7ede5a1 Ard Biesheuvel 2017-02-22  44  	struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr;
66e94ba3 Ard Biesheuvel 2016-08-18  45  	int idx = 0;
35fa91ee Ard Biesheuvel 2016-08-16  46  
66e94ba3 Ard Biesheuvel 2016-08-18  47  	/*
66e94ba3 Ard Biesheuvel 2016-08-18  48  	 * Look for an existing entry pointing to 'val'. Given that the
66e94ba3 Ard Biesheuvel 2016-08-18  49  	 * relocations are sorted, this will be the last entry we allocated.
66e94ba3 Ard Biesheuvel 2016-08-18  50  	 * (if one exists).
66e94ba3 Ard Biesheuvel 2016-08-18  51  	 */
b7ede5a1 Ard Biesheuvel 2017-02-22  52  	if (pltsec->plt_count > 0) {
b7ede5a1 Ard Biesheuvel 2017-02-22  53  		plt += (pltsec->plt_count - 1) / PLT_ENT_COUNT;
b7ede5a1 Ard Biesheuvel 2017-02-22  54  		idx = (pltsec->plt_count - 1) % PLT_ENT_COUNT;
7d485f64 Ard Biesheuvel 2014-11-24  55  
66e94ba3 Ard Biesheuvel 2016-08-18  56  		if (plt->lit[idx] == val)
66e94ba3 Ard Biesheuvel 2016-08-18  57  			return (u32)&plt->ldr[idx];
66e94ba3 Ard Biesheuvel 2016-08-18  58  
66e94ba3 Ard Biesheuvel 2016-08-18  59  		idx = (idx + 1) % PLT_ENT_COUNT;
66e94ba3 Ard Biesheuvel 2016-08-18  60  		if (!idx)
66e94ba3 Ard Biesheuvel 2016-08-18  61  			plt++;
66e94ba3 Ard Biesheuvel 2016-08-18  62  	}
66e94ba3 Ard Biesheuvel 2016-08-18  63  
b7ede5a1 Ard Biesheuvel 2017-02-22  64  	pltsec->plt_count++;
b7ede5a1 Ard Biesheuvel 2017-02-22  65  	BUG_ON(pltsec->plt_count * PLT_ENT_SIZE > pltsec->plt->sh_size);
7d485f64 Ard Biesheuvel 2014-11-24  66  
66e94ba3 Ard Biesheuvel 2016-08-18  67  	if (!idx)
7d485f64 Ard Biesheuvel 2014-11-24  68  		/* Populate a new set of entries */
7d485f64 Ard Biesheuvel 2014-11-24  69  		*plt = (struct plt_entries){
7d485f64 Ard Biesheuvel 2014-11-24  70  			{ [0 ... PLT_ENT_COUNT - 1] = PLT_ENT_LDR, },
7d485f64 Ard Biesheuvel 2014-11-24  71  			{ val, }
7d485f64 Ard Biesheuvel 2014-11-24  72  		};
66e94ba3 Ard Biesheuvel 2016-08-18  73  	else
66e94ba3 Ard Biesheuvel 2016-08-18  74  		plt->lit[idx] = val;
66e94ba3 Ard Biesheuvel 2016-08-18  75  
66e94ba3 Ard Biesheuvel 2016-08-18  76  	return (u32)&plt->ldr[idx];
7d485f64 Ard Biesheuvel 2014-11-24  77  }
7d485f64 Ard Biesheuvel 2014-11-24  78  

:::::: The code at line 36 was first introduced by commit
:::::: b7ede5a1f5905ac394cc8e61712a13e3c5cb7b8f ARM: 8662/1: module: split core and init PLT sections

:::::: TO: Ard Biesheuvel <ard.biesheuvel@linaro.org>
:::::: CC: Russell King <rmk+kernel@armlinux.org.uk>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Anders Roxell Jan. 31, 2018, 8:14 p.m. | #6
On 31 January 2018 at 12:46, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Wed, Jan 31, 2018 at 12:25:33PM +0100, Arnd Bergmann wrote:

>> On Tue, Jan 30, 2018 at 12:57 AM, Russell King - ARM Linux

>> <linux@armlinux.org.uk> wrote:

>> > On Tue, Jan 30, 2018 at 12:49:00AM +0100, Anders Roxell wrote:

>> >> While testing multi_v7_defconfig with LOCKDEP enabled, the kernel

>> >> fails to load simple modules, as reported by kselftest:

>> >>

>> >> [   34.107620] test_printf: section 4 reloc 2 sym 'memset': relocation

>> >> 28 out of range (0xbf046044 -> 0xc109f720)

>> >> selftests: printf.sh [FAIL]

>> >>

>> >> The problem that is seen when LOCKDEP is enabled without

>> >> ARM_MODULE_PLTS, is that LOCKDEP eats so much memory that the top of the

>> >> kernel gets out of reach from the bottom of the module area.

>> >

>> > This really doesn't follow IMHO - enabling various features can cause

>> > this, and we're not going to end up stuffing the Kconfig full of these

>> > select statements each time we find a combination of Kconfig symbols

>> > that cause it.

>> >

>> > lockdep isn't that special - I can (and do) build kernels with lockdep

>> > enabled, with modules, and I do not run into this problem.  So it's

>> > not as simple as you make out in this commit description.

>> >

>> > It's likely that you have either a fairly full kernel configuration (it

>> > must be to place memset() more than 16MB) or you are not placing the

>> > kernel at 0xc0008000 due to memory reservations in the low memory.

>> >

>> >> Suggested-by: Arnd Bergmann <arnd@arndb.de>

>> >

>> > I guess this was discussed privately with Arnd, since there's no record

>> > of the discussion on the lists - which is even more reason why the

>> > commit message needs to describe better why you need this change.

>>

>> I don't remember the original discussion exactly, but it's clear that

>> the addition of LOCKDEP to the build is what caused the size to

>> grow dramatically and prevent module loading.

>

> As I say above, it does not follow "lockdep is enabled" therefore "we

> need module plts" so I don't accept this argument.

>

> Yes, if you have a lot of features built in, then its entirely possible

> that enabling lockdep will cause the kernel to overflow the non-plt

> boundary.  It will also be true that enabling a few more other features

> will also cause the kernel to overflow the non-plt boundary as well.

> Should we add those other features to a Kconfig expression too?  At

> what point do we stop adding these?

>

>> There are two alternative ways to do this without forcing

>> ARM_MODULE_PLTS on all the time (which also triggered

>> the 0day bot warning):

>

> Yes, 0day is pointing out yet again what a silly idea it is to select

> symbols that (a) have dependencies and (b) are user visible, something

> that I've long disagreed about.

>

>> a) we could make ARM_MODULE_PLTS default to 'y' when

>> LOCKDEP is anbled, making it a more reasonable default while

>> also letting users turn it off when the lockdep-enabled kernel

>> is still small enough

>

> As I've said, I don't believe "LOCKDEP" therefore need "MODULE_PLTS"

> follows.  It's just a symptom of an already large kernel.  I suspect

> without lockdep, Ander's kernel is already approaching the

> problematical 16MB mark.

>

> Another option that causes the kernel to grow by a few megabytes is

> the kernel protection options (ronx etc).  I suspect if Anders built

> a kernel that had lockdep enabled and ronx etc disabled, that there

> would be no need for module plts.  Should we turn on module plts if

> lockdep or ronx is enabled?

>

> I don't think there's a _sane_ solution to this other than defaulting

> ARM_MODULE_PLTS to 'y' without any of these dependencies, and if people

> want to turn it off, they still can if they're sure they won't run into

> this situation.


I'll send out a v2 with this change shortly.

Thank you all for your comments.

Cheers,
Anders

Patch

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 51c8df561077..a339157497c6 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -20,6 +20,7 @@  config ARM
 	select ARCH_USE_BUILTIN_BSWAP
 	select ARCH_USE_CMPXCHG_LOCKREF
 	select ARCH_WANT_IPC_PARSE_VERSION
+	select ARM_MODULE_PLTS if LOCKDEP
 	select BUILDTIME_EXTABLE_SORT if MMU
 	select CLONE_BACKWARDS
 	select CPU_PM if (SUSPEND || CPU_IDLE)