[PATCH v2] MIPS: Fix MAX_REG_OFFSET and remove zero-length struct member

[PATCH v2] MIPS: Fix MAX_REG_OFFSET and remove zero-length struct member

[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.")
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
Compile-tested only.

Changes in v2:
- Fix MAX_REG_OFFSET as suggested by Maciej (thanks!)
- Link to v1: https://lore.kernel.org/lkml/20250411090032.7844-1-thorsten.blum@linux.dev/
---
 arch/mips/include/asm/ptrace.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/ptrace.h b/arch/mips/include/asm/ptrace.h
index 85fa9962266a..23acad11ac8e 100644
--- a/arch/mips/include/asm/ptrace.h
+++ b/arch/mips/include/asm/ptrace.h
@@ -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
-- 
2.49.0

Re: [PATCH v2] MIPS: Fix MAX_REG_OFFSET and remove zero-length struct member

[Thomas Bogendoerfer]

On Thu, Apr 17, 2025 at 07:47:13PM +0200, Thorsten Blum wrote:
> 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.

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

Re: [PATCH v2] MIPS: Fix MAX_REG_OFFSET and remove zero-length struct member

[Thorsten Blum]

On 18. Apr 2025, at 09:57, Thomas Bogendoerfer wrote:
> On Thu, Apr 17, 2025 at 07:47:13PM +0200, Thorsten Blum wrote:
>> 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

Re: [PATCH v2] MIPS: Fix MAX_REG_OFFSET and remove zero-length struct member

[Maciej W. Rozycki]

On Fri, 18 Apr 2025, Thorsten Blum wrote:

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

 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

Re: [PATCH v2] MIPS: Fix MAX_REG_OFFSET and remove zero-length struct member

[Thorsten Blum]

On 18. Apr 2025, at 12:36, Maciej W. Rozycki wrote:
> On Fri, 18 Apr 2025, Thorsten Blum wrote:
>>>> 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.
> 
> 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).

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

Re: [PATCH v2] MIPS: Fix MAX_REG_OFFSET and remove zero-length struct member

[Maciej W. Rozycki]

On Fri, 18 Apr 2025, Thorsten Blum wrote:

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

 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

Re: [PATCH v2] MIPS: Fix MAX_REG_OFFSET and remove zero-length struct member

[Thorsten Blum]

On 18. Apr 2025, at 14:44, Maciej W. Rozycki wrote:
> On Fri, 18 Apr 2025, Thorsten Blum wrote:
>>> 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)
> 
> 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

Re: [PATCH v2] MIPS: Fix MAX_REG_OFFSET and remove zero-length struct member

[Maciej W. Rozycki]

On Fri, 18 Apr 2025, Thorsten Blum wrote:

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

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

> 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

Re: [PATCH v2] MIPS: Fix MAX_REG_OFFSET and remove zero-length struct member

[Thorsten Blum]

On 18. Apr 2025, at 17:14, Maciej W. Rozycki wrote:
> On Fri, 18 Apr 2025, Thorsten Blum wrote:
>>>> 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.
> 
>> 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?

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

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

Re: [PATCH v2] MIPS: Fix MAX_REG_OFFSET and remove zero-length struct member

[Thorsten Blum]

On 18. Apr 2025, at 22:18, Thorsten Blum wrote:
> On 18. Apr 2025, at 17:14, Maciej W. Rozycki wrote:
>> On Fri, 18 Apr 2025, Thorsten Blum wrote:
>>>>> 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.
>> 
>>> 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?

Sorry, I meant Thomas, not Stefan.

Re: [PATCH v2] MIPS: Fix MAX_REG_OFFSET and remove zero-length struct member

[Huacai Chen]

On Sat, Apr 19, 2025 at 4:22 AM Thorsten Blum <thorsten.blum@linux.dev> wrote:
>
> On 18. Apr 2025, at 22:18, Thorsten Blum wrote:
> > On 18. Apr 2025, at 17:14, Maciej W. Rozycki wrote:
> >> On Fri, 18 Apr 2025, Thorsten Blum wrote:
> >>>>> 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.
> >>
> >>> 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?
>
> Sorry, I meant Thomas, not Stefan.
In my opinion just changing __last[0] to __last[] is OK, no other
actions needed.

Huacai
>
>

Re: [PATCH v2] MIPS: Fix MAX_REG_OFFSET and remove zero-length struct member

[Thorsten Blum]

Hi Huacai,

On 19. Apr 2025, at 04:56, Huacai Chen wrote:
> On Sat, Apr 19, 2025 at 4:22 AM Thorsten Blum wrote:
>> On 18. Apr 2025, at 22:18, Thorsten Blum wrote:
>>> On 18. Apr 2025, at 17:14, Maciej W. Rozycki wrote:
>>>> On Fri, 18 Apr 2025, Thorsten Blum wrote:
>>>>>>> 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.
>>>> 
>>>>> 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?
>> 
>> 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

Re: [PATCH v2] MIPS: Fix MAX_REG_OFFSET and remove zero-length struct member

[Thomas Bogendoerfer]

On Fri, Apr 18, 2025 at 10:21:22PM +0200, Thorsten Blum wrote:
> On 18. Apr 2025, at 22:18, Thorsten Blum wrote:
> > On 18. Apr 2025, at 17:14, Maciej W. Rozycki wrote:
> >> On Fri, 18 Apr 2025, Thorsten Blum wrote:
> >>>>> 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.
> >> 
> >>> 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?
> 
> Sorry, I meant Thomas, not Stefan.

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

Thomas.

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

Re: [PATCH v2] MIPS: Fix MAX_REG_OFFSET and remove zero-length struct member

[Thomas Bogendoerfer]

On Fri, Apr 18, 2025 at 03:38:36PM +0200, Thorsten Blum wrote:
> 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.

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