linux-kernel - Re: [PATCH] mm/migrate: Fix NULL movable_ops if CONFIG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAAhV-H7eX2OOaT=kMdvTZP5mO=_vpCs54h9_=xai6qPUJJixog@mail.gmail.com>
Date: Sun, 17 Aug 2025 16:34:01 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: David Hildenbrand <david@...hat.com>
Cc: Matthew Wilcox <willy@...radead.org>, Huacai Chen <chenhuacai@...ngson.cn>, 
	Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org, Zi Yan <ziy@...dia.com>, 
	Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, Harry Yoo <harry.yoo@...cle.com>, 
	linux-kernel@...r.kernel.org, Minchan Kim <minchan@...nel.org>, 
	Sergey Senozhatsky <senozhatsky@...omium.org>, "Michael S. Tsirkin" <mst@...hat.com>
Subject: Re: [PATCH] mm/migrate: Fix NULL movable_ops if CONFIG_ZSMALLOC=m

On Sun, Aug 17, 2025 at 4:05 PM David Hildenbrand <david@...hat.com> wrote:
>
> On 17.08.25 03:37, Huacai Chen wrote:
> > On Sun, Aug 17, 2025 at 1:02 AM David Hildenbrand <david@...hat.com> wrote:
> >>
> >> On 16.08.25 18:23, Matthew Wilcox wrote:
> >>> On Sat, Aug 16, 2025 at 12:54:52PM +0200, David Hildenbrand wrote:
> >>>> +++ b/mm/balloon_compaction.c
> >>>> @@ -256,8 +256,10 @@ const struct movable_operations balloon_mops = {
> >>>>    static int __init balloon_init(void)
> >>>>    {
> >>>> -    movable_ops[MOVABLE_BALLOON] = &balloon_mops;
> >>>> -    return 0;
> >>>> +    int rc;
> >>>> +
> >>>> +    rc = register_movable_ops(&balloon_mops, PGTY_offline);
> >>>> +    return rc;
> >>>
> >>> Using 'rc' as the name of this variable is an anti-pattern.  All it
> >>> tells you is "this is the return value".  Calling it 'err' is far
> >>> better because now we know it's an error number (or zero for success,
> >>> of course).
> >>
> >> I know, we all have our things to complain about. Some about Cc: above
> >> --, others about the name of error variables :P
> >>
> >> $ git grep "int rc" | wc -l
> >> 12730
> >> $ git grep "int ret" | wc -l
> >> 80386
> >> $ git grep "int error" | wc -l
> >> 4349
> >> $ git grep "int err " | wc -l
> >> 6117
> >>
> >>>
> >>> It seems to be a particularly IBM derived antipattern ;-)
> >>
> >> Careful miser :D
> >>
> >>> Some internal style guide, perhaps?
> >>
> >> Kernel-internal style guide maybe ;)
> >>
> >>>
> >>>> +void unregister_movable_ops(const struct movable_operations *ops, enum pagetype type)
> >>>> +{
> >>>> +    switch (type) {
> >>>> +    case PGTY_offline:
> >>>> +            WARN_ON_ONCE(offline_movable_ops != ops);
> >>>> +            offline_movable_ops = NULL;
> >>>> +            break;
> >>>> +    case PGTY_zsmalloc:
> >>>> +            WARN_ON_ONCE(zsmalloc_movable_ops != ops);
> >>>> +            zsmalloc_movable_ops = NULL;
> >>>> +            break;
> >>>
> >>> This might be a bit excessive ... just passing the pagetype and not
> >>> having the sanity checks should be enough for the tiny number of users
> >>> this interface will have.
> >>
> >> Yeah, no strong opinion, this was a 3 minute hack.
> > I have tested your code, everything works well. But if the checking is
> > too excessive, can I use a set_movalbe_ops() to replace both
> > register_movable_ops() and unregister_movable_ops()?
>
> Makes sense, given that I don't expect us to have multiple handlers for
> the same type in the near future.
>
> Please do keep a sanity check for replacing valid pointers
>
> if (old_ops && new_ops)
>         return -EBUSY;
OK, will done in V3 later.

Huacai

>
> Thanks!
>
> --
> Cheers
>
> David / dhildenb
>