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

[Suren Baghdasaryan <surenb@google.com>]

On Wed, Dec 11, 2024 at 8:03 PM David Wang <00107082@163.com> 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@163.com>
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202412112227.df61ebb-lkp@intel.com
> ---
>  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.
 */

> +       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
>
>