Re: [PATCH] mm/codetag: clear tags before swap

[David Wang]

At 2024-12-12 15:09:59, "Suren Baghdasaryan" <surenb@xxxxxxxxxx> wrote:
>On Wed, Dec 11, 2024 at 8:03 PM David Wang <00107082@xxxxxxx> wrote:
>>
>> When CONFIG_MEM_ALLOC_PROFILING_DEBUG is set, kernel WARN would be
>> triggered when calling __alloc_tag_ref_set() during swap:
>>
>>         alloc_tag was not cleared (got tag for mm/filemap.c:1951)
>>         WARNING: CPU: 0 PID: 816 at ./include/linux/alloc_tag.h...
>>
>> Clear code tags before swap can fix the warning. And this patch also fix
>> a potential invalid address dereference in alloc_tag_add_check() when
>> CONFIG_MEM_ALLOC_PROFILING_DEBUG is set and ref->ct is CODETAG_EMPTY,
>> which is defined as ((void *)1).
>^^^
>Good catch!
>
>>
>> Signed-off-by: David Wang <00107082@xxxxxxx>
>> Reported-by: kernel test robot <oliver.sang@xxxxxxxxx>
>> Closes: https://lore.kernel.org/oe-lkp/202412112227.df61ebb-lkp@xxxxxxxxx
>> ---
>>  include/linux/alloc_tag.h | 2 +-
>>  lib/alloc_tag.c           | 4 ++++
>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/alloc_tag.h b/include/linux/alloc_tag.h
>> index 7c0786bdf9af..cba024bf2db3 100644
>> --- a/include/linux/alloc_tag.h
>> +++ b/include/linux/alloc_tag.h
>> @@ -135,7 +135,7 @@ static inline struct alloc_tag_counters alloc_tag_read(struct alloc_tag *tag)
>>  #ifdef CONFIG_MEM_ALLOC_PROFILING_DEBUG
>>  static inline void alloc_tag_add_check(union codetag_ref *ref, struct alloc_tag *tag)
>>  {
>> -       WARN_ONCE(ref && ref->ct,
>> +       WARN_ONCE(ref && ref->ct && !is_codetag_empty(ref),
>>                   "alloc_tag was not cleared (got tag for %s:%u)\n",
>>                   ref->ct->filename, ref->ct->lineno);
>>
>> diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
>> index 35f7560a309a..cc5fda9901c2 100644
>> --- a/lib/alloc_tag.c
>> +++ b/lib/alloc_tag.c
>> @@ -209,6 +209,10 @@ void pgalloc_tag_swap(struct folio *new, struct folio *old)
>>                 return;
>>         }
>>
>> +       /* clear tags before swap */
>
>The above comment states what we already know from the code but does
>not explain why we do this. Better to describe the reason and not what
>we do. Something like:
>
>/*
> * Clear tag references to avoid debug warning when using
> *  __alloc_tag_ref_set() with non-empty reference.
> */
>

Copy that~!


Thanks!
David
>> +       set_codetag_empty(&ref_old);
>> +       set_codetag_empty(&ref_new);
>> +
>>         /* swap tags */
>>         __alloc_tag_ref_set(&ref_old, tag_new);
>>         update_page_tag_ref(handle_old, &ref_old);
>> --
>> 2.39.2
>>
>>