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@xxxxxxxxx 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@xxxxxxxxx>
> 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@xxxxxxxxxx>
> Signed-off-by: Roman Gushchin <roman.gushchin@xxxxxxxxx>
Acked-by: Michal Hocko <mhocko@xxxxxxxx>
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
