From c90012ac85c24547e5c3468ef00aabf44aa7332d Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Sun, 6 Oct 2019 10:30:28 +1100 Subject: lib: test_user_copy: style cleanup While writing the tests for copy_struct_from_user(), I used a construct that Linus doesn't appear to be too fond of: On 2019-10-04, Linus Torvalds wrote: > Hmm. That code is ugly, both before and after the fix. > > This just doesn't make sense for so many reasons: > > if ((ret |= test(umem_src == NULL, "kmalloc failed"))) > > where the insanity comes from > > - why "|=" when you know that "ret" was zero before (and it had to > be, for the test to make sense) > > - why do this as a single line anyway? > > - don't do the stupid "double parenthesis" to hide a warning. Make it > use an actual comparison if you add a layer of parentheses. So instead, use a bog-standard check that isn't nearly as ugly. Fixes: 341115822f88 ("usercopy: Add parentheses around assignment in test_copy_struct_from_user") Fixes: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper") Signed-off-by: Aleksa Sarai Reviewed-by: Nathan Chancellor Reviewed-by: Christian Brauner Link: https://lore.kernel.org/r/20191005233028.18566-1-cyphar@cyphar.com Signed-off-by: Christian Brauner --- lib/test_user_copy.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c index e365ace06538..ad2372727b1b 100644 --- a/lib/test_user_copy.c +++ b/lib/test_user_copy.c @@ -52,13 +52,14 @@ static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size) size_t zero_end = size - zero_start; /* - * We conduct a series of check_nonzero_user() tests on a block of memory - * with the following byte-pattern (trying every possible [start,end] - * pair): + * We conduct a series of check_nonzero_user() tests on a block of + * memory with the following byte-pattern (trying every possible + * [start,end] pair): * * [ 00 ff 00 ff ... 00 00 00 00 ... ff 00 ff 00 ] * - * And we verify that check_nonzero_user() acts identically to memchr_inv(). + * And we verify that check_nonzero_user() acts identically to + * memchr_inv(). */ memset(kmem, 0x0, size); @@ -93,11 +94,13 @@ static int test_copy_struct_from_user(char *kmem, char __user *umem, size_t ksize, usize; umem_src = kmalloc(size, GFP_KERNEL); - if ((ret |= test(umem_src == NULL, "kmalloc failed"))) + ret = test(umem_src == NULL, "kmalloc failed"); + if (ret) goto out_free; expected = kmalloc(size, GFP_KERNEL); - if ((ret |= test(expected == NULL, "kmalloc failed"))) + ret = test(expected == NULL, "kmalloc failed"); + if (ret) goto out_free; /* Fill umem with a fixed byte pattern. */ -- cgit v1.2.3 From f418dddffc8007945fd5962380ebde770a240cf5 Mon Sep 17 00:00:00 2001 From: Michael Ellerman Date: Wed, 16 Oct 2019 23:27:32 +1100 Subject: usercopy: Avoid soft lockups in test_check_nonzero_user() On a machine with a 64K PAGE_SIZE, the nested for loops in test_check_nonzero_user() can lead to soft lockups, eg: watchdog: BUG: soft lockup - CPU#4 stuck for 22s! [modprobe:611] Modules linked in: test_user_copy(+) vmx_crypto gf128mul crc32c_vpmsum virtio_balloon ip_tables x_tables autofs4 CPU: 4 PID: 611 Comm: modprobe Tainted: G L 5.4.0-rc1-gcc-8.2.0-00001-gf5a1a536fa14-dirty #1151 ... NIP __might_sleep+0x20/0xc0 LR __might_fault+0x40/0x60 Call Trace: check_zeroed_user+0x12c/0x200 test_user_copy_init+0x67c/0x1210 [test_user_copy] do_one_initcall+0x60/0x340 do_init_module+0x7c/0x2f0 load_module+0x2d94/0x30e0 __do_sys_finit_module+0xc8/0x150 system_call+0x5c/0x68 Even with a 4K PAGE_SIZE the test takes multiple seconds. Instead tweak it to only scan a 1024 byte region, but make it cross the page boundary. Fixes: f5a1a536fa14 ("lib: introduce copy_struct_from_user() helper") Suggested-by: Aleksa Sarai Signed-off-by: Michael Ellerman Reviewed-by: Aleksa Sarai Acked-by: Christian Brauner Link: https://lore.kernel.org/r/20191016122732.13467-1-mpe@ellerman.id.au Signed-off-by: Christian Brauner --- lib/test_user_copy.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c index ad2372727b1b..5ff04d8fe971 100644 --- a/lib/test_user_copy.c +++ b/lib/test_user_copy.c @@ -47,9 +47,25 @@ static bool is_zeroed(void *from, size_t size) static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size) { int ret = 0; - size_t start, end, i; - size_t zero_start = size / 4; - size_t zero_end = size - zero_start; + size_t start, end, i, zero_start, zero_end; + + if (test(size < 2 * PAGE_SIZE, "buffer too small")) + return -EINVAL; + + /* + * We want to cross a page boundary to exercise the code more + * effectively. We also don't want to make the size we scan too large, + * otherwise the test can take a long time and cause soft lockups. So + * scan a 1024 byte region across the page boundary. + */ + size = 1024; + start = PAGE_SIZE - (size / 2); + + kmem += start; + umem += start; + + zero_start = size / 4; + zero_end = size - zero_start; /* * We conduct a series of check_nonzero_user() tests on a block of -- cgit v1.2.3