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

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

[Jason Baron]

The core jump_label code makes use of the 2 lower bits of the
static_key::[type|entries|next] field. Thus, ensure that the jump_entry
table is at least 4-byte aligned.

Reported-and-tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Anton Blanchard <anton@samba.org>
Cc: Rabin Vincent <rabin@rab.in>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Chris Metcalf <cmetcalf@mellanox.com>
Cc: Zhigang Lu <zlu@ezchip.com>
Cc: David Daney <ddaney@caviumnetworks.com>
Signed-off-by: Jason Baron <jbaron@akamai.com>
---
 arch/arm/include/asm/jump_label.h     | 2 ++
 arch/mips/include/asm/jump_label.h    | 2 ++
 arch/powerpc/include/asm/jump_label.h | 3 +++
 arch/tile/include/asm/jump_label.h    | 2 ++
 4 files changed, 9 insertions(+)

diff --git a/arch/arm/include/asm/jump_label.h b/arch/arm/include/asm/jump_label.h
index 34f7b6980d21..9c017bb04d1c 100644
--- a/arch/arm/include/asm/jump_label.h
+++ b/arch/arm/include/asm/jump_label.h
@@ -13,6 +13,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran
 	asm_volatile_goto("1:\n\t"
 		 WASM(nop) "\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"
+		 ".balign 4\n\t"
 		 ".word 1b, %l[l_yes], %c0\n\t"
 		 ".popsection\n\t"
 		 : :  "i" (&((char *)key)[branch]) :  : l_yes);
@@ -27,6 +28,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool
 	asm_volatile_goto("1:\n\t"
 		 WASM(b) " %l[l_yes]\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"
+		 ".balign 4\n\t"
 		 ".word 1b, %l[l_yes], %c0\n\t"
 		 ".popsection\n\t"
 		 : :  "i" (&((char *)key)[branch]) :  : l_yes);
diff --git a/arch/mips/include/asm/jump_label.h b/arch/mips/include/asm/jump_label.h
index e77672539e8e..243791f3ae71 100644
--- a/arch/mips/include/asm/jump_label.h
+++ b/arch/mips/include/asm/jump_label.h
@@ -31,6 +31,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran
 	asm_volatile_goto("1:\t" NOP_INSN "\n\t"
 		"nop\n\t"
 		".pushsection __jump_table,  \"aw\"\n\t"
+		".balign 4\n\t"
 		WORD_INSN " 1b, %l[l_yes], %0\n\t"
 		".popsection\n\t"
 		: :  "i" (&((char *)key)[branch]) : : l_yes);
@@ -45,6 +46,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool
 	asm_volatile_goto("1:\tj %l[l_yes]\n\t"
 		"nop\n\t"
 		".pushsection __jump_table,  \"aw\"\n\t"
+		".balign 4\n\t"
 		WORD_INSN " 1b, %l[l_yes], %0\n\t"
 		".popsection\n\t"
 		: :  "i" (&((char *)key)[branch]) : : l_yes);
diff --git a/arch/powerpc/include/asm/jump_label.h b/arch/powerpc/include/asm/jump_label.h
index 9a287e0ac8b1..bfe83496b590 100644
--- a/arch/powerpc/include/asm/jump_label.h
+++ b/arch/powerpc/include/asm/jump_label.h
@@ -24,6 +24,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran
 	asm_volatile_goto("1:\n\t"
 		 "nop # arch_static_branch\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"
+		 ".balign 4\n\t"
 		 JUMP_ENTRY_TYPE "1b, %l[l_yes], %c0\n\t"
 		 ".popsection \n\t"
 		 : :  "i" (&((char *)key)[branch]) : : l_yes);
@@ -38,6 +39,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool
 	asm_volatile_goto("1:\n\t"
 		 "b %l[l_yes] # arch_static_branch_jump\n\t"
 		 ".pushsection __jump_table,  \"aw\"\n\t"
+		 ".balign 4\n\t"
 		 JUMP_ENTRY_TYPE "1b, %l[l_yes], %c0\n\t"
 		 ".popsection \n\t"
 		 : :  "i" (&((char *)key)[branch]) : : l_yes);
@@ -63,6 +65,7 @@ struct jump_entry {
 #define ARCH_STATIC_BRANCH(LABEL, KEY)		\
 1098:	nop;					\
 	.pushsection __jump_table, "aw";	\
+	.balign 4;				\
 	FTR_ENTRY_LONG 1098b, LABEL, KEY;	\
 	.popsection
 #endif
diff --git a/arch/tile/include/asm/jump_label.h b/arch/tile/include/asm/jump_label.h
index cde7573f397b..a964e6135ea3 100644
--- a/arch/tile/include/asm/jump_label.h
+++ b/arch/tile/include/asm/jump_label.h
@@ -25,6 +25,7 @@ static __always_inline bool arch_static_branch(struct static_key *key,
 	asm_volatile_goto("1:\n\t"
 		"nop" "\n\t"
 		".pushsection __jump_table,  \"aw\"\n\t"
+		".balign 4\n\t"
 		".quad 1b, %l[l_yes], %0 + %1 \n\t"
 		".popsection\n\t"
 		: :  "i" (key), "i" (branch) : : l_yes);
@@ -39,6 +40,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key,
 	asm_volatile_goto("1:\n\t"
 		"j %l[l_yes]" "\n\t"
 		".pushsection __jump_table,  \"aw\"\n\t"
+		".balign 4\n\t"
 		".quad 1b, %l[l_yes], %0 + %1 \n\t"
 		".popsection\n\t"
 		: :  "i" (key), "i" (branch) : : l_yes);
-- 
2.6.1

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

[David Daney]

On 02/27/2017 10:49 AM, Jason Baron wrote:
> The core jump_label code makes use of the 2 lower bits of the
> static_key::[type|entries|next] field. Thus, ensure that the jump_entry
> table is at least 4-byte aligned.
>
[...]
> diff --git a/arch/mips/include/asm/jump_label.h b/arch/mips/include/asm/jump_label.h
> index e77672539e8e..243791f3ae71 100644
> --- a/arch/mips/include/asm/jump_label.h
> +++ b/arch/mips/include/asm/jump_label.h
> @@ -31,6 +31,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran
>  	asm_volatile_goto("1:\t" NOP_INSN "\n\t"
>  		"nop\n\t"
>  		".pushsection __jump_table,  \"aw\"\n\t"
> +		".balign 4\n\t"
>  		WORD_INSN " 1b, %l[l_yes], %0\n\t"
>  		".popsection\n\t"
>  		: :  "i" (&((char *)key)[branch]) : : l_yes);
> @@ -45,6 +46,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool
>  	asm_volatile_goto("1:\tj %l[l_yes]\n\t"
>  		"nop\n\t"
>  		".pushsection __jump_table,  \"aw\"\n\t"
> +		".balign 4\n\t"
>  		WORD_INSN " 1b, %l[l_yes], %0\n\t"
>  		".popsection\n\t"
>  		: :  "i" (&((char *)key)[branch]) : : l_yes);


I will speak only for the MIPS part.

If the section is not already properly aligned, this change will add 
padding, which is probably not what we want.

Have you ever seen a problem with misalignment in the real world?

If so, I think a better approach might be to set properties on the 
__jump_table section to force the proper alignment, or do something in 
the linker script.

David Daney

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

[Jason Baron]

On 02/27/2017 01:57 PM, David Daney wrote:
> On 02/27/2017 10:49 AM, Jason Baron wrote:
>> The core jump_label code makes use of the 2 lower bits of the
>> static_key::[type|entries|next] field. Thus, ensure that the jump_entry
>> table is at least 4-byte aligned.
>>
> [...]
>> diff --git a/arch/mips/include/asm/jump_label.h
>> b/arch/mips/include/asm/jump_label.h
>> index e77672539e8e..243791f3ae71 100644
>> --- a/arch/mips/include/asm/jump_label.h
>> +++ b/arch/mips/include/asm/jump_label.h
>> @@ -31,6 +31,7 @@ static __always_inline bool
>> arch_static_branch(struct static_key *key, bool bran
>>      asm_volatile_goto("1:\t" NOP_INSN "\n\t"
>>          "nop\n\t"
>>          ".pushsection __jump_table,  \"aw\"\n\t"
>> +        ".balign 4\n\t"
>>          WORD_INSN " 1b, %l[l_yes], %0\n\t"
>>          ".popsection\n\t"
>>          : :  "i" (&((char *)key)[branch]) : : l_yes);
>> @@ -45,6 +46,7 @@ static __always_inline bool
>> arch_static_branch_jump(struct static_key *key, bool
>>      asm_volatile_goto("1:\tj %l[l_yes]\n\t"
>>          "nop\n\t"
>>          ".pushsection __jump_table,  \"aw\"\n\t"
>> +        ".balign 4\n\t"
>>          WORD_INSN " 1b, %l[l_yes], %0\n\t"
>>          ".popsection\n\t"
>>          : :  "i" (&((char *)key)[branch]) : : l_yes);
>
>
> I will speak only for the MIPS part.
>
> If the section is not already properly aligned, this change will add
> padding, which is probably not what we want.
>
> Have you ever seen a problem with misalignment in the real world?
>

Hi,

Yes, there was a WARN_ON() reported on POWER here:

https://lkml.org/lkml/2017/2/19/85

The WARN_ON() triggers if either of the 2 lsb are set. More 
specifically, its coming from 'static_key_set_entries()' from the 
following patch, which was recently added to linux-next:

https://lkml.org/lkml/2017/2/3/558

So this was not seen on mips, but I included all arches in this patch 
that I though might be affected.

> If so, I think a better approach might be to set properties on the
> __jump_table section to force the proper alignment, or do something in
> the linker script.
>

So in include/asm-generic/vmlinux.lds.h, we are already setting 
'ALIGN(8)', but that does not appear to be sufficient for POWER...

Also, I checked the size of the vmlinux generated after this change on 
all 4 arches, and it was rather minimal. I think POWER increased the 
most, but the other arches increased by only a few bytes.

Thanks,

-Jason

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

[David Daney]

On 02/27/2017 11:18 AM, Jason Baron wrote:
>
>
> On 02/27/2017 01:57 PM, David Daney wrote:
>> On 02/27/2017 10:49 AM, Jason Baron wrote:
>>> The core jump_label code makes use of the 2 lower bits of the
>>> static_key::[type|entries|next] field. Thus, ensure that the jump_entry
>>> table is at least 4-byte aligned.
>>>
>> [...]
>>> diff --git a/arch/mips/include/asm/jump_label.h
>>> b/arch/mips/include/asm/jump_label.h
>>> index e77672539e8e..243791f3ae71 100644
>>> --- a/arch/mips/include/asm/jump_label.h
>>> +++ b/arch/mips/include/asm/jump_label.h
>>> @@ -31,6 +31,7 @@ static __always_inline bool
>>> arch_static_branch(struct static_key *key, bool bran
>>>      asm_volatile_goto("1:\t" NOP_INSN "\n\t"
>>>          "nop\n\t"
>>>          ".pushsection __jump_table,  \"aw\"\n\t"
>>> +        ".balign 4\n\t"
>>>          WORD_INSN " 1b, %l[l_yes], %0\n\t"
>>>          ".popsection\n\t"
>>>          : :  "i" (&((char *)key)[branch]) : : l_yes);
>>> @@ -45,6 +46,7 @@ static __always_inline bool
>>> arch_static_branch_jump(struct static_key *key, bool
>>>      asm_volatile_goto("1:\tj %l[l_yes]\n\t"
>>>          "nop\n\t"
>>>          ".pushsection __jump_table,  \"aw\"\n\t"
>>> +        ".balign 4\n\t"
>>>          WORD_INSN " 1b, %l[l_yes], %0\n\t"
>>>          ".popsection\n\t"
>>>          : :  "i" (&((char *)key)[branch]) : : l_yes);
>>
>>
>> I will speak only for the MIPS part.
>>
>> If the section is not already properly aligned, this change will add
>> padding, which is probably not what we want.
>>
>> Have you ever seen a problem with misalignment in the real world?
>>
>
> Hi,
>
> Yes, there was a WARN_ON() reported on POWER here:
>
> https://lkml.org/lkml/2017/2/19/85
>
> The WARN_ON() triggers if either of the 2 lsb are set. More
> specifically, its coming from 'static_key_set_entries()' from the
> following patch, which was recently added to linux-next:
>
> https://lkml.org/lkml/2017/2/3/558
>
> So this was not seen on mips, but I included all arches in this patch
> that I though might be affected.
>
>> If so, I think a better approach might be to set properties on the
>> __jump_table section to force the proper alignment, or do something in
>> the linker script.
>>
>
> So in include/asm-generic/vmlinux.lds.h, we are already setting
> 'ALIGN(8)', but that does not appear to be sufficient for POWER...

Yes, I just looked at that.  The ALIGN(8), combined with the fact that 
on MIPS, WORD_INSN will always be of size 4 or 8 (32-bit vs. 64-bit 
kernel), seems to make any additional .balign completely unnecessary.


Really, I think you need to figure out why on powerpc the alignments are 
getting screwed up.  The struct jump_entry entries are on a stride of 12 
(for a 32-bit kernel) or 24 (for a 64-bit kernel), any alignment padding 
added by .balign 4 will surely screw that up.

This patch seems like it is papering over a bigger issue that is not 
understood.

I think a proper fix would be to fix whatever is causing the powerpc 
JUMP_ENTRY_TYPE to misbehave.


>
> Also, I checked the size of the vmlinux generated after this change on
> all 4 arches, and it was rather minimal. I think POWER increased the
> most, but the other arches increased by only a few bytes.

For me the size is not the important issue, it is the alignment of the 
struct jump_entry entries in the table.  I don't understand how your 
patch helps, and I cannot Acked-by unless I understand what is being 
done and can see that it is both correct and necessary.

David.

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

[Steven Rostedt]

On Mon, 27 Feb 2017 11:59:50 -0800
David Daney <ddaney@caviumnetworks.com> wrote:

> For me the size is not the important issue, it is the alignment of the 
> struct jump_entry entries in the table.  I don't understand how your 
> patch helps, and I cannot Acked-by unless I understand what is being 
> done and can see that it is both correct and necessary.

You brought up a very good point and I'm glad that I had Jason Cc all
the arch maintainers in one patch.

I think jump_labels may be much more broken than we think, and Jason's
fix doesn't fix anything. We had this same issues with tracepoints.

I'm looking at jump_label_init, and how we iterate over an array of
struct jump_entry's that was put together by the linker. The problem is
that jump_entry is not a power of 2 in size.

struct jump_entry {
	jump_label_t code;
	jump_label_t target;
	jump_label_t key;
};

When putting together arrays of this kind, the linker is in its right
to add padding for alignment, in the middle of the array! It has no
idea that this is an array, and there's nothing stopping the linker
from messing it up.

For those structs that are a power of 2 in size, there's no reason for
the linker to do anything else, and it "just works". There's plenty of
instances in the kernel that depend on this.

I'm thinking that the sort algorithm either hid the problem or fixed it
somehow (I'm guessing it hid the problem).

I hit the same issue with trace event structures. The solution was to
create the array of pointers to each structure, and dereference the
structures from the array.

See commit e4a9ea5ee ("tracing: Replace trace_event struct array with
pointer array")

-- Steve

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

[David Daney]

On 02/27/2017 01:06 PM, Steven Rostedt wrote:
> On Mon, 27 Feb 2017 11:59:50 -0800
> David Daney <ddaney@caviumnetworks.com> wrote:
>
>> For me the size is not the important issue, it is the alignment of the
>> struct jump_entry entries in the table.  I don't understand how your
>> patch helps, and I cannot Acked-by unless I understand what is being
>> done and can see that it is both correct and necessary.
>
> You brought up a very good point and I'm glad that I had Jason Cc all
> the arch maintainers in one patch.
>
> I think jump_labels may be much more broken than we think, and Jason's
> fix doesn't fix anything. We had this same issues with tracepoints.
>
> I'm looking at jump_label_init, and how we iterate over an array of
> struct jump_entry's that was put together by the linker. The problem is
> that jump_entry is not a power of 2 in size.
>

ELF sections may have an ENTSIZE property exactly for arrays.  Since 
each jump_entry will have a unique value they cannot be merged, but we 
can tell the assembler they are an array and get them properly packed. 
Perhaps something like (untested):

    .pushsection __jump_table,  \"awM\", at progbits,24
    FOO
    .popsection


> struct jump_entry {
> 	jump_label_t code;
> 	jump_label_t target;
> 	jump_label_t key;
> };
>
> When putting together arrays of this kind, the linker is in its right
> to add padding for alignment, in the middle of the array! It has no
> idea that this is an array, and there's nothing stopping the linker
> from messing it up.
>
> For those structs that are a power of 2 in size, there's no reason for
> the linker to do anything else, and it "just works". There's plenty of
> instances in the kernel that depend on this.
>
> I'm thinking that the sort algorithm either hid the problem or fixed it
> somehow (I'm guessing it hid the problem).
>
> I hit the same issue with trace event structures. The solution was to
> create the array of pointers to each structure, and dereference the
> structures from the array.
>
> See commit e4a9ea5ee ("tracing: Replace trace_event struct array with
> pointer array")
>
> -- Steve
>

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

[Steven Rostedt]

On Mon, 27 Feb 2017 13:41:13 -0800
David Daney <ddaney@caviumnetworks.com> wrote:

> On 02/27/2017 01:06 PM, Steven Rostedt wrote:
> > On Mon, 27 Feb 2017 11:59:50 -0800
> > David Daney <ddaney@caviumnetworks.com> wrote:
> >  
> >> For me the size is not the important issue, it is the alignment of the
> >> struct jump_entry entries in the table.  I don't understand how your
> >> patch helps, and I cannot Acked-by unless I understand what is being
> >> done and can see that it is both correct and necessary.  
> >
> > You brought up a very good point and I'm glad that I had Jason Cc all
> > the arch maintainers in one patch.
> >
> > I think jump_labels may be much more broken than we think, and Jason's
> > fix doesn't fix anything. We had this same issues with tracepoints.
> >
> > I'm looking at jump_label_init, and how we iterate over an array of
> > struct jump_entry's that was put together by the linker. The problem is
> > that jump_entry is not a power of 2 in size.
> >  
> 
> ELF sections may have an ENTSIZE property exactly for arrays.  Since 
> each jump_entry will have a unique value they cannot be merged, but we 
> can tell the assembler they are an array and get them properly packed. 
> Perhaps something like (untested):
> 
>     .pushsection __jump_table,  \"awM\", at progbits,24
>     FOO
>     .popsection
> 

And the linker will honor this too?

-- Steve

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

[David Daney]

On 02/27/2017 02:09 PM, Steven Rostedt wrote:
> On Mon, 27 Feb 2017 13:41:13 -0800
> David Daney <ddaney@caviumnetworks.com> wrote:
>
>> On 02/27/2017 01:06 PM, Steven Rostedt wrote:
>>> On Mon, 27 Feb 2017 11:59:50 -0800
>>> David Daney <ddaney@caviumnetworks.com> wrote:
>>>
>>>> For me the size is not the important issue, it is the alignment of the
>>>> struct jump_entry entries in the table.  I don't understand how your
>>>> patch helps, and I cannot Acked-by unless I understand what is being
>>>> done and can see that it is both correct and necessary.
>>>
>>> You brought up a very good point and I'm glad that I had Jason Cc all
>>> the arch maintainers in one patch.
>>>
>>> I think jump_labels may be much more broken than we think, and Jason's
>>> fix doesn't fix anything. We had this same issues with tracepoints.
>>>
>>> I'm looking at jump_label_init, and how we iterate over an array of
>>> struct jump_entry's that was put together by the linker. The problem is
>>> that jump_entry is not a power of 2 in size.
>>>
>>
>> ELF sections may have an ENTSIZE property exactly for arrays.  Since
>> each jump_entry will have a unique value they cannot be merged, but we
>> can tell the assembler they are an array and get them properly packed.
>> Perhaps something like (untested):
>>
>>     .pushsection __jump_table,  \"awM\", at progbits,24
>>     FOO
>>     .popsection
>>
>
> And the linker will honor this too?

See attached for mips.  It seems to do the right thing.

I leave it as an exercise to the reader to fix the other architectures.

Consult your own  binutils experts to verify that what I say is true.


David Daney
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-MIPS-jump_lable-Give-__jump_table-elements-an-entsiz.patch
Type: text/x-patch
Size: 2317 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170227/b2d33eff/attachment-0001.bin>

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

[Steven Rostedt]

On Mon, 27 Feb 2017 14:21:21 -0800
David Daney <ddaney@caviumnetworks.com> wrote:

> See attached for mips.  It seems to do the right thing.
> 
> I leave it as an exercise to the reader to fix the other architectures.
> 
> Consult your own  binutils experts to verify that what I say is true.

It may still just be safer to do the pointers instead. That way we
don't need to worry about some strange arch or off by one binutils
messing it up.

-- Steve

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

[David Daney]

On 02/27/2017 02:36 PM, Steven Rostedt wrote:
> On Mon, 27 Feb 2017 14:21:21 -0800
> David Daney <ddaney@caviumnetworks.com> wrote:
>
>> See attached for mips.  It seems to do the right thing.
>>
>> I leave it as an exercise to the reader to fix the other architectures.
>>
>> Consult your own  binutils experts to verify that what I say is true.
>
> It may still just be safer to do the pointers instead. That way we
> don't need to worry about some strange arch or off by one binutils
> messing it up.

Obviously it is your choice, but this is bog standard ELF linking.  In 
theory even the arrays of power-of-2 sized objects should also supply an 
entity size.  Think __ex_table and its ilk.


The benefit of supplying an entsize is that you don't have to change the 
structure of the existing code and risk breaking something in the process.

David Daney

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

[Jason Baron]

On 02/27/2017 05:45 PM, David Daney wrote:
> On 02/27/2017 02:36 PM, Steven Rostedt wrote:
>> On Mon, 27 Feb 2017 14:21:21 -0800
>> David Daney <ddaney@caviumnetworks.com> wrote:
>>
>>> See attached for mips.  It seems to do the right thing.
>>>
>>> I leave it as an exercise to the reader to fix the other architectures.
>>>
>>> Consult your own  binutils experts to verify that what I say is true.
>>
>> It may still just be safer to do the pointers instead. That way we
>> don't need to worry about some strange arch or off by one binutils
>> messing it up.
>
> Obviously it is your choice, but this is bog standard ELF linking.  In
> theory even the arrays of power-of-2 sized objects should also supply an
> entity size.  Think __ex_table and its ilk.
>
>
> The benefit of supplying an entsize is that you don't have to change the
> structure of the existing code and risk breaking something in the process.
>
> David Daney
>
>

Thanks for the suggestion! I would like to see if this resolves the ppc 
issue we had. I'm attaching a powerpc patch based on your suggestion. 
Hopefully, Sachin can try it.

Thanks,

-Jason
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ppc-pack.patch
Type: text/x-patch
Size: 1975 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170227/b285fe75/attachment.bin>

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

[Steven Rostedt]

On Mon, 27 Feb 2017 14:45:37 -0800
David Daney <ddaney@caviumnetworks.com> wrote:

> On 02/27/2017 02:36 PM, Steven Rostedt wrote:
> > On Mon, 27 Feb 2017 14:21:21 -0800
> > David Daney <ddaney@caviumnetworks.com> wrote:
> >  
> >> See attached for mips.  It seems to do the right thing.
> >>
> >> I leave it as an exercise to the reader to fix the other architectures.
> >>
> >> Consult your own  binutils experts to verify that what I say is true.  
> >
> > It may still just be safer to do the pointers instead. That way we
> > don't need to worry about some strange arch or off by one binutils
> > messing it up.  
> 
> Obviously it is your choice, but this is bog standard ELF linking.  In 
> theory even the arrays of power-of-2 sized objects should also supply an 
> entity size.  Think __ex_table and its ilk.
> 
> 
> The benefit of supplying an entsize is that you don't have to change the 
> structure of the existing code and risk breaking something in the process.

I agree that this may be the better answer. The issue tracepoints had
is that they were defined in C with a "section" attribute. I'm not sure
you can pass various section attributes via a gcc section attribute.

-- Steve

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

[David Daney]

On 02/27/2017 02:50 PM, Jason Baron wrote:
>
>
> On 02/27/2017 05:45 PM, David Daney wrote:
>> On 02/27/2017 02:36 PM, Steven Rostedt wrote:
>>> On Mon, 27 Feb 2017 14:21:21 -0800
>>> David Daney <ddaney@caviumnetworks.com> wrote:
>>>
>>>> See attached for mips.  It seems to do the right thing.
>>>>
>>>> I leave it as an exercise to the reader to fix the other architectures.
>>>>
>>>> Consult your own  binutils experts to verify that what I say is true.
>>>
>>> It may still just be safer to do the pointers instead. That way we
>>> don't need to worry about some strange arch or off by one binutils
>>> messing it up.
>>
>> Obviously it is your choice, but this is bog standard ELF linking.  In
>> theory even the arrays of power-of-2 sized objects should also supply an
>> entity size.  Think __ex_table and its ilk.
>>
>>
>> The benefit of supplying an entsize is that you don't have to change the
>> structure of the existing code and risk breaking something in the
>> process.
>>
>> David Daney
>>
>>
>
> Thanks for the suggestion! I would like to see if this resolves the ppc
> issue we had. I'm attaching a powerpc patch based on your suggestion.
> Hopefully, Sachin can try it.
>

If there are problems, you could try something like:


$ find . -name \*\.o | xargs mips64-octeon-linux-gnu-readelf -eW  | grep 
'File:\| __jump_table'
File: ./drivers/firmware/built-in.o
File: ./drivers/built-in.o
   [3249] __jump_table      PROGBITS        0000000000000000 1838c8 
0022c8 18 WAM  0   0  8
File: ./drivers/spi/built-in.o
   [82] __jump_table      PROGBITS        0000000000000000 008cb0 000048 
18 WAM  0   0  8
File: ./drivers/spi/spi-cavium-octeon.o
File: ./drivers/spi/spi-cavium.o
File: ./drivers/spi/spi.o
.
.
.

Look for files where the size of the __jump_table section is not a 
integer multiple of the entsize.



> Thanks,
>
> -Jason

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

[Steven Rostedt]

On Tue, 28 Feb 2017 10:25:46 +0530
Sachin Sant <sachinp@linux.vnet.ibm.com> wrote:

> File: ./net/ipv4/xfrm4_input.o
>   [12] __jump_table      PROGBITS        0000000000000000 000639 000018 18 WAM  0   0  1
> File: ./net/ipv4/udplite.o
> File: ./net/ipv4/xfrm4_output.o
>   [ 9] __jump_table      PROGBITS        0000000000000000 000481 000018 18 WAM  0   0  1

Looks like there's some issues right there.

-- Steve

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

[David Daney]

On 02/28/2017 08:21 AM, Steven Rostedt wrote:
> On Tue, 28 Feb 2017 10:25:46 +0530
> Sachin Sant <sachinp@linux.vnet.ibm.com> wrote:
>
>> File: ./net/ipv4/xfrm4_input.o
>>   [12] __jump_table      PROGBITS        0000000000000000 000639 000018 18 WAM  0   0  1
>> File: ./net/ipv4/udplite.o
>> File: ./net/ipv4/xfrm4_output.o
>>   [ 9] __jump_table      PROGBITS        0000000000000000 000481 000018 18 WAM  0   0  1
>
> Looks like there's some issues right there.

Those look good to me 18/18 = 1 with no remainder.  The odd numbers are 
the offset of the section in the ELF file.

If you look at the stack trace, it seems that it is during module loading.

Are the primitives for generating the tables doing something different 
for the module case?  I am not familiar enough with the powerpc ABIs to 
know.

Try this:

$ perl -n -e 's/\[ /\[/; my @f = split " "; print hex($f[5]) % 0x18 if 
$#f > 5; print $_' <~/jump_table.log


There are no entries with size that is not a multiple of 0x18.

I think my patch to add the ENTSIZE is not doing anything here.

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.




>
> -- Steve
>

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

[Jason Baron]

On 02/28/2017 01:16 PM, David Daney wrote:
> On 02/28/2017 08:21 AM, Steven Rostedt wrote:
>> On Tue, 28 Feb 2017 10:25:46 +0530
>> Sachin Sant <sachinp@linux.vnet.ibm.com> wrote:
>>
>>> File: ./net/ipv4/xfrm4_input.o
>>>   [12] __jump_table      PROGBITS        0000000000000000 000639
>>> 000018 18 WAM  0   0  1
>>> File: ./net/ipv4/udplite.o
>>> File: ./net/ipv4/xfrm4_output.o
>>>   [ 9] __jump_table      PROGBITS        0000000000000000 000481
>>> 000018 18 WAM  0   0  1
>>
>> Looks like there's some issues right there.
>
> Those look good to me 18/18 = 1 with no remainder.  The odd numbers are
> the offset of the section in the ELF file.
>
> If you look at the stack trace, it seems that it is during module loading.
>
> Are the primitives for generating the tables doing something different
> for the module case?  I am not familiar enough with the powerpc ABIs to
> know.
>
> Try this:
>
> $ perl -n -e 's/\[ /\[/; my @f = split " "; print hex($f[5]) % 0x18 if
> $#f > 5; print $_' <~/jump_table.log
>
>
> There are no entries with size that is not a multiple of 0x18.
>
> I think my patch to add the ENTSIZE is not doing anything here.
>
> 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

So its not the size but rather the start offset '03fd77', that is the 
problem here. That is what the WARN_ON triggers on, that the start of 
the table is not 4-byte aligned.

Using a ppc cross-compiler and the ENTSIZE patch that line does not 
change, however if I use the initial patch posted in this thread, the 
start does align to 4-bytes and thus the warning goes away, as Sachin 
verified. In fact, without the patch I found several modules that don't 
start at the proper alignment, however with the patch that started this 
thread they were all properly aligned.

In terms of the '.balign' causing holes, we originally added the 
'_ASM_ALIGN' to x86 for precisely this reason. See commit:
ef64789 jump label: Add _ASM_ALIGN for x86 and x86_64 and discussion.

In addition, we have a lot of runtime with the .balign in the tree and 
I'm not aware of any holes in the table. I think the code would blow up 
pretty badly if there were.

A number of arches were already using the '.balign', and the patch I 
proposed simply added it to remaining ones, now that we added a 
WARN_ON() to catch this condition.

Thanks,

-Jason

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

[David Daney]

On 02/28/2017 10:39 AM, Jason Baron wrote:
>
>
> On 02/28/2017 01:16 PM, David Daney wrote:
>> On 02/28/2017 08:21 AM, Steven Rostedt wrote:
>>> On Tue, 28 Feb 2017 10:25:46 +0530
>>> Sachin Sant <sachinp@linux.vnet.ibm.com> wrote:
>>>
>>>> File: ./net/ipv4/xfrm4_input.o
>>>>   [12] __jump_table      PROGBITS        0000000000000000 000639
>>>> 000018 18 WAM  0   0  1
>>>> File: ./net/ipv4/udplite.o
>>>> File: ./net/ipv4/xfrm4_output.o
>>>>   [ 9] __jump_table      PROGBITS        0000000000000000 000481
>>>> 000018 18 WAM  0   0  1
>>>
>>> Looks like there's some issues right there.
>>
>> Those look good to me 18/18 = 1 with no remainder.  The odd numbers are
>> the offset of the section in the ELF file.
>>
>> If you look at the stack trace, it seems that it is during module
>> loading.
>>
>> Are the primitives for generating the tables doing something different
>> for the module case?  I am not familiar enough with the powerpc ABIs to
>> know.
>>
>> Try this:
>>
>> $ perl -n -e 's/\[ /\[/; my @f = split " "; print hex($f[5]) % 0x18 if
>> $#f > 5; print $_' <~/jump_table.log
>>
>>
>> There are no entries with size that is not a multiple of 0x18.
>>
>> I think my patch to add the ENTSIZE is not doing anything here.
>>
>> 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


>
> So its not the size but rather the start offset '03fd77', that is the
> problem here. That is what the WARN_ON triggers on, that the start of
> the table is not 4-byte aligned.
>
> Using a ppc cross-compiler and the ENTSIZE patch that line does not
> change, however if I use the initial patch posted in this thread, the
> start does align to 4-bytes and thus the warning goes away, as Sachin
> verified. In fact, without the patch I found several modules that don't
> start at the proper alignment, however with the patch that started this
> thread they were all properly aligned.
>
> In terms of the '.balign' causing holes, we originally added the
> '_ASM_ALIGN' to x86 for precisely this reason. See commit:
> ef64789 jump label: Add _ASM_ALIGN for x86 and x86_64 and discussion.
>
> In addition, we have a lot of runtime with the .balign in the tree and
> I'm not aware of any holes in the table. I think the code would blow up
> pretty badly if there were.
>
> A number of arches were already using the '.balign', and the patch I
> proposed simply added it to remaining ones, now that we added a
> WARN_ON() to catch this condition.
>
> Thanks,
>
> -Jason
>
>
>
>

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

[David Daney]

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.



>
>>
>> So its not the size but rather the start offset '03fd77', that is the
>> problem here. That is what the WARN_ON triggers on, that the start of
>> the table is not 4-byte aligned.
>>
>> Using a ppc cross-compiler and the ENTSIZE patch that line does not
>> change, however if I use the initial patch posted in this thread, the
>> start does align to 4-bytes and thus the warning goes away, as Sachin
>> verified. In fact, without the patch I found several modules that don't
>> start at the proper alignment, however with the patch that started this
>> thread they were all properly aligned.
>>
>> In terms of the '.balign' causing holes, we originally added the
>> '_ASM_ALIGN' to x86 for precisely this reason. See commit:
>> ef64789 jump label: Add _ASM_ALIGN for x86 and x86_64 and discussion.
>>
>> In addition, we have a lot of runtime with the .balign in the tree and
>> I'm not aware of any holes in the table. I think the code would blow up
>> pretty badly if there were.
>>
>> A number of arches were already using the '.balign', and the patch I
>> proposed simply added it to remaining ones, now that we added a
>> WARN_ON() to catch this condition.
>>
>> Thanks,
>>
>> -Jason
>>
>>
>>
>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jason Baron]

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

Thanks,

-Jason


> 
>>
>>>
>>> So its not the size but rather the start offset '03fd77', that is the
>>> problem here. That is what the WARN_ON triggers on, that the start of
>>> the table is not 4-byte aligned.
>>>
>>> Using a ppc cross-compiler and the ENTSIZE patch that line does not
>>> change, however if I use the initial patch posted in this thread, the
>>> start does align to 4-bytes and thus the warning goes away, as Sachin
>>> verified. In fact, without the patch I found several modules that don't
>>> start at the proper alignment, however with the patch that started this
>>> thread they were all properly aligned.
>>>
>>> In terms of the '.balign' causing holes, we originally added the
>>> '_ASM_ALIGN' to x86 for precisely this reason. See commit:
>>> ef64789 jump label: Add _ASM_ALIGN for x86 and x86_64 and discussion.
>>>
>>> In addition, we have a lot of runtime with the .balign in the tree and
>>> I'm not aware of any holes in the table. I think the code would blow up
>>> pretty badly if there were.
>>>
>>> A number of arches were already using the '.balign', and the patch I
>>> proposed simply added it to remaining ones, now that we added a
>>> WARN_ON() to catch this condition.
>>>
>>> Thanks,
>>>
>>> -Jason
>>>
>>>
>>>
>>>
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

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

[David Daney]

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.





-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-module-set-__jump_table-alignment-to-8.patch
Type: text/x-patch
Size: 916 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170228/f4a8f907/attachment.bin>

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

[Jason Baron]

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

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

[Michael Ellerman]

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

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

[David Daney]

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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

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

[Jason Baron]

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

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

[PATCH] MIPS: jump_lable: Give __jump_table elements an entsize.

[kbuild test robot]

Hi David,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.11-rc1 next-20170303]
[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/David-Daney/MIPS-jump_lable-Give-__jump_table-elements-an-entsize/20170228-231935
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/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=mips 

All error/warnings (new ones prefixed by >>):

   {standard input}: Assembler messages:
>> {standard input}:968: Warning: entity size for SHF_MERGE not specified
>> {standard input}:968: Error: junk at end of line, first unrecognized character is `@'
>> {standard input}:968: Warning: ignoring changed section attributes for __jump_table
   {standard input}:987: Warning: entity size for SHF_MERGE not specified
   {standard input}:987: Error: junk at end of line, first unrecognized character is `@'
   {standard input}:987: Warning: ignoring changed section attributes for __jump_table
   {standard input}:1092: Warning: entity size for SHF_MERGE not specified
   {standard input}:1092: Error: junk at end of line, first unrecognized character is `@'
   {standard input}:1092: Warning: ignoring changed section attributes for __jump_table
   {standard input}:1113: Warning: entity size for SHF_MERGE not specified
   {standard input}:1113: Error: junk at end of line, first unrecognized character is `@'
   {standard input}:1113: Warning: ignoring changed section attributes for __jump_table

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 45689 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170306/b764249a/attachment-0001.gz>