Re: [PATCH] jump_label: align jump_entry table to at least 4-bytes

[David Daney]

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@xxxxxxxxxx> 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.