Re: [bug report] mm: memcg: move charge migration code to memcontrol-v1.c

[Michal Hocko <mhocko@suse.com>]

On Fri 12-07-24 18:56:03, Roman Gushchin wrote:
> On Fri, Jul 12, 2024 at 09:07:45AM -0500, Dan Carpenter wrote:
> > Hello Roman Gushchin,
> > 
> > Commit e548ad4a7cbf ("mm: memcg: move charge migration code to
> > memcontrol-v1.c") from Jun 24, 2024 (linux-next), leads to the
> > following Smatch static checker warning:
> > 
> > 	mm/memcontrol-v1.c:609 mem_cgroup_move_charge_write()
> > 	warn: was expecting a 64 bit value instead of '~(1 | 2)'
> > 
> > mm/memcontrol-v1.c
> >     599 #ifdef CONFIG_MMU
> >     600 static int mem_cgroup_move_charge_write(struct cgroup_subsys_state *css,
> >     601                                  struct cftype *cft, u64 val)
> >     602 {
> >     603         struct mem_cgroup *memcg = mem_cgroup_from_css(css);
> >     604 
> >     605         pr_warn_once("Cgroup memory moving (move_charge_at_immigrate) is deprecated. "
> >     606                      "Please report your usecase to linux-mm@kvack.org if you "
> >     607                      "depend on this functionality.\n");
> >     608 
> > --> 609         if (val & ~MOVE_MASK)
> > 
> > val is a u64 and MOVE_MASK is a u32.  This only checks if something in the low
> > 32 bits is set and it ignores the high 32 bits.
> 
> Hi Dan,
> 
> thank you for the report!
> 
> The mentioned commit just moved to code from memcontrol.c to memcontrol-v1.c,
> so the bug is actually much much older. Anyway, the fix is attached below.
> 
> Andrew, can you please pick it up?
> 
> I don't think the problem and the fix deserve a stable backport.

Agreed!

> Thank you!
> 
> --
> 
> >From 8b3d1ebb8d99cd9d3ab331f69ba9170f09a5c9b6 Mon Sep 17 00:00:00 2001
> From: Roman Gushchin <roman.gushchin@linux.dev>
> Date: Fri, 12 Jul 2024 18:35:14 +0000
> Subject: [PATCH] mm: memcg1: convert charge move flags to unsigned long long
> 
> Currently MOVE_ANON and MOVE_FILE flags are defined as integers
> and it leads to the following Smatch static checker warning:
>     mm/memcontrol-v1.c:609 mem_cgroup_move_charge_write()
>     warn: was expecting a 64 bit value instead of '~(1 | 2)'
> 
> Fix this be redefining them as unsigned long long.
> 
> Even though the issue allows to set high 32 bits of mc.flags
> to an arbitrary number, these bits are never used, so it doesn't
> have any significant consequences.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

> ---
>  mm/memcontrol-v1.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memcontrol-v1.c b/mm/memcontrol-v1.c
> index 6b3e56e88a8a..2aeea4d8bf8e 100644
> --- a/mm/memcontrol-v1.c
> +++ b/mm/memcontrol-v1.c
> @@ -44,8 +44,8 @@ static struct mem_cgroup_tree soft_limit_tree __read_mostly;
>  /*
>   * Types of charges to be moved.
>   */
> -#define MOVE_ANON	0x1U
> -#define MOVE_FILE	0x2U
> +#define MOVE_ANON	0x1ULL
> +#define MOVE_FILE	0x2ULL
>  #define MOVE_MASK	(MOVE_ANON | MOVE_FILE)
>  
>  /* "mc" and its members are protected by cgroup_mutex */
> -- 
> 2.45.2.993.g49e7a77208-goog

-- 
Michal Hocko
SUSE Labs