arch/mips/include/asm/ptrace.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
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.0On 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 ]
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
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
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
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
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
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 ]
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
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
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.
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 ]
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 > >
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
© 2016 - 2026 Red Hat, Inc.