[PATCH v3] ia64: fix module loading for gcc-5.4

[PATCH v3] ia64: fix module loading for gcc-5.4

[Sergei Trofimovich]

Starting from gcc-5.4+ gcc generates MLX
instructions in more cases to refer local
symbols:
    https://gcc.gnu.org/PR60465

That caused ia64 module loader to choke
on such instructions:
    fuse: invalid slot number 1 for IMM64

Linux kernel used to handle only case where
relocation pointed to slot=2 instruction in
the bundle. That limitation was fixed in linux by
commit 9c184a073bfd ("[IA64] Fix 2.6 kernel for the new ia64 assembler")
See http://sources.redhat.com/bugzilla/show_bug.cgi?id\x1433

This change lifts the slot=2 restriction from
linux kernel module loader.

Tested on 'fuse' and 'btrfs' kernel modules.

Cc: Markus Elfring <elfring@users.sourceforge.net>
Cc: H. J. Lu <hjl.tools@gmail.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: linux-ia64@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Andrew Morton <akpm@linux-foundation.org>
Bug: https://bugs.gentoo.org/601014
Tested-by: Émeric MASCHINO <emeric.maschino@gmail.com>
Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
---
Change since v1: added 'Tested-by'
Change since v2: checkpatched, fixed typos by found by Markus Elfring

 arch/ia64/kernel/module.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/ia64/kernel/module.c b/arch/ia64/kernel/module.c
index 6ab0ae7d6535..d1d945c6bd05 100644
--- a/arch/ia64/kernel/module.c
+++ b/arch/ia64/kernel/module.c
@@ -153,7 +153,7 @@ slot (const struct insn *insn)
 static int
 apply_imm64 (struct module *mod, struct insn *insn, uint64_t val)
 {
-	if (slot(insn) != 2) {
+	if (slot(insn) != 1 && slot(insn) != 2) {
 		printk(KERN_ERR "%s: invalid slot number %d for IMM64\n",
 		       mod->name, slot(insn));
 		return 0;
@@ -165,7 +165,7 @@ apply_imm64 (struct module *mod, struct insn *insn, uint64_t val)
 static int
 apply_imm60 (struct module *mod, struct insn *insn, uint64_t val)
 {
-	if (slot(insn) != 2) {
+	if (slot(insn) != 1 && slot(insn) != 2) {
 		printk(KERN_ERR "%s: invalid slot number %d for IMM60\n",
 		       mod->name, slot(insn));
 		return 0;
-- 
2.12.0

[PATCH v3] ia64: fix module loading for gcc-5.4

[Sergei Trofimovich]

Starting from gcc-5.4+ gcc generates MLX
instructions in more cases to refer local
symbols:
    https://gcc.gnu.org/PR60465

That caused ia64 module loader to choke
on such instructions:
    fuse: invalid slot number 1 for IMM64

Linux kernel used to handle only case where
relocation pointed to slot=2 instruction in
the bundle. That limitation was fixed in linux by
commit 9c184a073bfd ("[IA64] Fix 2.6 kernel for the new ia64 assembler")
See http://sources.redhat.com/bugzilla/show_bug.cgi?id=1433

This change lifts the slot=2 restriction from
linux kernel module loader.

Tested on 'fuse' and 'btrfs' kernel modules.

Cc: Markus Elfring <elfring@users.sourceforge.net>
Cc: H. J. Lu <hjl.tools@gmail.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: linux-ia64@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Andrew Morton <akpm@linux-foundation.org>
Bug: https://bugs.gentoo.org/601014
Tested-by: Émeric MASCHINO <emeric.maschino@gmail.com>
Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
---
Change since v1: added 'Tested-by'
Change since v2: checkpatched, fixed typos by found by Markus Elfring

 arch/ia64/kernel/module.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/ia64/kernel/module.c b/arch/ia64/kernel/module.c
index 6ab0ae7d6535..d1d945c6bd05 100644
--- a/arch/ia64/kernel/module.c
+++ b/arch/ia64/kernel/module.c
@@ -153,7 +153,7 @@ slot (const struct insn *insn)
 static int
 apply_imm64 (struct module *mod, struct insn *insn, uint64_t val)
 {
-	if (slot(insn) != 2) {
+	if (slot(insn) != 1 && slot(insn) != 2) {
 		printk(KERN_ERR "%s: invalid slot number %d for IMM64\n",
 		       mod->name, slot(insn));
 		return 0;
@@ -165,7 +165,7 @@ apply_imm64 (struct module *mod, struct insn *insn, uint64_t val)
 static int
 apply_imm60 (struct module *mod, struct insn *insn, uint64_t val)
 {
-	if (slot(insn) != 2) {
+	if (slot(insn) != 1 && slot(insn) != 2) {
 		printk(KERN_ERR "%s: invalid slot number %d for IMM60\n",
 		       mod->name, slot(insn));
 		return 0;
-- 
2.12.0

Re: [PATCH v3] ia64: fix module loading for gcc-5.4

[SF Markus Elfring]

> Starting from gcc-5.4+ gcc generates MLX

How do you think about to omit the plus character?


> instructions in more cases to refer local
> symbols:

I wonder about your choice of a line length limit here.


> That caused ia64 module loader to choke
> on such instructions:
>     fuse: invalid slot number 1 for IMM64

Why does it matter to check such a value?


> … That limitation was fixed in linux by

Would it be nicer to write “in corresponding source code by the”?


> Change since v2: checkpatched, fixed typos by found by Markus Elfring

Does this version information contain an unwanted word repetition?
How does it fit to the identifier “v3” in the commit subject?

Regards,
Markus

Re: [PATCH v3] ia64: fix module loading for gcc-5.4

[SF Markus Elfring]

> Starting from gcc-5.4+ gcc generates MLX

How do you think about to omit the plus character?


> instructions in more cases to refer local
> symbols:

I wonder about your choice of a line length limit here.


> That caused ia64 module loader to choke
> on such instructions:
>     fuse: invalid slot number 1 for IMM64

Why does it matter to check such a value?


> … That limitation was fixed in linux by

Would it be nicer to write “in corresponding source code by the”?


> Change since v2: checkpatched, fixed typos by found by Markus Elfring

Does this version information contain an unwanted word repetition?
How does it fit to the identifier “v3” in the commit subject?

Regards,
Markus

Re: [PATCH v3] ia64: fix module loading for gcc-5.4

[Sergei Trofimovich]

[-- Attachment #1: Type: text/plain, Size: 418 bytes --]

On Sun, 9 Apr 2017 10:27:52 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> > That caused ia64 module loader to choke
> > on such instructions:
> >     fuse: invalid slot number 1 for IMM64  
> 
> Why does it matter to check such a value?

I'm not sure I follow the question. Is your question about
linux kernel relocation code handler, gcc or ia64 instruction
format?

-- 

  Sergei

[-- Attachment #2: =?unknown-8bit?b?0KbQuNGE0YDQvtCy0LDRjyDQv9C+0LTQv9C40YHRjA==?= OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v3] ia64: fix module loading for gcc-5.4

[Sergei Trofimovich]

[-- Attachment #1: Type: text/plain, Size: 418 bytes --]

On Sun, 9 Apr 2017 10:27:52 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> > That caused ia64 module loader to choke
> > on such instructions:
> >     fuse: invalid slot number 1 for IMM64  
> 
> Why does it matter to check such a value?

I'm not sure I follow the question. Is your question about
linux kernel relocation code handler, gcc or ia64 instruction
format?

-- 

  Sergei

[-- Attachment #2: Цифровая подпись OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: ia64: fix module loading for gcc-5.4

[SF Markus Elfring]

>>> That caused ia64 module loader to choke
>>> on such instructions:
>>>     fuse: invalid slot number 1 for IMM64  
>>
>> Why does it matter to check such a value?
> 
> I'm not sure I follow the question. Is your question about
> linux kernel relocation code handler, gcc or ia64 instruction format?

I am just curious if this source code could also work without
the mentioned check.
Would it make sense to check more than two values there?

Regards,
Markus

Re: ia64: fix module loading for gcc-5.4

[SF Markus Elfring]

>>> That caused ia64 module loader to choke
>>> on such instructions:
>>>     fuse: invalid slot number 1 for IMM64  
>>
>> Why does it matter to check such a value?
> 
> I'm not sure I follow the question. Is your question about
> linux kernel relocation code handler, gcc or ia64 instruction format?

I am just curious if this source code could also work without
the mentioned check.
Would it make sense to check more than two values there?

Regards,
Markus

Re: ia64: fix module loading for gcc-5.4

[Sergei Trofimovich]

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

On Sun, 9 Apr 2017 11:02:43 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> >>> That caused ia64 module loader to choke
> >>> on such instructions:
> >>>     fuse: invalid slot number 1 for IMM64    
> >>
> >> Why does it matter to check such a value?  
> > 
> > I'm not sure I follow the question. Is your question about
> > linux kernel relocation code handler, gcc or ia64 instruction format?  
> 
> I am just curious if this source code could also work without
> the mentioned check.

It should work for valid code, yes. The flip side of check removal
is to miss malformed relocation (say, when instruction "address" is
wrong due to obscure toolchain bug). In this case apply_imm64()
would silently corrupt unrelated memory instead of crashing kernel.

> Would it make sense to check more than two values there?

AFAIU ia64 does not allow encoding imm64/imm60 instructions
spanning slot=0 at all.

ia64_patch_imm64() can handle only imm64 bundles that span
only both slot 1 and slot 2 at the same time. It can accept
either slot=1 "address" or slot=2 "address". Anything else would
be malformed.

-- 

  Sergei

[-- Attachment #2: =?unknown-8bit?b?0KbQuNGE0YDQvtCy0LDRjyDQv9C+0LTQv9C40YHRjA==?= OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: ia64: fix module loading for gcc-5.4

[Sergei Trofimovich]

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

On Sun, 9 Apr 2017 11:02:43 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> >>> That caused ia64 module loader to choke
> >>> on such instructions:
> >>>     fuse: invalid slot number 1 for IMM64    
> >>
> >> Why does it matter to check such a value?  
> > 
> > I'm not sure I follow the question. Is your question about
> > linux kernel relocation code handler, gcc or ia64 instruction format?  
> 
> I am just curious if this source code could also work without
> the mentioned check.

It should work for valid code, yes. The flip side of check removal
is to miss malformed relocation (say, when instruction "address" is
wrong due to obscure toolchain bug). In this case apply_imm64()
would silently corrupt unrelated memory instead of crashing kernel.

> Would it make sense to check more than two values there?

AFAIU ia64 does not allow encoding imm64/imm60 instructions
spanning slot=0 at all.

ia64_patch_imm64() can handle only imm64 bundles that span
only both slot 1 and slot 2 at the same time. It can accept
either slot=1 "address" or slot=2 "address". Anything else would
be malformed.

-- 

  Sergei

[-- Attachment #2: Цифровая подпись OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v3] ia64: fix module loading for gcc-5.4+

[SF Markus Elfring]

> +++ b/arch/ia64/kernel/module.c
> @@ -153,7 +153,7 @@ slot (const struct insn *insn)
>  static int
>  apply_imm64 (struct module *mod, struct insn *insn, uint64_t val)
>  {

I have got another idea (after your clarification) for the suggested change.


> -	if (slot(insn) != 2) {
> +	if (slot(insn) != 1 && slot(insn) != 2) {

+	int const s = slot(insn);
+	if (s < 1 || s > 2) {

Do run time characteristics matter for such a condition check here?


>  		printk(KERN_ERR "%s: invalid slot number %d for IMM64\n",

-  		       mod->name, slot(insn));
+  		       mod->name, s);


>  		return 0;


How do you think about my update suggestion for this function implementation?

Regards,
Markus

Re: [PATCH v3] ia64: fix module loading for gcc-5.4+

[SF Markus Elfring]

> +++ b/arch/ia64/kernel/module.c
> @@ -153,7 +153,7 @@ slot (const struct insn *insn)
>  static int
>  apply_imm64 (struct module *mod, struct insn *insn, uint64_t val)
>  {

I have got another idea (after your clarification) for the suggested change.


> -	if (slot(insn) != 2) {
> +	if (slot(insn) != 1 && slot(insn) != 2) {

+	int const s = slot(insn);
+	if (s < 1 || s > 2) {

Do run time characteristics matter for such a condition check here?


>  		printk(KERN_ERR "%s: invalid slot number %d for IMM64\n",

-  		       mod->name, slot(insn));
+  		       mod->name, s);


>  		return 0;


How do you think about my update suggestion for this function implementation?

Regards,
Markus

Re: [PATCH v3] ia64: fix module loading for gcc-5.4+

[Sergei Trofimovich]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

On Mon, 10 Apr 2017 19:23:28 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> > -	if (slot(insn) != 2) {
> > +	if (slot(insn) != 1 && slot(insn) != 2) {  
> 
> +	int const s = slot(insn);
> +	if (s < 1 || s > 2) {
> 
> Do run time characteristics matter for such a condition check here?

It's done once at kernel module load time. My guess would be
"not critical at all".

slot() is a pure arithmetic static inline function. You can compare
assembly output before and after your change.

You can measure the difference yourself using 'ski' emulator.
That's for example how I debugged and tested the patch:
     http://trofi.github.io/posts/199-ia64-machine-emulation.html

-- 

  Sergei

[-- Attachment #2: =?unknown-8bit?b?0KbQuNGE0YDQvtCy0LDRjyDQv9C+0LTQv9C40YHRjA==?= OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v3] ia64: fix module loading for gcc-5.4+

[Sergei Trofimovich]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

On Mon, 10 Apr 2017 19:23:28 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> > -	if (slot(insn) != 2) {
> > +	if (slot(insn) != 1 && slot(insn) != 2) {  
> 
> +	int const s = slot(insn);
> +	if (s < 1 || s > 2) {
> 
> Do run time characteristics matter for such a condition check here?

It's done once at kernel module load time. My guess would be
"not critical at all".

slot() is a pure arithmetic static inline function. You can compare
assembly output before and after your change.

You can measure the difference yourself using 'ski' emulator.
That's for example how I debugged and tested the patch:
     http://trofi.github.io/posts/199-ia64-machine-emulation.html

-- 

  Sergei

[-- Attachment #2: Цифровая подпись OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v3] ia64: fix module loading for gcc-5.4

[Sergei Trofimovich]

[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]

On Sat,  8 Apr 2017 20:53:18 +0100
Sergei Trofimovich <slyfox@gentoo.org> wrote:

> Starting from gcc-5.4+ gcc generates MLX
> instructions in more cases to refer local
> symbols:
>     https://gcc.gnu.org/PR60465
> 
> That caused ia64 module loader to choke
> on such instructions:
>     fuse: invalid slot number 1 for IMM64
> 
> Linux kernel used to handle only case where
> relocation pointed to slot=2 instruction in
> the bundle. That limitation was fixed in linux by
> commit 9c184a073bfd ("[IA64] Fix 2.6 kernel for the new ia64 assembler")
> See http://sources.redhat.com/bugzilla/show_bug.cgi?id=1433
> 
> This change lifts the slot=2 restriction from
> linux kernel module loader.
> 
> Tested on 'fuse' and 'btrfs' kernel modules.
> 
> Cc: Markus Elfring <elfring@users.sourceforge.net>
> Cc: H. J. Lu <hjl.tools@gmail.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: linux-ia64@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Bug: https://bugs.gentoo.org/601014
> Tested-by: Émeric MASCHINO <emeric.maschino@gmail.com>
> Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
> ---
> Change since v1: added 'Tested-by'
> Change since v2: checkpatched, fixed typos by found by Markus Elfring

Ping :)

-- 

  Sergei

[-- Attachment #2: =?unknown-8bit?b?0KbQuNGE0YDQvtCy0LDRjyDQv9C+0LTQv9C40YHRjA==?= OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v3] ia64: fix module loading for gcc-5.4

[Sergei Trofimovich]

[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]

On Sat,  8 Apr 2017 20:53:18 +0100
Sergei Trofimovich <slyfox@gentoo.org> wrote:

> Starting from gcc-5.4+ gcc generates MLX
> instructions in more cases to refer local
> symbols:
>     https://gcc.gnu.org/PR60465
> 
> That caused ia64 module loader to choke
> on such instructions:
>     fuse: invalid slot number 1 for IMM64
> 
> Linux kernel used to handle only case where
> relocation pointed to slot=2 instruction in
> the bundle. That limitation was fixed in linux by
> commit 9c184a073bfd ("[IA64] Fix 2.6 kernel for the new ia64 assembler")
> See http://sources.redhat.com/bugzilla/show_bug.cgi?id=1433
> 
> This change lifts the slot=2 restriction from
> linux kernel module loader.
> 
> Tested on 'fuse' and 'btrfs' kernel modules.
> 
> Cc: Markus Elfring <elfring@users.sourceforge.net>
> Cc: H. J. Lu <hjl.tools@gmail.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: linux-ia64@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Bug: https://bugs.gentoo.org/601014
> Tested-by: Émeric MASCHINO <emeric.maschino@gmail.com>
> Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
> ---
> Change since v1: added 'Tested-by'
> Change since v2: checkpatched, fixed typos by found by Markus Elfring

Ping :)

-- 

  Sergei

[-- Attachment #2: Цифровая подпись OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]