| Message ID | 912fa97a-aa1d-c0e4-dc83-fc5c745db1c1@caviumnetworks.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/28/2017 03:15 PM, David Daney wrote: > On 02/28/2017 11:34 AM, Jason Baron wrote: >> >> >> On 02/28/2017 02:22 PM, David Daney wrote: >>> On 02/28/2017 11:05 AM, David Daney wrote: >>>> On 02/28/2017 10:39 AM, Jason Baron wrote: >>>>> >>> [...] >>>>>> I suspect that the alignment of the __jump_table section in the .ko >>>>>> files is not correct, and you are seeing some sort of problem due to >>>>>> that. >>>>>> >>>>>> >>>>> >>>>> Hi, >>>>> >>>>> Yes, if you look at the trace that Sachin sent the module being loaded >>>>> that does the WARN_ON() is nfsd.ko. >>>>> >>>>> That module from Sachin's trace has: >>>>> >>>>> [31] __jump_table PROGBITS 0000000000000000 03fd77 >>>>> 0000c0 >>>>> 18 WAM 0 0 1 >>>> >>>> The problem is then the section alignment (last column) for power. >>>> >>>> On mips with no patches applied, we get: >>>> >>>> [17] __jump_table PROGBITS 0000000000000000 00d2c0 000048 >>>> 00 WA 0 0 8 >>>> >>>> Look, proper alignment! >>>> >>>> The question I have is why do the power ".llong" and ".long" assembler >>>> directives not force section alignment? Is there an alternative that >>>> could be used that would result in the proper alignment? Would ".word" >>>> work? >>>> >>>> If not, then I would say patch only power with your balign thing. >>>> 8-byte >>>> alignment for 64-bit kernel, 4-byte alignment for 32-bit kernel >>>> >>> >>> I think the proper fix is either: >>> >>> A) Modify scripts/module-common.lds to force __jump_table alignment for >>> all architectures. >>> >>> B) Add arch/powerpc/kernel/module.lds to force __jump_table alignment >>> for powerpc only. >>> >>> David. >>> >>> >> >> Ok, I can try adding it to the linger script. >> >> FWIW, here is my before and after with the .balign thing for the nfsd.ko >> module on powperc (using a cross-compiler): >> >> before: >> >> [31] __jump_table PROGBITS 0000000000000000 03ee3e 0000f0 >> 00 WA 0 0 1 >> >> after: >> >> [31] __jump_table PROGBITS 0000000000000000 03ee40 0000f0 >> 00 WA 0 0 4 >> > > Try the (lightly tested) attached. > > If it works and Steven likes it, perhaps someone can merge it. > > David. > > > So before your module.lds script: # powerpc64-linux-readelf -eW fs/nfsd/nfsd.o | grep jump [31] __jump_table PROGBITS 0000000000000000 03edfe 0000f0 00 WA 0 0 1 # powerpc64-linux-readelf -eW fs/nfsd/nfsd.ko | grep jump [32] __jump_table PROGBITS 0000000000000000 044046 0000f0 00 WA 0 0 1 With your patch: # powerpc64-linux-readelf -eW fs/nfsd/nfsd.o | grep jump [31] __jump_table PROGBITS 0000000000000000 03edfe 0000f0 00 WA 0 0 1 # powerpc64-linux-readelf -eW fs/nfsd/nfsd.ko | grep jump [18] __jump_table PROGBITS 0000000000000000 03e358 0000f0 00 WA 0 0 8 I also checked all the other .ko files and they were properly aligned. So I think this should hopefully work, and I like that its not a per-arch fix. Sachin, sorry to bother you again, but I'm hoping you can try David's latest patch to scripts/module-common.lds, just to test in your setup. Thanks, -Jason
Jason Baron <jbaron@akamai.com> writes: ... > I also checked all the other .ko files and they were properly aligned. > So I think this should hopefully work, and I like that its not a > per-arch fix. > > Sachin, sorry to bother you again, but I'm hoping you can try David's > latest patch to scripts/module-common.lds, just to test in your setup. It does fix the problem. I was reproducing with crc_t10dif: [ 695.890552] ------------[ cut here ]------------ [ 695.890709] WARNING: CPU: 15 PID: 3019 at ../kernel/jump_label.c:287 static_key_set_entries+0x74/0xa0 [ 695.890710] Modules linked in: crc_t10dif(+) crct10dif_generic crct10dif_common ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 xt_addrtype iptable_filter ip_tables xt_conntrack x_tables nf_nat nf_conntrack bridge stp llc dm_thin_pool dm_persistent_data dm_bio_prison dm_bufio libcrc32c kvm virtio_balloon binfmt_misc autofs4 virtio_net virtio_pci virtio_ring virtio Which had: [21] __jump_table PROGBITS 0000000000000000 0004e8 000018 00 WA 0 0 1 And now has: [18] __jump_table PROGBITS 0000000000000000 0004d0 000018 00 WA 0 0 8 And all other modules have an alignment of 8 on __jump_table, as expected. I'm inclined to merge a version of the balign patch for powerpc anyway, just to be on the safe side. I guess the old code was coping fine with the unaligned keys, but it still makes me nervous. cheers
On 02/28/2017 10:34 PM, Michael Ellerman wrote: > Jason Baron <jbaron@akamai.com> writes: > ... >> I also checked all the other .ko files and they were properly aligned. >> So I think this should hopefully work, and I like that its not a >> per-arch fix. >> >> Sachin, sorry to bother you again, but I'm hoping you can try David's >> latest patch to scripts/module-common.lds, just to test in your setup. > > It does fix the problem. > > I was reproducing with crc_t10dif: > > [ 695.890552] ------------[ cut here ]------------ > [ 695.890709] WARNING: CPU: 15 PID: 3019 at ../kernel/jump_label.c:287 static_key_set_entries+0x74/0xa0 > [ 695.890710] Modules linked in: crc_t10dif(+) crct10dif_generic crct10dif_common ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 xt_addrtype iptable_filter ip_tables xt_conntrack x_tables nf_nat nf_conntrack bridge stp llc dm_thin_pool dm_persistent_data dm_bio_prison dm_bufio libcrc32c kvm virtio_balloon binfmt_misc autofs4 virtio_net virtio_pci virtio_ring virtio > > Which had: > > [21] __jump_table PROGBITS 0000000000000000 0004e8 000018 00 WA 0 0 1 > > > And now has: > > [18] __jump_table PROGBITS 0000000000000000 0004d0 000018 00 WA 0 0 8 > > And all other modules have an alignment of 8 on __jump_table, as expected. > > I'm inclined to merge a version of the balign patch for powerpc anyway, > just to be on the safe side. I guess the old code was coping fine with > the unaligned keys, but it still makes me nervous. The original "balign patch" has a couple of problems: 1) 4-byte alignment is not sufficient for 64-bit kernels 2) It is redundant if the linker script patch is accepted. > > cheers > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On 03/01/2017 11:40 AM, David Daney wrote: > On 02/28/2017 10:34 PM, Michael Ellerman wrote: >> Jason Baron <jbaron@akamai.com> writes: >> ... >>> I also checked all the other .ko files and they were properly aligned. >>> So I think this should hopefully work, and I like that its not a >>> per-arch fix. >>> >>> Sachin, sorry to bother you again, but I'm hoping you can try David's >>> latest patch to scripts/module-common.lds, just to test in your setup. >> >> It does fix the problem. >> >> I was reproducing with crc_t10dif: >> >> [ 695.890552] ------------[ cut here ]------------ >> [ 695.890709] WARNING: CPU: 15 PID: 3019 at >> ../kernel/jump_label.c:287 static_key_set_entries+0x74/0xa0 >> [ 695.890710] Modules linked in: crc_t10dif(+) crct10dif_generic >> crct10dif_common ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat >> nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 xt_addrtype >> iptable_filter ip_tables xt_conntrack x_tables nf_nat nf_conntrack >> bridge stp llc dm_thin_pool dm_persistent_data dm_bio_prison dm_bufio >> libcrc32c kvm virtio_balloon binfmt_misc autofs4 virtio_net virtio_pci >> virtio_ring virtio >> >> Which had: >> >> [21] __jump_table PROGBITS 0000000000000000 0004e8 >> 000018 00 WA 0 0 1 >> >> >> And now has: >> >> [18] __jump_table PROGBITS 0000000000000000 0004d0 >> 000018 00 WA 0 0 8 >> >> And all other modules have an alignment of 8 on __jump_table, as >> expected. >> >> I'm inclined to merge a version of the balign patch for powerpc anyway, >> just to be on the safe side. I guess the old code was coping fine with >> the unaligned keys, but it still makes me nervous. > > > The original "balign patch" has a couple of problems: > > 1) 4-byte alignment is not sufficient for 64-bit kernels > > 2) It is redundant if the linker script patch is accepted. > > The linker script patch seems reasonable to me. Maybe its worth adding a comment that the alignment is necessary because the core jump_label makes use of the 2 lsb bits of its __jump_table pointer due to commit: 3821fd3 jump_label: Reduce the size of struct static_key Also, in the comment it says that it fixes an oops. We hit a WARN_ON() not an oops, although bad things are likely to happen when the branch is updated. Thanks, -Jason
On 03/01/2017 12:02 PM, Jason Baron wrote: > > > On 03/01/2017 11:40 AM, David Daney wrote: >> On 02/28/2017 10:34 PM, Michael Ellerman wrote: >>> Jason Baron <jbaron@akamai.com> writes: >>> ... >>>> I also checked all the other .ko files and they were properly aligned. >>>> So I think this should hopefully work, and I like that its not a >>>> per-arch fix. >>>> >>>> Sachin, sorry to bother you again, but I'm hoping you can try David's >>>> latest patch to scripts/module-common.lds, just to test in your setup. >>> >>> It does fix the problem. >>> >>> I was reproducing with crc_t10dif: >>> >>> [ 695.890552] ------------[ cut here ]------------ >>> [ 695.890709] WARNING: CPU: 15 PID: 3019 at >>> ../kernel/jump_label.c:287 static_key_set_entries+0x74/0xa0 >>> [ 695.890710] Modules linked in: crc_t10dif(+) crct10dif_generic >>> crct10dif_common ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat >>> nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 xt_addrtype >>> iptable_filter ip_tables xt_conntrack x_tables nf_nat nf_conntrack >>> bridge stp llc dm_thin_pool dm_persistent_data dm_bio_prison dm_bufio >>> libcrc32c kvm virtio_balloon binfmt_misc autofs4 virtio_net virtio_pci >>> virtio_ring virtio >>> >>> Which had: >>> >>> [21] __jump_table PROGBITS 0000000000000000 0004e8 >>> 000018 00 WA 0 0 1 >>> >>> >>> And now has: >>> >>> [18] __jump_table PROGBITS 0000000000000000 0004d0 >>> 000018 00 WA 0 0 8 >>> >>> And all other modules have an alignment of 8 on __jump_table, as >>> expected. >>> >>> I'm inclined to merge a version of the balign patch for powerpc anyway, >>> just to be on the safe side. I guess the old code was coping fine with >>> the unaligned keys, but it still makes me nervous. >> >> >> The original "balign patch" has a couple of problems: >> >> 1) 4-byte alignment is not sufficient for 64-bit kernels >> >> 2) It is redundant if the linker script patch is accepted. >> >> > > The linker script patch seems reasonable to me. > > Maybe its worth adding a comment that the alignment is necessary because > the core jump_label makes use of the 2 lsb bits of its __jump_table > pointer due to commit: > > 3821fd3 jump_label: Reduce the size of struct static_key > > Also, in the comment it says that it fixes an oops. We hit a WARN_ON() > not an oops, although bad things are likely to happen when the branch is > updated. > OK, I guess I will send a proper patch e-mail with an updated changelog with the improvements you suggest. Thanks, David.
From 89d4deafbc920351b93afb1ac4b4124995e1f19d Mon Sep 17 00:00:00 2001 From: David Daney <david.daney@cavium.com> Date: Tue, 28 Feb 2017 12:10:02 -0800 Subject: [PATCH] module: set __jump_table alignment to 8 For powerpc the __jump_table section in modules is not aligned, this causes an OOPS when loading a module containing a __jump_table. Fix by forcing __jump_table to 8, which is the same alignment used for this section in the kernel proper. Signed-off-by: David Daney <david.daney@cavium.com> --- scripts/module-common.lds | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/module-common.lds b/scripts/module-common.lds index 73a2c7d..53234e8 100644 --- a/scripts/module-common.lds +++ b/scripts/module-common.lds @@ -19,4 +19,6 @@ SECTIONS { . = ALIGN(8); .init_array 0 : { *(SORT(.init_array.*)) *(.init_array) } + + __jump_table 0 : ALIGN(8) { KEEP(*(__jump_table)) } } -- 2.9.3