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

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

[Dan Carpenter]

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.

    610                 return -EINVAL;
    611 
    612         /*
    613          * No kind of locking is needed in here, because ->can_attach() will
    614          * check this value once in the beginning of the process, and then carry
    615          * on with stale data. This means that changes to this value will only
    616          * affect task migrations starting after the change.
    617          */
    618         memcg->move_charge_at_immigrate = val;
    619         return 0;
    620 }

regards,
dan carpenter

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

[Roman Gushchin]

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.

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

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

[Michal Hocko]

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