Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1588868 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] jump_label: align jump_entry table to at least 4-bytes Jason Baron <jbaron@akamai.com> - 2017-02-27 20:00 +0100
    Re: [PATCH] jump_label: align jump_entry table to at least 4-bytes Jason Baron <jbaron@akamai.com> - 2017-02-27 20:30 +0100
      Re: [PATCH] jump_label: align jump_entry table to at least 4-bytes Steven Rostedt <rostedt@goodmis.org> - 2017-02-27 23:20 +0100
        Re: [PATCH] jump_label: align jump_entry table to at least 4-bytes Steven Rostedt <rostedt@goodmis.org> - 2017-02-27 23:40 +0100
          Re: [PATCH] jump_label: align jump_entry table to at least 4-bytes Jason Baron <jbaron@akamai.com> - 2017-02-28 00:00 +0100
            Re: [PATCH] jump_label: align jump_entry table to at least 4-bytes Steven Rostedt <rostedt@goodmis.org> - 2017-02-28 17:50 +0100
              Re: [PATCH] jump_label: align jump_entry table to at least 4-bytes Jason Baron <jbaron@akamai.com> - 2017-02-28 19:50 +0100
                Re: [PATCH] jump_label: align jump_entry table to at least 4-bytes Jason Baron <jbaron@akamai.com> - 2017-02-28 20:40 +0100
                  Re: [PATCH] jump_label: align jump_entry table to at least 4-bytes Jason Baron <jbaron@akamai.com> - 2017-03-01 00:00 +0100
                    Re: [PATCH] jump_label: align jump_entry table to at least 4-bytes Michael Ellerman <mpe@ellerman.id.au> - 2017-03-01 07:50 +0100
                      Re: [PATCH] jump_label: align jump_entry table to at least 4-bytes Jason Baron <jbaron@akamai.com> - 2017-03-01 22:00 +0100
                    Re: [PATCH] jump_label: align jump_entry table to at least 4-bytes Sachin Sant <sachinp@linux.vnet.ibm.com> - 2017-03-01 12:10 +0100
          Re: [PATCH] jump_label: align jump_entry table to at least 4-bytes Steven Rostedt <rostedt@goodmis.org> - 2017-02-28 00:20 +0100
      Re: [PATCH] jump_label: align jump_entry table to at least 4-bytes Steven Rostedt <rostedt@goodmis.org> - 2017-02-28 03:40 +0100

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

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

[toc] | [next] | [standalone]

#1588886

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

[toc] | [prev] | [next] | [standalone]

#1588979

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\",@progbits,24
>     FOO
>     .popsection
> 

And the linker will honor this too?

-- Steve

[toc] | [prev] | [next] | [standalone]

#1588999

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

[toc] | [prev] | [next] | [standalone]

#1589019

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1589623

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

[toc] | [prev] | [next] | [standalone]

#1589693

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

[toc] | [prev] | [next] | [standalone]

#1589724

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

[toc] | [prev] | [next] | [standalone]

#1589854

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

[toc] | [prev] | [next] | [standalone]

#1590035

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

[toc] | [prev] | [next] | [standalone]

#1590623

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

[toc] | [prev] | [next] | [standalone]

#1590251

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

I tested the patch on 2 different systems where I ran into this problem. In both cases
the system boots without any warning. A quick module load/unload test also worked
correctly.

Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com>

Thanks
-Sachin

[toc] | [prev] | [next] | [standalone]

#1589040

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

[toc] | [prev] | [next] | [standalone]

#1589117

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web