Re: [PATCH mptcp-next] x86/pm: fix false positive kmemleak report in msr_build_context()

[Matthieu Baerts <matthieu.baerts@tessares.net>]

Hi Mat, Rafael,

(oops, please ignore the "mptcp-next" tag I added by reflex in the
subject: this is not related to MPTCP :) )

On 22/04/2022 13:51, Rafael J. Wysocki wrote:
> On Fri, Apr 22, 2022 at 1:21 AM Mat Martineau
> <mathew.j.martineau@linux.intel.com> wrote:
>>
>> On Thu, 21 Apr 2022, Matthieu Baerts wrote:
>>
>>> Since commit e2a1256b17b1 ("x86/speculation: Restore speculation related MSRs during S3 resume"),
>>> kmemleak reports this issue:
>>>
>>>  unreferenced object 0xffff888009cedc00 (size 256):
>>>    comm "swapper/0", pid 1, jiffies 4294693823 (age 73.764s)
>>>    hex dump (first 32 bytes):
>>>      00 00 00 00 00 00 00 00 48 00 00 00 00 00 00 00  ........H.......
>>>      00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>>>    backtrace:
>>>      msr_build_context (include/linux/slab.h:621)
>>>      pm_check_save_msr (arch/x86/power/cpu.c:520)
>>>      do_one_initcall (init/main.c:1298)
>>>      kernel_init_freeable (init/main.c:1370)
>>>      kernel_init (init/main.c:1504)
>>>      ret_from_fork (arch/x86/entry/entry_64.S:304)
>>>
>>> It is easy to reproduce it on my side:
>>>
>>>  - boot the VM with a debug kernel config [1]
>>>  - wait ~1 minute
>>>  - start a kmemleak scan
>>>
>>> It seems kmemleak has an issue with the array allocated in
>>> msr_build_context() and assigned to a pointer in a static structure
>>> (saved_context.saved_msrs->array): there is no leak then.
>>>
>>> It looks like this is a limitation from kmemleak but that's alright,
>>> kmemleak_no_leak() can be used to avoid complaining about that.
>>>
>>> Please note that it looks like this issue is not new, e.g.
>>>
>>>  https://lore.kernel.org/all/9f1bb619-c4ee-21c4-a251-870bd4db04fa@lwfinger.net/
>>>  https://lore.kernel.org/all/94e48fcd-1dbd-ebd2-4c91-f39941735909@molgen.mpg.de/
>>>
>>> But on my side, msr_build_context() is only used since:
>>>
>>>  commit e2a1256b17b1 ("x86/speculation: Restore speculation related MSRs during S3 resume").
>>>
>>> Depending on their CPUs, others have probably the same issue since:
>>>
>>>  commit 7a9c2dd08ead ("x86/pm: Introduce quirk framework to save/restore extra MSR registers around suspend/resume"),
>>>
>>> hence the 'Fixes' tag here below to help with the backports. But I
>>> understand if someone says the origin of this issue is more on
>>> kmemleak's side. What is unclear to me is why this issue was not seen by
>>> other people and CIs. Maybe the kernel config [1]?
>>>
>>> [1] https://github.com/multipath-tcp/mptcp_net-next/files/8531660/kmemleak-cpu-sched-bisect.kconfig.txt
>>>
>>
>> Hi Matthieu -
>>
>> It looks like the root cause here is alignment within the packed struct
>> saved_context (from suspend_64.h). Kmemleak only searches for pointers
>> that are aligned, but pahole shows that the saved_msrs struct member and
>> all members after it in the structure are unaligned:

@Mat: Thank you for the analysis and finding the root cause!

>> (gcc 11.2.1, x86_64)
>>
>> struct saved_context {
>>         struct pt_regs             regs;                 /*     0   168 */
>>         /* --- cacheline 2 boundary (128 bytes) was 40 bytes ago --- */
>>         u16                        ds;                   /*   168     2 */
>>         u16                        es;                   /*   170     2 */
>>         u16                        fs;                   /*   172     2 */
>>         u16                        gs;                   /*   174     2 */
>>         long unsigned int          kernelmode_gs_base;   /*   176     8 */
>>         long unsigned int          usermode_gs_base;     /*   184     8 */
>>         /* --- cacheline 3 boundary (192 bytes) --- */
>>         long unsigned int          fs_base;              /*   192     8 */
>>         long unsigned int          cr0;                  /*   200     8 */
>>         long unsigned int          cr2;                  /*   208     8 */
>>         long unsigned int          cr3;                  /*   216     8 */
>>         long unsigned int          cr4;                  /*   224     8 */
>>         u64                        misc_enable;          /*   232     8 */
>>         bool                       misc_enable_saved;    /*   240     1 */
>>
>> /* Note odd offset values for the remainder of this struct    vvv       */
>>
>>         struct saved_msrs          saved_msrs;           /*   241    16 */
>>         /* --- cacheline 4 boundary (256 bytes) was 1 bytes ago --- */
>>         long unsigned int          efer;                 /*   257     8 */
>>         u16                        gdt_pad;              /*   265     2 */
>>         struct desc_ptr            gdt_desc;             /*   267    10 */
>>         u16                        idt_pad;              /*   277     2 */
>>         struct desc_ptr            idt;                  /*   279    10 */
>>         u16                        ldt;                  /*   289     2 */
>>         u16                        tss;                  /*   291     2 */
>>         long unsigned int          tr;                   /*   293     8 */
>>         long unsigned int          safety;               /*   301     8 */
>>         long unsigned int          return_address;       /*   309     8 */
>>
>>         /* size: 317, cachelines: 5, members: 25 */
>>         /* last cacheline: 61 bytes */
>> } __attribute__((__packed__));
>>
>> If I move misc_enable_saved to the end of the struct declaration,
>> saved_msrs fits in before the cacheline 4 boundary and the kmemleak
>> warning goes away. The comment above the saved_context declaration says to
>> check wakeup_64.S and __save/__restore_processor_state() if the struct is
>> modified - looks like it's the members before misc_enable that must be
>> carefully placed.
> 
> Yes, you can move misc_enable_saved to the end of it safely AFAICS.

@Rafael: thank you for the reply!

Before doing that, is it still needed to keep the "packed" attribute?
This attribute was already there before the first Git commit.


Without it, I no longer have the kmemleak and pahole reports this:


  struct saved_context {

  (...)
        bool                       misc_enable_saved;    /*   240   1 */

        /* XXX 7 bytes hole, try to pack */

        struct saved_msrs          saved_msrs;           /*   248  16 */

  (...)

        /* size: 328, cachelines: 6, members: 25 */
        /* sum members: 317, holes: 2, sum holes: 11 */
        /* last cacheline: 8 bytes */
  };


Everything is still at the same place before 'misc_enable' member.

If it is important to reduce the cachelines, it is still interesting to
move the bool to avoid a whole which costs one cacheline.


>> So far I've only tried this on my local machine, I'll work on getting more
>> thorough validation.
>>
>> Looks like struct saved_context in suspend_32.h has similar odd alignment.
> 
> Right, and it can be changed too AFAICS.


Thanks!

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net