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

["Markus Schneider-Pargmann" <msp@baylibre.com>]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 289 bytes --]