From: Andrea Cervesato via ltp <ltp@lists.linux.it>
To: Martin Doucha <mdoucha@suse.cz>,
Andrea Cervesato <andrea.cervesato@suse.de>,
ltp@lists.linux.it
Subject: Re: [LTP] [PATCH v5] Refactor fork14 using new LTP API
Date: Fri, 3 May 2024 10:08:07 +0200 [thread overview]
Message-ID: <d575106b-6550-4498-b178-be126c6ef44d@suse.com> (raw)
In-Reply-To: <b4d53d0b-5b13-494e-bd01-18bc52af3069@suse.cz>
Hi Martin,
thanks for the review. I'm going to fix it
On 5/2/24 15:20, Martin Doucha wrote:
> Hi,
> there's a stale pointer issue in memvec if you run the test with
> multiple iterations. See below.
>
> On 22. 03. 24 10:04, Andrea Cervesato wrote:
>> From: Andrea Cervesato <andrea.cervesato@suse.com>
>>
>> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
>> ---
>> .skip_in_compat usage
>> Fixed typos
>> Fixed license
>>
>> testcases/kernel/syscalls/fork/fork14.c | 205 +++++++++++-------------
>> 1 file changed, 92 insertions(+), 113 deletions(-)
>>
>> diff --git a/testcases/kernel/syscalls/fork/fork14.c
>> b/testcases/kernel/syscalls/fork/fork14.c
>> index 93af2ebac..421f2caa3 100644
>> --- a/testcases/kernel/syscalls/fork/fork14.c
>> +++ b/testcases/kernel/syscalls/fork/fork14.c
>> @@ -1,143 +1,122 @@
>> -/*********************************************************************
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> * Copyright (C) 2014 Red Hat, Inc.
>> - *
>> - * This program is free software; you can redistribute it and/or
>> - * modify it under the terms of version 2 of the GNU General Public
>> - * License as published by the Free Software Foundation.
>> - *
>> - * This program is distributed in the hope that it would be useful,
>> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>> - *
>> - * Further, this software is distributed without any warranty that it
>> - * is free of the rightful claim of any third person regarding
>> - * infringement or the like. Any license provided herein, whether
>> - * implied or otherwise, applies only to this software file. Patent
>> - * licenses, if any, provided herein do not apply to combinations of
>> - * this program with other software, or any other product whatsoever.
>> - *
>> - * You should have received a copy of the GNU General Public License
>> - * along with this program; if not, write the Free Software
>> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>> - * 02110-1301, USA.
>> + * Copyright (C) 2023 SUSE LLC Andrea Cervesato
>> <andrea.cervesato@suse.com>
>> + */
>> +
>> +/*\
>> + * [Description]
>> *
>> * This test is a reporducer for this patch:
>> - * https://lkml.org/lkml/2012/4/24/328
>> + *
>> https://lore.kernel.org/lkml/1335289853-2923-1-git-send-email-siddhesh.poyarekar@gmail.com/
>> * Since vma length in dup_mmap is calculated and stored in a unsigned
>> * int, it will overflow when length of mmaped memory > 16 TB. When
>> - * overflow occur, fork will incorrectly succeed. The patch above
>> - * fixed it.
>> - ********************************************************************/
>> + * overflow occurs, fork will incorrectly succeed. The patch above
>> fixed it.
>> + */
>> -#include <sys/mman.h>
>> +#include "tst_test.h"
>> +#include <stdlib.h>
>> #include <sys/wait.h>
>> -#include <stdio.h>
>> -#include <unistd.h>
>> -#include "test.h"
>> -#include "safe_macros.h"
>> -#include "lapi/abisize.h"
>> -
>> -char *TCID = "fork14";
>> -int TST_TOTAL = 1;
>> -#define GB (1024 * 1024 * 1024L)
>> -
>> -/* set mmap threshold to 16TB */
>> #define LARGE (16 * 1024)
>> #define EXTENT (16 * 1024 + 10)
>> -static char **pointer_vec;
>> -
>> -static void setup(void);
>> -static void cleanup(void);
>> -static int fork_test(void);
>> +static char **memvec;
>> -int main(int ac, char **av)
>> +static void run(void)
>> {
>> - int lc, reproduced;
>> + int i, j, ret;
>> + pid_t pid;
>> + void *mem;
>> + int prev_failed = 0;
>> + int passed = 1;
>> + int failures = 0;
>> - tst_parse_opts(ac, av, NULL, NULL);
>> -/*
>> - * Tested on ppc64/x86_64/i386/s390x. And only 64bit has this issue.
>> - * Since a 32bit program can't mmap so many memory.
>> - */
>> -#ifdef TST_ABI32
>> - tst_brkm(TCONF, NULL, "This test is only for 64bit.");
>> -#endif
>> - setup();
>> - for (lc = 0; TEST_LOOPING(lc); lc++) {
>> - tst_count = 0;
>> -
>> - reproduced = fork_test();
>> - if (reproduced == 0)
>> - tst_resm(TPASS, "fork failed as expected.");
>> - }
>> - cleanup();
>> - tst_exit();
>> -}
>> + for (i = 0; i < EXTENT; i++) {
>> + mem = mmap(NULL, 1 * TST_GB,
>> + PROT_READ | PROT_WRITE,
>> + MAP_PRIVATE | MAP_ANONYMOUS,
>> + 0, 0);
>> -static void setup(void)
>> -{
>> - tst_sig(FORK, DEF_HANDLER, cleanup);
>> - TEST_PAUSE;
>> + if (mem == MAP_FAILED) {
>> + failures++;
>> - pointer_vec = SAFE_MALLOC(cleanup, EXTENT * sizeof(char *));
>> -}
>> + tst_res(TINFO, "mmap() failed");
>> -static void cleanup(void)
>> -{
>> - free(pointer_vec);
>> -}
>> + if (failures > 10) {
>> + tst_brk(TCONF, "mmap() fails too many "
>> + "times, so it's almost impossible to "
>> + "get a vm_area_struct sized 16TB.");
>> + }
>> -static int fork_test(void)
>> -{
>> - int i, j, prev_failed = 0, fails = 0, cnt = 0;
>> - int reproduced = 0;
>> - void *addr;
>> + continue;
>> + }
>> - for (i = 0; i < EXTENT; i++) {
>> - addr = mmap(NULL, 1 * GB, PROT_READ | PROT_WRITE,
>> - MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
>> - if (addr == MAP_FAILED) {
>> - pointer_vec[i] = NULL;
>> - fails++;
>> - /*
>> - * EXTENT is "16*1024+10", if fails count exceeds 10,
>> - * we are almost impossible to get an vm_area_struct
>> - * sized 16TB
>> + memvec[i] = mem;
>> +
>> + pid = fork();
>> +
>> + if (pid == -1) {
>> + /* keep track of the failed fork() and verify that next one
>> + * is failing as well.
>> */
>> - if (fails == 11) {
>> - tst_brkm(TCONF, cleanup, "mmap() fails too many"
>> - "times, so we are almost impossible to"
>> - " get an vm_area_struct sized 16TB.");
>> - }
>> - } else {
>> - pointer_vec[i] = addr;
>> + prev_failed = 1;
>> + continue;
>> }
>> - cnt++;
>> - switch (tst_fork()) {
>> - case -1:
>> - prev_failed = 1;
>> - break;
>> - case 0:
>> + if (!pid)
>> exit(0);
>> - default:
>> - SAFE_WAITPID(cleanup, -1, NULL, 0);
>> - if (prev_failed > 0 && i >= LARGE) {
>> - tst_resm(TFAIL, "Fork succeeds incorrectly");
>> - reproduced = 1;
>> - goto clear_memory_map;
>> - }
>> + ret = waitpid(pid, NULL, 0);
>> + if (ret == -1 && errno != ECHILD)
>> + tst_brk(TBROK | TERRNO, "waitpid() error");
>> +
>> + if (prev_failed && i >= LARGE) {
>> + passed = 0;
>> + break;
>> }
>> +
>> + prev_failed = 0;
>> +
>> + tst_res(TINFO, "fork() passed at %d attempt", i);
>> }
>> -clear_memory_map:
>> - for (j = 0; j < cnt; j++) {
>> - if (pointer_vec[j])
>> - SAFE_MUNMAP(cleanup, pointer_vec[j], 1 * GB);
>> + for (j = 0; j < i; j++) {
>> + if (memvec[j])
>> + SAFE_MUNMAP(memvec[j], 1 * TST_GB);
>
> The memvec array never gets cleared after setup() so if you run the
> test with multiple iterations (e.g. -i 5), non-NULL pointers will
> accumulate in memvec between test iterations and you may end up
> unmapping an address range that was reused by libc.
>
>> }
>> - return reproduced;
>> + if (passed)
>> + tst_res(TPASS, "fork() failed as expected");
>> + else
>> + tst_res(TFAIL, "fork() succeeded incorrectly");
>> }
>> +
>> +static void setup(void)
>> +{
>> + memvec = SAFE_MALLOC(EXTENT * sizeof(char *));
>> + memset(memvec, 0, EXTENT);
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> + for (long i = 0; i < EXTENT; i++) {
>> + if (memvec && memvec[i])
>> + SAFE_MUNMAP(memvec[i], 1 * TST_GB);
>> + }
>> +
>> + if (memvec)
>> + free(memvec);
>> +}
>> +
>> +static struct tst_test test = {
>> + .test_all = run,
>> + .setup = setup,
>> + .cleanup = cleanup,
>> + .forks_child = 1,
>> + .skip_in_compat = 1,
>> + .tags = (const struct tst_tag[]) {
>> + {"linux-git", "7edc8b0ac16c"},
>> + {}
>> + }
>> +};
>
Andrea
--
Mailing list info: https://lists.linux.it/listinfo/ltp
prev parent reply other threads:[~2024-05-03 8:08 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-22 9:04 [LTP] [PATCH v5] Refactor fork14 using new LTP API Andrea Cervesato
2024-03-22 10:05 ` Petr Vorel
2024-05-02 13:20 ` Martin Doucha
2024-05-03 8:08 ` Andrea Cervesato via ltp [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d575106b-6550-4498-b178-be126c6ef44d@suse.com \
--to=ltp@lists.linux.it \
--cc=andrea.cervesato@suse.com \
--cc=andrea.cervesato@suse.de \
--cc=mdoucha@suse.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).