tracing: fprobe: Remove __packed from generic __fprobe_header

[PATCH] tracing: fprobe: Remove __packed from generic __fprobe_header

Posted by Markus Schneider-Pargmann (The Capable Hub) 1 month, 2 weeks ago

fp pointer and unsigned long have the same size on all relevant
architectures that build Linux. Furthermore this struct is only used in
architectures that do not set ARCH_DEFINE_ENCODE_FPROBE_HEADER which is
set only for 64bit architectures (apart from LoongArch).

Both fields are aligned on these architectures so the struct with
__packed and without it are the same.

Remove the __packed as it is unnecessary.

Fixes: 4346ba160409 ("fprobe: Rewrite fprobe on function-graph tracer")
Signed-off-by: Markus Schneider-Pargmann (The Capable Hub) <msp@baylibre.com>
---
 kernel/trace/fprobe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
index cc49ebd2a773..21751dcdb7b9 100644
--- a/kernel/trace/fprobe.c
+++ b/kernel/trace/fprobe.c
@@ -181,7 +181,7 @@ static inline void read_fprobe_header(unsigned long *stack,
 struct __fprobe_header {
 	struct fprobe *fp;
 	unsigned long size_words;
-} __packed;
+};
 
 #define FPROBE_HEADER_SIZE_IN_LONG	SIZE_IN_LONG(sizeof(struct __fprobe_header))
 

---
base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
change-id: 20260427-topic-fprobe-packed-v7-1-f44f9bbdedf6

Best regards,
--  
Markus Schneider-Pargmann (The Capable Hub) <msp@baylibre.com>

Re: [PATCH] tracing: fprobe: Remove __packed from generic __fprobe_header

Posted by Masami Hiramatsu (Google) 6 days, 18 hours ago

Hi Markus,

Thanks for ping me.

On Tue, 28 Apr 2026 10:30:29 +0200
"Markus Schneider-Pargmann (The Capable Hub)" <msp@baylibre.com> wrote:

> fp pointer and unsigned long have the same size on all relevant
> architectures that build Linux. Furthermore this struct is only used in
> architectures that do not set ARCH_DEFINE_ENCODE_FPROBE_HEADER which is
> set only for 64bit architectures (apart from LoongArch).
> 
> Both fields are aligned on these architectures so the struct with
> __packed and without it are the same.
> 
> Remove the __packed as it is unnecessary.
> 
> Fixes: 4346ba160409 ("fprobe: Rewrite fprobe on function-graph tracer")

NOTE: This is not a Fix, but just cleanup or minor update. Or, you have
any problem with this __packed attribute?

Unless there is no problem (or any concern), I would like to keep this
as it is.

Thank you,

> Signed-off-by: Markus Schneider-Pargmann (The Capable Hub) <msp@baylibre.com>
> ---
>  kernel/trace/fprobe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> index cc49ebd2a773..21751dcdb7b9 100644
> --- a/kernel/trace/fprobe.c
> +++ b/kernel/trace/fprobe.c
> @@ -181,7 +181,7 @@ static inline void read_fprobe_header(unsigned long *stack,
>  struct __fprobe_header {
>  	struct fprobe *fp;
>  	unsigned long size_words;
> -} __packed;
> +};
>  
>  #define FPROBE_HEADER_SIZE_IN_LONG	SIZE_IN_LONG(sizeof(struct __fprobe_header))
>  
> 
> ---
> base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
> change-id: 20260427-topic-fprobe-packed-v7-1-f44f9bbdedf6
> 
> Best regards,
> --  
> Markus Schneider-Pargmann (The Capable Hub) <msp@baylibre.com>
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH] tracing: fprobe: Remove __packed from generic __fprobe_header

Posted by David Laight 6 days, 16 hours ago

On Wed, 10 Jun 2026 17:17:40 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> Hi Markus,
> 
> Thanks for ping me.
> 
> On Tue, 28 Apr 2026 10:30:29 +0200
> "Markus Schneider-Pargmann (The Capable Hub)" <msp@baylibre.com> wrote:
> 
> > fp pointer and unsigned long have the same size on all relevant
> > architectures that build Linux. Furthermore this struct is only used in
> > architectures that do not set ARCH_DEFINE_ENCODE_FPROBE_HEADER which is
> > set only for 64bit architectures (apart from LoongArch).
> > 
> > Both fields are aligned on these architectures so the struct with
> > __packed and without it are the same.
> > 
> > Remove the __packed as it is unnecessary.
> > 
> > Fixes: 4346ba160409 ("fprobe: Rewrite fprobe on function-graph tracer")  
> 
> NOTE: This is not a Fix, but just cleanup or minor update. Or, you have
> any problem with this __packed attribute?
> 
> Unless there is no problem (or any concern), I would like to keep this
> as it is.

There is likely to be a difference on architectures that fault misaligned
accesses.
On those gcc will use multiple byte-sized accesses (and a log of shifts etc)
for code that accesses those members because it will assume that the
structure itself can be misaligned.

So you only want __packed on structures that might be misaligned and those
that contain misaligned members.

If the structure is only guaranteed to be 32bit aligned then use __packed
__aligned(4) so that two 32bit accesses get used instead of 8 8bit ones.

-- David

> 
> Thank you,
> 
> > Signed-off-by: Markus Schneider-Pargmann (The Capable Hub) <msp@baylibre.com>
> > ---
> >  kernel/trace/fprobe.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> > index cc49ebd2a773..21751dcdb7b9 100644
> > --- a/kernel/trace/fprobe.c
> > +++ b/kernel/trace/fprobe.c
> > @@ -181,7 +181,7 @@ static inline void read_fprobe_header(unsigned long *stack,
> >  struct __fprobe_header {
> >  	struct fprobe *fp;
> >  	unsigned long size_words;
> > -} __packed;
> > +};
> >  
> >  #define FPROBE_HEADER_SIZE_IN_LONG	SIZE_IN_LONG(sizeof(struct __fprobe_header))
> >  
> > 
> > ---
> > base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
> > change-id: 20260427-topic-fprobe-packed-v7-1-f44f9bbdedf6
> > 
> > Best regards,
> > --  
> > Markus Schneider-Pargmann (The Capable Hub) <msp@baylibre.com>
> >   
> 
>

Re: [PATCH] tracing: fprobe: Remove __packed from generic __fprobe_header

Posted by Steven Rostedt 6 days, 7 hours ago

On Wed, 10 Jun 2026 12:06:59 +0100
David Laight <david.laight.linux@gmail.com> wrote:

> So you only want __packed on structures that might be misaligned and those
> that contain misaligned members.
> 
> If the structure is only guaranteed to be 32bit aligned then use __packed
> __aligned(4) so that two 32bit accesses get used instead of 8 8bit ones.
> 
> -- David
> 
> > 
> > Thank you,
> >   
> > > Signed-off-by: Markus Schneider-Pargmann (The Capable Hub) <msp@baylibre.com>
> > > ---
> > >  kernel/trace/fprobe.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> > > index cc49ebd2a773..21751dcdb7b9 100644
> > > --- a/kernel/trace/fprobe.c
> > > +++ b/kernel/trace/fprobe.c
> > > @@ -181,7 +181,7 @@ static inline void read_fprobe_header(unsigned long *stack,
> > >  struct __fprobe_header {
> > >  	struct fprobe *fp;
> > >  	unsigned long size_words;
> > > -} __packed;
> > > +};
> > >  

Does "__packed" really do anything between a pointer and a long?

-- Steve

Re: [PATCH] tracing: fprobe: Remove __packed from generic __fprobe_header

Posted by Mathieu Desnoyers 6 days, 7 hours ago

On 2026-06-10 15:51, Steven Rostedt wrote:
> On Wed, 10 Jun 2026 12:06:59 +0100
> David Laight <david.laight.linux@gmail.com> wrote:
> 
>> So you only want __packed on structures that might be misaligned and those
>> that contain misaligned members.
>>
>> If the structure is only guaranteed to be 32bit aligned then use __packed
>> __aligned(4) so that two 32bit accesses get used instead of 8 8bit ones.
>>
>> -- David
>>
>>>
>>> Thank you,
>>>    
>>>> Signed-off-by: Markus Schneider-Pargmann (The Capable Hub) <msp@baylibre.com>
>>>> ---
>>>>   kernel/trace/fprobe.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
>>>> index cc49ebd2a773..21751dcdb7b9 100644
>>>> --- a/kernel/trace/fprobe.c
>>>> +++ b/kernel/trace/fprobe.c
>>>> @@ -181,7 +181,7 @@ static inline void read_fprobe_header(unsigned long *stack,
>>>>   struct __fprobe_header {
>>>>   	struct fprobe *fp;
>>>>   	unsigned long size_words;
>>>> -} __packed;
>>>> +};
>>>>   
> 
> Does "__packed" really do anything between a pointer and a long?

If that structure is allocated at a non-void-ptr-aligned address, the
packed attribute will ensure that the compiler don't emit instructions
that require aligned loads/stores when accessing those fields.

It does not change the layout of the structure per se in this specific
case, but it informs the compiler about the lack of guarantees about
alignment for the entire structure.

x86 32/64 cannot care less about this, but it's relevant on other
architectures.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [PATCH] tracing: fprobe: Remove __packed from generic __fprobe_header

Posted by Markus Schneider-Pargmann 4 days, 14 hours ago

Hi,

On Wed Jun 10, 2026 at 10:05 PM CEST, Mathieu Desnoyers wrote:
> On 2026-06-10 15:51, Steven Rostedt wrote:
>> On Wed, 10 Jun 2026 12:06:59 +0100
>> David Laight <david.laight.linux@gmail.com> wrote:
>> 
>>> So you only want __packed on structures that might be misaligned and those
>>> that contain misaligned members.
>>>
>>> If the structure is only guaranteed to be 32bit aligned then use __packed
>>> __aligned(4) so that two 32bit accesses get used instead of 8 8bit ones.
>>>
>>> -- David
>>>
>>>>
>>>> Thank you,
>>>>    
>>>>> Signed-off-by: Markus Schneider-Pargmann (The Capable Hub) <msp@baylibre.com>
>>>>> ---
>>>>>   kernel/trace/fprobe.c | 2 +-
>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
>>>>> index cc49ebd2a773..21751dcdb7b9 100644
>>>>> --- a/kernel/trace/fprobe.c
>>>>> +++ b/kernel/trace/fprobe.c
>>>>> @@ -181,7 +181,7 @@ static inline void read_fprobe_header(unsigned long *stack,
>>>>>   struct __fprobe_header {
>>>>>   	struct fprobe *fp;
>>>>>   	unsigned long size_words;
>>>>> -} __packed;
>>>>> +};
>>>>>   
>> 
>> Does "__packed" really do anything between a pointer and a long?
>
> If that structure is allocated at a non-void-ptr-aligned address, the
> packed attribute will ensure that the compiler don't emit instructions
> that require aligned loads/stores when accessing those fields.
>
> It does not change the layout of the structure per se in this specific
> case, but it informs the compiler about the lack of guarantees about
> alignment for the entire structure.
>
> x86 32/64 cannot care less about this, but it's relevant on other
> architectures.

Thanks for your feedback. I checked this before submitting the patch.
The struct is always aligned to sizeof(long):

struct __fprobe_header is only ever accessed through
read_fprobe_header() and write_fprobe_header(). Since the read will only
read what we have previously written, only the write part is relevant
here. write_fprobe_header() is only called from fprobe_fgraph_entry():

  if (write_fprobe_header(&fgraph_data[used], fp, size_words))
    used += FPROBE_HEADER_SIZE_IN_LONG + size_words;

used is always kept aligned to sizeof(long), in fact the above snippet
is the only part where it is actually changed. fgraph_data is assigned
here:

  fgraph_data = fgraph_reserve_data(gops->idx, reserved_words * sizeof(long));

fgraph_reserve_data() returns a pointer into an unsigned long array
ret_stack. ret_stack is allocated with

  ret_stack = kmem_cache_alloc(fgraph_stack_cachep, GFP_KERNEL);

and fgraph_stack_cachep is allocated with

  fgraph_stack_cachep = kmem_cache_create("fgraph_stack",
                                          SHADOW_STACK_SIZE,
                                          SHADOW_STACK_SIZE, 0, NULL);

So as far as I can see everything is sizeof(long) aligned here and it is
not allocated at a non-void-ptr-aligned address.

Best
Markus

Re: [PATCH] tracing: fprobe: Remove __packed from generic __fprobe_header

Posted by Steven Rostedt 4 days, 10 hours ago

On Fri, 12 Jun 2026 14:51:58 +0200
"Markus Schneider-Pargmann" <msp@baylibre.com> wrote:

>   fgraph_data = fgraph_reserve_data(gops->idx, reserved_words * sizeof(long));
> 
> fgraph_reserve_data() returns a pointer into an unsigned long array
> ret_stack. ret_stack is allocated with

Correct. It is in fact a requirement that fgraph_reserve_data() returns
a long aligned pointer.

-- Steve

Re: [PATCH] tracing: fprobe: Remove __packed from generic __fprobe_header

Posted by Markus Schneider-Pargmann 6 days, 17 hours ago

Hi Masami,

On Wed Jun 10, 2026 at 10:17 AM CEST, Masami Hiramatsu wrote:
> Hi Markus,
>
> Thanks for ping me.
>
> On Tue, 28 Apr 2026 10:30:29 +0200
> "Markus Schneider-Pargmann (The Capable Hub)" <msp@baylibre.com> wrote:
>
>> fp pointer and unsigned long have the same size on all relevant
>> architectures that build Linux. Furthermore this struct is only used in
>> architectures that do not set ARCH_DEFINE_ENCODE_FPROBE_HEADER which is
>> set only for 64bit architectures (apart from LoongArch).
>> 
>> Both fields are aligned on these architectures so the struct with
>> __packed and without it are the same.
>> 
>> Remove the __packed as it is unnecessary.
>> 
>> Fixes: 4346ba160409 ("fprobe: Rewrite fprobe on function-graph tracer")
>
> NOTE: This is not a Fix, but just cleanup or minor update. Or, you have
> any problem with this __packed attribute?

Thanks, yes it is not fixing a bug, I can remove this.

>
> Unless there is no problem (or any concern), I would like to keep this
> as it is.

There is currently no problem with __packed in the upstream kernel. I
just thought this would be a good cleanup to remove the unnecessary
attribute. I am working on CHERI architectures where pointers have
capabilities. __packed breaks these capability tags and therefore
doesn't work on CHERI. When looking into why this struct has a __packed
attribute I didn't see a reason, so I thought this would be a good patch
for upstream as well even though CHERI is not yet relevant for upstream
linux.

Best
Markus

Re: [PATCH] tracing: fprobe: Remove __packed from generic __fprobe_header

Posted by Markus Schneider-Pargmann 6 days, 19 hours ago

Hi,

On Tue Apr 28, 2026 at 10:30 AM CEST, Markus Schneider-Pargmann (The Capable Hub) wrote:
> fp pointer and unsigned long have the same size on all relevant
> architectures that build Linux. Furthermore this struct is only used in
> architectures that do not set ARCH_DEFINE_ENCODE_FPROBE_HEADER which is
> set only for 64bit architectures (apart from LoongArch).
>
> Both fields are aligned on these architectures so the struct with
> __packed and without it are the same.
>
> Remove the __packed as it is unnecessary.

Friendly ping on this.

Best
Markus

>
> Fixes: 4346ba160409 ("fprobe: Rewrite fprobe on function-graph tracer")
> Signed-off-by: Markus Schneider-Pargmann (The Capable Hub) <msp@baylibre.com>
> ---
>  kernel/trace/fprobe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> index cc49ebd2a773..21751dcdb7b9 100644
> --- a/kernel/trace/fprobe.c
> +++ b/kernel/trace/fprobe.c
> @@ -181,7 +181,7 @@ static inline void read_fprobe_header(unsigned long *stack,
>  struct __fprobe_header {
>  	struct fprobe *fp;
>  	unsigned long size_words;
> -} __packed;
> +};
>  
>  #define FPROBE_HEADER_SIZE_IN_LONG	SIZE_IN_LONG(sizeof(struct __fprobe_header))
>  
>
> ---
> base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
> change-id: 20260427-topic-fprobe-packed-v7-1-f44f9bbdedf6
>
> Best regards,
> --  
> Markus Schneider-Pargmann (The Capable Hub) <msp@baylibre.com>