linux-kernel - Re: [patch] mm, thp: fix defrag setting if newline is not used

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <887178bd-1590-673d-eb30-4c257a2b060e@suse.cz>
Date:   Wed, 15 Jan 2020 13:45:20 +0100
From:   Vlastimil Babka <vbabka@...e.cz>
To:     David Rientjes <rientjes@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     Mel Gorman <mgorman@...hsingularity.net>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [patch] mm, thp: fix defrag setting if newline is not used

On 1/15/20 2:58 AM, David Rientjes wrote:
> If thp defrag setting "defer" is used and a newline is *not* used when
> writing to the sysfs file, this is interpreted as the "defer+madvise"
> option.
> 
> This is because we do prefix matching and if five characters are written
> without a newline, the current code ends up comparing to the first five
> bytes of the "defer+madvise" option and using that instead.
> 
> Find the length of what the user is writing and use that to guide our
> decision on which string comparison to do.
> 
> Fixes: 21440d7eb904 ("mm, thp: add new defer+madvise defrag option")
> Signed-off-by: David Rientjes <rientjes@...gle.com>
> ---
>  This can be done in *many* different ways including extracting logic to
>  a helper function.  If someone would like this to be implemented
>  differently, please suggest it.

I've come up with this:

diff --git mm/huge_memory.c mm/huge_memory.c
index 41a0fbddc96b..f36b93334874 100644
--- mm/huge_memory.c
+++ mm/huge_memory.c
@@ -256,7 +256,7 @@ static ssize_t defrag_store(struct kobject *kobj,
                clear_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_OR_MADV_FLAG, &transparent_hugepage_flags);
                clear_bit(TRANSPARENT_HUGEPAGE_DEFRAG_REQ_MADV_FLAG, &transparent_hugepage_flags);
                set_bit(TRANSPARENT_HUGEPAGE_DEFRAG_DIRECT_FLAG, &transparent_hugepage_flags);
-       } else if (!memcmp("defer+madvise", buf,
+       } else if (count > sizeof("defer")-1 && !memcmp("defer+madvise", buf,
                    min(sizeof("defer+madvise")-1, count))) {
                clear_bit(TRANSPARENT_HUGEPAGE_DEFRAG_DIRECT_FLAG, &transparent_hugepage_flags);
                clear_bit(TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_FLAG, &transparent_hugepage_flags);

It's smaller, but more hacky. But it doesn't add new restrictions.
E.g. this still works:

# echo -n 'alw' > /sys/kernel/mm/transparent_hugepage/defrag
# cat /sys/kernel/mm/transparent_hugepage/defrag
[always] defer defer+madvise madvise never

But whether anyone does that, I don't know (it doesn't work without -n).
Also this still works:

# echo -n  'defer   ' > /sys/kernel/mm/transparent_hugepage/defrag
# cat /sys/kernel/mm/transparent_hugepage/defrag
always [defer] defer+madvise madvise never

Ideally we would have had strict matching as you propose (no matching of prefixes)
since the beginning and use e.g. strstrip() to remove all whitespace from buffer
first. But it's 'const char *' and I'm not sure if it's null-terminated.