Skip to contentSkip to conversation

MIPS: Fix MAX_REG_OFFSET and remove zero-length struct member

PATCH In review Thorsten Blum 4 participants 14 messages Apr 17, 2025 – Apr 27, 2025

Subsystems (2)
MIPS ptrace support
lore
Share & tools
lore

Thorsten Blum

author
#0 lore raw

PATCH v2 1/1 MIPS: Fix MAX_REG_OFFSET and remove zero-length struct member

Remove the unnecessary zero-length struct member '__last' and fix MAX_REG_OFFSET to point to the last register in 'pt_regs'.

Fixes 40e084a506eba ("MIPS: Add uprobes support.")
Signed-off-by Thorsten Blum <[email protected]>

---
Compile-tested only.

Changes in v2:
- Fix MAX_REG_OFFSET as suggested by Maciej (thanks!)
- Link to v1: https://lore.kernel.org/lkml/[email protected]/

---
 arch/mips/include/asm/ptrace.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
arch/mips/include/asm/ptrace.h +5 -2
index 85fa9962266a..23acad11ac8e 100644
@@ -48,7 +48,6 @@ struct pt_regs {
 	unsigned long long mpl[6];        /* MTM{0-5} */
 	unsigned long long mtp[6];        /* MTP{0-5} */
 #endif
-	unsigned long __last[0];
 } __aligned(8);
 
 static inline unsigned long kernel_stack_pointer(struct pt_regs *regs)

@@ -65,7 +64,11 @@ static inline void instruction_pointer_set(struct pt_regs *regs,
 
 /* Query offset/name of register from its name/offset */
 extern int regs_query_register_offset(const char *name);
-#define MAX_REG_OFFSET (offsetof(struct pt_regs, __last))
+#ifdef CONFIG_CPU_CAVIUM_OCTEON
+#define MAX_REG_OFFSET offsetof(struct pt_regs, mtp[5])
+#else
+#define MAX_REG_OFFSET offsetof(struct pt_regs, cp0_epc)
+#endif
 
 /**
  * regs_get_register() - get register value from its offset
Signature · 1 line
2.49.0
maintainer
#1 lore raw
Quoted 4 lines · Thorsten Blum

Remove the unnecessary zero-length struct member '__last' and fix MAX_REG_OFFSET to point to the last register in 'pt_regs'.

Fixes 40e084a506eba ("MIPS: Add uprobes support.")

what does it fix ?

Thomas.

Signature · 2 lines
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]
author
#2 lore raw
Quoted 7 lines · Thomas Bogendoerfer
Quoted 4 lines · Thorsten Blum

Remove the unnecessary zero-length struct member '__last' and fix MAX_REG_OFFSET to point to the last register in 'pt_regs'.

Fixes 40e084a506eba ("MIPS: Add uprobes support.")

what does it fix ?

The value of MAX_REG_OFFSET and thus how regs_get_register() behaves.

From my understanding, MAX_REG_OFFSET points to the marker '__last[0]' instead of the actual last register in 'pt_regs', which could allow regs_get_register() to return an invalid offset.

Let me know if I'm missing anything.

Thanks,
Thorsten

#3 lore raw
Quoted 5 lines · Thorsten Blum
Quoted 5 lines
Quoted 4 lines

Remove the unnecessary zero-length struct member '__last' and fix MAX_REG_OFFSET to point to the last register in 'pt_regs'.

Fixes 40e084a506eba ("MIPS: Add uprobes support.")
Quoted 7 lines
Quoted 1 line

what does it fix ?

The value of MAX_REG_OFFSET and thus how regs_get_register() behaves.

From my understanding, MAX_REG_OFFSET points to the marker '__last[0]' instead of the actual last register in 'pt_regs', which could allow regs_get_register() to return an invalid offset.

Or actually it permits an out-of-range access beyond the end of `struct
pt_regs': if you call regs_get_register(pt_regs, MAX_REG_OFFSET)', it'll read memory beyond pt_regs' rather than returning 0 right away. It may not happen in reality (I haven't checked), but it's a QoI issue we should address IMO. Other platforms that I've checked (riscv, x86) get it right.

Though the fix is incorrect for CPU_CAVIUM_OCTEON, because it doesn't
allow one to access the second half of the last register, and I find it exceedingly complex anyway. Just:

#define MAX_REG_OFFSET \
(offsetof(struct pt_regs, __last) - sizeof(unsigned long))

will do (as regs_get_register' operates on unsigned long' quantities).

Using `sizeof(struct pt_regs)' is problematic, as there might be padding
at the end of the structure, depending on the configuration (which is also surely why Ralf chose to add this extra `__last' member instead), and we don't want let one access that padding area either.

Maciej

author
#4 lore raw
Quoted 20 lines · Maciej W. Rozycki
Quoted 5 lines · Thorsten Blum
Quoted 5 lines
Quoted 4 lines

Remove the unnecessary zero-length struct member '__last' and fix MAX_REG_OFFSET to point to the last register in 'pt_regs'.

Fixes 40e084a506eba ("MIPS: Add uprobes support.")
Quoted 7 lines
Quoted 1 line

what does it fix ?

The value of MAX_REG_OFFSET and thus how regs_get_register() behaves.

From my understanding, MAX_REG_OFFSET points to the marker '__last[0]' instead of the actual last register in 'pt_regs', which could allow regs_get_register() to return an invalid offset.

Or actually it permits an out-of-range access beyond the end of struct pt_regs': if you call regs_get_register(pt_regs, MAX_REG_OFFSET)', it'll read memory beyond `pt_regs' rather than returning 0 right away. It may not happen in reality (I haven't checked), but it's a QoI issue we should address IMO. Other platforms that I've checked (riscv, x86) get it right.

Quoted 8 lines

Though the fix is incorrect for CPU_CAVIUM_OCTEON, because it doesn't allow one to access the second half of the last register, and I find it exceedingly complex anyway. Just:

#define MAX_REG_OFFSET \
(offsetof(struct pt_regs, __last) - sizeof(unsigned long))

will do (as regs_get_register' operates on unsigned long' quantities).

Does regs_get_register() even work for CPU_CAVIUM_OCTEON when accessing the last two registers because they're both ULL, not UL? (independent of my patch)

Thorsten

#5 lore raw
Quoted 4 lines · Thorsten Blum
Quoted 4 lines

Though the fix is incorrect for CPU_CAVIUM_OCTEON, because it doesn't allow one to access the second half of the last register, and I find it exceedingly complex anyway. Just:

Quoted 8 lines
Quoted 4 lines

#define MAX_REG_OFFSET \
(offsetof(struct pt_regs, __last) - sizeof(unsigned long))

will do (as regs_get_register' operates on unsigned long' quantities).

Does regs_get_register() even work for CPU_CAVIUM_OCTEON when accessing the last two registers because they're both ULL, not UL? (independent of my patch)

Or rather two arrays of registers. With 32-bit configurations their
contents have to be retrieved by pieces. I don't know if it's handled by the caller(s) though as I'm not familiar with this interface.

Maciej

author
#6 lore raw
Quoted 10 lines · Maciej W. Rozycki
Quoted 9 lines · Thorsten Blum
Quoted 8 lines

Though the fix is incorrect for CPU_CAVIUM_OCTEON, because it doesn't allow one to access the second half of the last register, and I find it exceedingly complex anyway. Just:

#define MAX_REG_OFFSET \
(offsetof(struct pt_regs, __last) - sizeof(unsigned long))

will do (as regs_get_register' operates on unsigned long' quantities).

Quoted 7 lines
Quoted 3 lines

Does regs_get_register() even work for CPU_CAVIUM_OCTEON when accessing the last two registers because they're both ULL, not UL? (independent of my patch)

Or rather two arrays of registers. With 32-bit configurations their contents have to be retrieved by pieces. I don't know if it's handled by the caller(s) though as I'm not familiar with this interface.

Ah, CPU_CAVIUM_OCTEON seems to be 64-bit only, so there's no difference between UL and ULL. Then both my patch and your suggestion:

  #define MAX_REG_OFFSET (offsetof(struct pt_regs, __last) - sizeof(unsigned long))

should be fine.

I still prefer my approach without '__last[0]' because it also silences the following false-positive Coccinelle warning, which is how I stumbled upon this in the first place:

./ptrace.h:51:15-21: WARNING use flexible-array member instead

Would it make sense to also change the register arrays 'mpl' and 'mtp' from ULL to UL? ULL seems unnecessarily confusing to me.

Thanks,
Thorsten

#7 lore raw
Quoted 10 lines · Thorsten Blum
Quoted 7 lines
Quoted 3 lines

Does regs_get_register() even work for CPU_CAVIUM_OCTEON when accessing the last two registers because they're both ULL, not UL? (independent of my patch)

Or rather two arrays of registers. With 32-bit configurations their contents have to be retrieved by pieces. I don't know if it's handled by the caller(s) though as I'm not familiar with this interface.

Ah, CPU_CAVIUM_OCTEON seems to be 64-bit only, so there's no difference between UL and ULL. Then both my patch and your suggestion:

So it seems odd to use `long long int' here, but I can't be bothered to
check history. There could be a valid reason or it could be just sloppy coding.

Quoted 5 lines

I still prefer my approach without '__last[0]' because it also silences the following false-positive Coccinelle warning, which is how I stumbled upon this in the first place:

./ptrace.h:51:15-21: WARNING use flexible-array member instead

So make `__last' a flexible array instead? With a separate patch.

Quoted 2 lines

Would it make sense to also change the register arrays 'mpl' and 'mtp' from ULL to UL? ULL seems unnecessarily confusing to me.

Maybe, but I'm not familiar enough with the Cavium Octeon platform to
decide offhand and I won't dive into it, sorry.

Maciej

author
#8 lore raw
Quoted 16 lines · Maciej W. Rozycki
Quoted 10 lines · Thorsten Blum
Quoted 7 lines
Quoted 3 lines

Does regs_get_register() even work for CPU_CAVIUM_OCTEON when accessing the last two registers because they're both ULL, not UL? (independent of my patch)

Or rather two arrays of registers. With 32-bit configurations their contents have to be retrieved by pieces. I don't know if it's handled by the caller(s) though as I'm not familiar with this interface.

Ah, CPU_CAVIUM_OCTEON seems to be 64-bit only, so there's no difference between UL and ULL. Then both my patch and your suggestion:

So it seems odd to use `long long int' here, but I can't be bothered to check history. There could be a valid reason or it could be just sloppy coding.

Quoted 7 lines
Quoted 5 lines

I still prefer my approach without '__last[0]' because it also silences the following false-positive Coccinelle warning, which is how I stumbled upon this in the first place:

./ptrace.h:51:15-21: WARNING use flexible-array member instead

So make `__last' a flexible array instead? With a separate patch.

No, '__last[0]' is a fake flexible array and the Coccinelle warning is wrong. We should either ignore the warning or silence it by removing the marker, but turning it into a real flexible array doesn't make sense. I'd prefer to just remove it from the struct.

Stefan or Oleg, do you have any preference?

Quoted 5 lines

Would it make sense to also change the register arrays 'mpl' and 'mtp'

Quoted 1 line

from ULL to UL? ULL seems unnecessarily confusing to me.

Maybe, but I'm not familiar enough with the Cavium Octeon platform to decide offhand and I won't dive into it, sorry.

Everything I've found about Cavium Octeon indicates it's 64-bit only, so whether we use UL or ULL doesn't matter - they're both 64-bit values.

Thorsten

author
#9 lore raw
Quoted 23 lines · Thorsten Blum
Quoted 22 lines · Maciej W. Rozycki
Quoted 10 lines · Thorsten Blum
Quoted 7 lines
Quoted 3 lines

Does regs_get_register() even work for CPU_CAVIUM_OCTEON when accessing the last two registers because they're both ULL, not UL? (independent of my patch)

Or rather two arrays of registers. With 32-bit configurations their contents have to be retrieved by pieces. I don't know if it's handled by the caller(s) though as I'm not familiar with this interface.

Ah, CPU_CAVIUM_OCTEON seems to be 64-bit only, so there's no difference between UL and ULL. Then both my patch and your suggestion:

So it seems odd to use `long long int' here, but I can't be bothered to check history. There could be a valid reason or it could be just sloppy coding.

Quoted 5 lines

I still prefer my approach without '__last[0]' because it also silences the following false-positive Coccinelle warning, which is how I stumbled upon this in the first place:

./ptrace.h:51:15-21: WARNING use flexible-array member instead

Quoted 8 lines
Quoted 1 line

So make `__last' a flexible array instead? With a separate patch.

No, '__last[0]' is a fake flexible array and the Coccinelle warning is wrong. We should either ignore the warning or silence it by removing the marker, but turning it into a real flexible array doesn't make sense. I'd prefer to just remove it from the struct.

Stefan or Oleg, do you have any preference?

Sorry, I meant Thomas, not Stefan.

#10 lore raw
Quoted 27 lines · Thorsten Blum
Quoted 25 lines · Thorsten Blum
Quoted 23 lines · Maciej W. Rozycki
Quoted 10 lines · Thorsten Blum
Quoted 7 lines
Quoted 3 lines

Does regs_get_register() even work for CPU_CAVIUM_OCTEON when accessing the last two registers because they're both ULL, not UL? (independent of my patch)

Or rather two arrays of registers. With 32-bit configurations their contents have to be retrieved by pieces. I don't know if it's handled by the caller(s) though as I'm not familiar with this interface.

Ah, CPU_CAVIUM_OCTEON seems to be 64-bit only, so there's no difference between UL and ULL. Then both my patch and your suggestion:

So it seems odd to use `long long int' here, but I can't be bothered to check history. There could be a valid reason or it could be just sloppy coding.

Quoted 5 lines

I still prefer my approach without '__last[0]' because it also silences the following false-positive Coccinelle warning, which is how I stumbled upon this in the first place:

./ptrace.h:51:15-21: WARNING use flexible-array member instead

So make `__last' a flexible array instead? With a separate patch.

Quoted 8 lines
Quoted 6 lines

No, '__last[0]' is a fake flexible array and the Coccinelle warning is wrong. We should either ignore the warning or silence it by removing the marker, but turning it into a real flexible array doesn't make sense. I'd prefer to just remove it from the struct.

Stefan or Oleg, do you have any preference?

Sorry, I meant Thomas, not Stefan.

In my opinion just changing __last[0] to __last[] is OK, no other actions needed.

Huacai

Quoted 2 lines
author
#11 lore raw

Hi Huacai,

Quoted 32 lines · Huacai Chen
Quoted 31 lines · Thorsten Blum
Quoted 30 lines · Thorsten Blum
Quoted 16 lines · Maciej W. Rozycki
Quoted 10 lines · Thorsten Blum
Quoted 7 lines
Quoted 3 lines

Does regs_get_register() even work for CPU_CAVIUM_OCTEON when accessing the last two registers because they're both ULL, not UL? (independent of my patch)

Or rather two arrays of registers. With 32-bit configurations their contents have to be retrieved by pieces. I don't know if it's handled by the caller(s) though as I'm not familiar with this interface.

Ah, CPU_CAVIUM_OCTEON seems to be 64-bit only, so there's no difference between UL and ULL. Then both my patch and your suggestion:

So it seems odd to use `long long int' here, but I can't be bothered to check history. There could be a valid reason or it could be just sloppy coding.

Quoted 7 lines
Quoted 5 lines

I still prefer my approach without '__last[0]' because it also silences the following false-positive Coccinelle warning, which is how I stumbled upon this in the first place:

./ptrace.h:51:15-21: WARNING use flexible-array member instead

So make `__last' a flexible array instead? With a separate patch.

No, '__last[0]' is a fake flexible array and the Coccinelle warning is wrong. We should either ignore the warning or silence it by removing the marker, but turning it into a real flexible array doesn't make sense. I'd prefer to just remove it from the struct.

Quoted 5 lines
Quoted 3 lines
Quoted 1 line

Stefan or Oleg, do you have any preference?

Sorry, I meant Thomas, not Stefan.

In my opinion just changing __last[0] to __last[] is OK, no other actions needed.

That doesn't fix the value of MAX_REG_OFFSET - you might be missing some of the context here.

Thanks,
Thorsten

maintainer
#12 lore raw
In reply to Thorsten Blum
Quoted 26 lines · Thorsten Blum
Quoted 25 lines · Thorsten Blum
Quoted 23 lines · Maciej W. Rozycki
Quoted 10 lines · Thorsten Blum
Quoted 7 lines
Quoted 3 lines

Does regs_get_register() even work for CPU_CAVIUM_OCTEON when accessing the last two registers because they're both ULL, not UL? (independent of my patch)

Or rather two arrays of registers. With 32-bit configurations their contents have to be retrieved by pieces. I don't know if it's handled by the caller(s) though as I'm not familiar with this interface.

Ah, CPU_CAVIUM_OCTEON seems to be 64-bit only, so there's no difference between UL and ULL. Then both my patch and your suggestion:

So it seems odd to use `long long int' here, but I can't be bothered to check history. There could be a valid reason or it could be just sloppy coding.

Quoted 5 lines

I still prefer my approach without '__last[0]' because it also silences the following false-positive Coccinelle warning, which is how I stumbled upon this in the first place:

./ptrace.h:51:15-21: WARNING use flexible-array member instead

So make `__last' a flexible array instead? With a separate patch.

Quoted 8 lines
Quoted 6 lines

No, '__last[0]' is a fake flexible array and the Coccinelle warning is wrong. We should either ignore the warning or silence it by removing the marker, but turning it into a real flexible array doesn't make sense. I'd prefer to just remove it from the struct.

Stefan or Oleg, do you have any preference?

Sorry, I meant Thomas, not Stefan.

I don't like the #ifdefery, so please keep __last

Thomas.

Signature · 2 lines
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]
maintainer
#13 lore raw
In reply to Thorsten Blum
Quoted 2 lines · Thorsten Blum

Would it make sense to also change the register arrays 'mpl' and 'mtp' from ULL to UL? ULL seems unnecessarily confusing to me.

no, ULL is correct. These registers are always 64bit independent whether CPU is in 32bit or 64bit mode.

Thomas.

Signature · 2 lines
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]