Re: hugetlb/hugeshmat: Add hugeshmat06 migrated from libhugetlbfs shm-perms

On 21/08/26 12:10 am, [email protected] wrote:
> Hi Samir,
>
> On Aug 20, 2026, Samir Mulani wrote:
>> hugetlb/hugeshmat: Add hugeshmat06 migrated from libhugetlbfs shm-perms
>> #define HPAGES_IN_SEG	4
>> #define MAX_CHILDREN	128
>>
>> 	iterations = MIN((long)tst_hugepages, (long)MAX_CHILDREN);
> Could the iteration count retain the original test's exhaustion guarantee?
> Each leaked attach reserves HPAGES_IN_SEG pages, so a pool containing N
> hugepages requires more than N / HPAGES_IN_SEG attaches to expose the leak.
> The original test calculated that value plus one.
>
> With this cap, one execution can pass without detecting the bug when the pool
> contains 512 or more hugepages. Even the "-i 5" runtest entry is insufficient
> at 2560 or more. Could the count instead be derived from the pool and segment
> sizes, including the final attach that exceeds the pool?
Fixed. Replaced MIN(tst_hugepages, MAX_CHILDREN) with tst_hugepages / 
HPAGES_IN_SEG + 1 and removed MAX_CHILDREN. Now the count is derived 
from the pool size and segment size, and includes the final attach that 
must fail on a buggy kernel.
>
>> 			if (shmaddr == (void *)-1) {
>> 				tst_res(TFAIL | TERRNO,
>> 					"Child %d: shmat() failed, hugepage reservation pool may be exhausted",
>> 					getpid());
>> 				exit(EXIT_FAILURE);
>> 			}
> Could the child exit successfully after reporting TFAIL?
> tst_reap_children() converts this nonzero status into an additional TBROK
> ("Invalid child exit value") and aborts the parent. LTP already propagates the
> child's tst_res() result, so the regression should remain a TFAIL rather than
> also becoming a broken-test result.
Fixed. Changed both exit(EXIT_FAILURE) to exit(EXIT_SUCCESS) — once in 
the shmat() failure path and once in the pattern-mismatch path. Also 
updated the comment above tst_reap_children() which was incorrectly 
describing the behavior.
>> static struct tst_test test = {
>> 	.needs_root	 = 1,
> This is described as a regression test, but the test has no .tags entry.
> Could the corresponding linux-git regression fix tag be added?
Fixed. The regression was fixed by upstream commit f83a275dbc5c ("mm: 
account for MAP_SHARED mappings using VM_MAYSHARE and not VM_SHARED in 
hugetlbfs" by Mel Gorman). Added the .tags entry with the 12-character 
hash, consistent with the other tests in the same directory.
>> +/hugetlb/hugeshmat/hugeshmat06
> Could this binary instead be ignored by
> testcases/kernel/mem/hugetlb/hugeshmat/.gitignore as /hugeshmat06? New test
> binaries should use a leaf-directory .gitignore rather than the shared
> testcases/kernel/mem/.gitignore.
This change is not required.
>> /*\
>>   * Tests shared memory behavior when multiple processes attach to a
>>   * hugepage-backed segment with different permissions.
> Could the exported description explain why root is required? The test sets
> .needs_root because the framework adjusts the hugepage pool and mounts
> hugetlbfs.
>
>>   * pool.  This is detected by using raw shmat() in each child so that a
> Could shmat() use the ":manpage:`shmat(2)`" role here? Syscalls in exported
> test descriptions should use their man-page role.
Fixed both. Added a sentence in the exported description explaining that 
root is required because the LTP framework adjusts the hugepage pool and 
mounts hugetlbfs. Also changed shmat() to :manpage:`shmat(2)` in the 
same block.
> Verdict - Needs revision
>
> ---
> Note:
>
> The agent can sometimes produce false positives although often its
> findings are genuine. If you find issues with the review, please
> comment this email or ignore the suggestions.
>
> Regards,
> LTP AI Reviewer

Regards,

Samir


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp