| 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
| ||
|
Message-ID: <20171219012611.5tx3zx26iqxr7ada@redbean.fios-router.home>
Date: Tue, 19 Dec 2017 02:26:13 +0100
From: Jessica Yu <jeyu@...nel.org>
To: "Luis R. Rodriguez" <mcgrof@...nel.org>
Cc: rusty@...tcorp.com.au, keescook@...omium.org, tixxdz@...il.com,
mbenes@...e.cz, atomlin@...hat.com, pmladek@...e.com,
hare@...e.com, james.l.morris@...cle.com, ebiederm@...ssion.com,
davem@...emloft.net, akpm@...ux-foundation.org,
torvalds@...ux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: module: avoid allocation if module is already present and ready
+++ Luis R. Rodriguez [07/12/17 16:15 -0800]:
>load_module() will allocate a struct module before even checking
>if the module is already loaded. This can create unecessary memory
>pressure since we can easily just check if the module is already
>present early with the copy of the module information from userspace
>after we've validated it a bit.
>
>This can only be an issue if a system is getting hammered with multiple
>request_module() calls which are issued blindly without first checking
>if the module is already present. Fortunately we have test cases on
>selftests for kmod to prove this now.
>
>This is more evident on kmod selftest test case 0008 than 0009, given
>get_fs_type() has its own checker for if the module is present before
>poking userspace for the module. The two kmod selftests evaluated
>below are:
>
>0008 - multithreaded - push kmod_concurrent over max_modprobes for request_module()
>0009 - multithreaded - push kmod_concurrent over max_modprobes for get_fs_type()
>
>In terms of run time nothing changes much:
>
>Before:
>root@...gy:~# time ./kmod.sh -t 0008
>real 0m29.377s
>user 0m1.291s
>sys 0m17.775s
>
>root@...gy:~# time ./kmod.sh -t 0009
>real 1m4.234s
>user 0m1.172s
>sys 0m15.378s
>
>After:
>root@...gy:~# time ./kmod.sh -t 0008
>real 0m29.191s
>user 0m1.372s
>sys 0m17.428s
>
>root@...gy:~# time ./kmod.sh -t 0009
>real 1m3.972s
>user 0m1.137s
>sys 0m15.870s
>
>But if we generate two memory-use plots during each test before and
>after this commit we can see test case 0008 ends up consuming much
>less memory as the test runs now, saving about 14 MiB in the worst
>case.
>
>For test case 0008 since both test run for about 35 seconds we can
>collect memory-use in KiB for 35 seconds as the test runs, before
>and then after this commit:
>
> # For test case 0008, run for 35 seconds, before this commit:
>free -k -s 1 -c 35 | grep Mem | awk '{print $3}' > log-0008-before.txt
> # For test case 0008, run for 35 seconds, after this commit:
>free -k -s 1 -c 35 | grep Mem | awk '{print $3}' > log-0008-after.txt
>
>Likewise for test 0009 since both tests run for about 75 seconds we can
>collect memory-use in KiB for 75 seconds as the test runs, before
>and then after this commit:
>
> # For test case 0009, run for 75 seconds, before this commit:
>free -k -s 1 -c 75 | grep Mem | awk '{print $3}' > log-0009-before.txt
> # For test case 0008, run for 75 seconds, after this commit:
>free -k -s 1 -c 75 | grep Mem | awk '{print $3}' > log-0009-after.txt
>
>Assuming we have a kmod.plot for gnuplut as follows:
>
>cat kmod.gnuplot
>set term dumb
>set output fileout
>plot filein with linespoints title "Memory usage (KiB)"
>
>We can now plot each graph:
>
>gnuplot -e filein='log-0008-before.txt' -e fileout='graph-0008-before.txt' kmod.gnuplot
>gnuplot -e filein='log-0009-before.txt' -e fileout='graph-0009-before.txt' kmod.gnuplot
>gnuplot -e filein='log-0008-after.txt' -e fileout='graph-0008-after.txt' kmod.gnuplot
>gnuplot -e filein='log-0009-after.txt' -e fileout='graph-0009-after.txt' kmod.gnuplot
>
>kmod selftest 0008 before:
>
> 124000 +-----------------------------------------------------------------+
> | + + * + + + + |
> | * * Memory usage (KiB) ***A*** |
> 123000 |-+ * * A +-|
> | * * * * A*A |
> | A * A *A * * * *A*A* A |
> | A * * ** *A*A **A *A A*A A * |
> 122000 |-+ * A* * A** A A A * +-|
> | *A A*A*A* * A A*A*A |
> | A A |
> 121000 |-+ * +-|
> | * |
> | * |
> 120000 |-+* +-|
> | * |
> | * |
> | * |
> 119000 |*A +-|
> | |
> | + + + + + + |
> 118000 +-----------------------------------------------------------------+
> 0 5 10 15 20 25 30 35
>
>kmod selftest 0008 after:
>
> 110000 +-----------------------------------------------------------------+
> | A + + + + + |
> 108000 |-+ * Memory usage (KiB) ***A***-|
> | * |
> | ** |
> 106000 |-+ ** +-|
> | * * |
> 104000 |-+ * * +-|
> | * * *A A* *A* *A*A*A*A*A |
> | * * *A * *A* * A*A*A*AA A*A A*A*A*A |
> 102000 |-+ A*A **A*A*A*A AA A +-|
> | * A |
> 100000 |-+ * +-|
> | A |
> | * |
> 98000 |-+* +-|
> | * |
> 96000 |-+* +-|
> | * |
> |*A + + + + + + |
> 94000 +-----------------------------------------------------------------+
> 0 5 10 15 20 25 30 35
>
>The max memory-use before and after:
>
>$ sort -n -r log-0008-before.txt | head -1
>123904
>$ sort -n -r log-0008-after.txt | head -1
>108964
>
>Although this is just one test case the saving of about 14940 KiB
>(~14 MiB) is considerable enough to consider a two line change.
>
>For test case 0009 we don't see much drastic changes:
>
>kmod selftest 0009 before:
>
> 2.2e+06 +----------------------------------------------------------------+
> | + + + A + + + + |
> 2e+06 |-+ * Memory Asage (KiA) ***A***-|
> | A A * * * |
> 1.8e+06 |-+ * * * * * +-|
> 1.6e+06 |-+ ** * * * * +-|
> | *A * * * * |
> 1.4e+06 |-+ A ** ** ** A ** A +-|
> | * ** * * ** * *A * |
> 1.2e+06 |-+ A * ** * * * * A * ** * +-|
> | AA * ** ** A * A* * * * * * ** |
> 1e+06 |-** * A ** ** * * ** * * ** * * * * +-|
> | ** * * *A* * * A ** A A ** ** * A * * |
> 800000 |-** * * *** ** * ** * A * ** ** * * * * +-|
> | ** * *A * ** ** * ** * * * AA *** * * A * * |
> 600000 |-* * A * **** ** ** * *A * * * * **A * *A * * A +-|
> 400000 |-* * *A* **** ** ** A * *** ** *** A ** ** A * +-|
> | * * **A * ** ** ** A * *** ** *A ** ***** * * |
> 200000 |-* ** ** * ** A* ** * * *** ** ** * * **A* *A +-|
> |AA AA A+ A AA A AA AAAAA AAA AA A+ A A AA A A+AAA |
> 0 +----------------------------------------------------------------+
> 0 10 20 30 40 50 60 70 80
>
>kmod selftest 0009 after:
>
> 2.2e+06 +----------------------------------------------------------------+
> | + + + + + A + + |
> 2e+06 |-+ Memory us*ge (KiB) ***A***-|
> | * |
> 1.8e+06 |-+ A * +-|
> 1.6e+06 |-+ * * +-|
> | A * * |
> 1.4e+06 |-+ * * ** +-|
> | * * ** |
> 1.2e+06 |-+ A * * ** +-|
> | * * A * A ** |
> 1e+06 |-+ * * * ** * * * +-|
> | *** A A ** * * * * A |
> 800000 |-+AA ** ** A ** A AAA ** * ** * * A +-|
> | A* * ** ** * *A * A * ** * ** * * * |
> 600000 |-** A*** ***** ** * * * ** * * A * * * +-|
> 400000 |-** ***A ***A * A A** *** A* A* * A * * * *** +-|
> |** *** **** * * ** * A* ** ** A * * ** * * |
> 200000 |** ** **A ** * * * ** ** *A * A A* ** * * +-|
> |AA AA+ AA AA A A AA+A AA A AA AA AA A AAAAAAAAAA |
> 0 +----------------------------------------------------------------+
> 0 10 20 30 40 50 60 70 80
>
>In terms of max memory use we have:
>
>$ sort -n -r log-0009-before.txt | head -1
>2083344
>$ sort -n -r log-0009-after.txt | head -1
>2076236
>
>So there is not much difference for this commit when we hammer on
>get_fs_type().
>
>Signed-off-by: Luis R. Rodriguez <mcgrof@...nel.org>
>---
> kernel/module.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index fb2afcde5d20..6a0e22293502 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -3321,6 +3321,9 @@ static int early_mod_check(struct load_info *info, int flags,
> if (err)
> return err;
>
>+ if (finished_loading(info->name))
>+ return 0;
Did you mean to return -EEXIST here? Otherwise this check doesn't do
anything, i.e. if the module is live then early_mod_check() returns 0
and proceeds to layout_and_allocate() all the same.
Also, finished_loading() wouldn't work here in its current form,
because it also returns true if the module doesn't exist, i.e.
find_module_all() returns NULL. This is because we want to wake up in
add_unformed_module() if the same module failed to load and then went
away, then we can try to add it to the modules list again. Perhaps we
can split out the !mod check in the wait condition in add_unformed_module()
so that we can use finished_loading() here as intended.
Thanks,
Jessica
Powered by blists - more mailing lists