Jason Baron <[email protected]> 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 <[email protected]> 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
> [email protected]
> 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 <[email protected]> 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 <[email protected]> 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.