[LTP] [PATCH v6] Refactor fork14 using new LTP API

[Andrea Cervesato]

Hi Petr,

On 5/15/24 16:22, Petr Vorel wrote:
> Hi Andrea,
>
>> Hi!
>> On 5/6/24 22:26, Petr Vorel wrote:
>>> Hi Andrea, Martin,
>>>> Hi,
>>>> Reviewed-by: Martin Doucha <mdoucha@suse.cz>
>>> +1
>>> ...
>>> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>>>>> +
>>>>> +static struct tst_test test = {
>>>>> +	.test_all = run,
>>>>> +	.setup = setup,
>>>>> +	.cleanup = cleanup,
>>>>> +	.forks_child = 1,
>>>>> +	.skip_in_compat = 1,
>>> BTW test on x86 (i.e. true 64 bit) behaves exactly the same as on compat mode:
>>> tst_test.c:1614: TINFO: Timeout per run is 0h 00m 30s
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:46: TINFO: mmap() failed
>>> fork14.c:49: TCONF: mmap() fails too many times, so it's almost impossible to get a vm_area_struct sized 16TB.
>>> IMHO we should whitelist it as well, I can change before merge with diff below.
>>> (More elegant way would be to add .skip_in_32bit.)
>>> Kind regards,
>>> Petr
>>> +++ testcases/kernel/syscalls/fork/fork14.c
>>> @@ -18,6 +18,8 @@
>>>    #include <stdlib.h>
>>>    #include <sys/wait.h>
>>> +#ifndef __i386__
>> TST_ABI32 is not enough? What about ".skip_in_compat"?
> Yes, TST_ABI32 is actually better than limit to __i386__.
> Do you give ack for these changes below? Or feel free to send new version?
Sure feel free to use TST_API32
> IMHO the problem is 16TB is too much for 32 bit kernel (4 GiB limit).
This is wanted. It's explained in the bug description
>
> .skip_in_compat would not be enough, because the problem is on pure 32 bit
> distro e.g. with 32bit kernel (yes, there are people who use them), but
> .skip_in_compat is for 64 bit kernel with 32bit userspace (compiled with -m32 in
> CFLAGS and LDFLAGS).
>
> Also, I suggested in the patch below to remove .skip_in_compat, but for info
> version it would be good to keep it. I suppose it's not worth to add new flag
> .skip_in_32bit for this single case (it'd be good for doc purposes).
> @Cyril WDYT?
>
> +++ testcases/kernel/syscalls/fork/fork14.c
> @@ -7,17 +7,21 @@
>   /*\
>    * [Description]
>    *
> - * This test is a reporducer for this patch:
> - * 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
> + * This test is a reproducer for kernel 3.5:
> + * 7edc8b0ac16c ("mm/fork: fix overflow in vma length when copying mmap on clone")
> + *
> + * 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 occurs, fork will incorrectly succeed. The patch above fixed it.
>    */
>   
> +#include "lapi/abisize.h"
>   #include "tst_test.h"
>   #include <stdlib.h>
>   #include <sys/wait.h>
>   
> +#ifndef TST_ABI32
> +
>   #define LARGE		(16 * 1024)
>   #define EXTENT		(16 * 1024 + 10)
>   
> @@ -48,7 +52,7 @@ static void run(void)
>   			if (failures > 10) {
>   				tst_brk(TCONF, "mmap() fails too many "
>   					"times, so it's almost impossible to "
> -					"get a vm_area_struct sized 16TB.");
> +					"get a vm_area_struct sized 16TB");
>   			}
>   
>   			continue;
> @@ -115,9 +119,11 @@ static struct tst_test test = {
>   	.setup = setup,
>   	.cleanup = cleanup,
>   	.forks_child = 1,
> -	.skip_in_compat = 1,
>   	.tags = (const struct tst_tag[]) {
>   		{"linux-git", "7edc8b0ac16c"},
>   		{}
>   	}
>   };
> +#else
> +TST_TEST_TCONF("Not supported on x86 in 32-bit");
> +#endif

Andrea