Re: [LTP] [PATCH v2] ioctl10.c: New case test PROCMAP_QUERY ioctl()

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
I've pushed the patch with following changes:

- removed .needs_root since the test runs fine without root

- moved the check for ioctl() support into the test setup() and the
  check for availability is only done on kernels older than 6.11 since
  it has to be available since that kernel version

- made use of guarded buffers for the procmap_query structure passed to
  the syscalls

What should still be done:

- we miss a few more errnos:
  - EINVAL if q->size is too small
  - E2BIG if q->size is larger than page size
  - EINVAL on invalid q->flags
  - EINVAL if only one of q->vma_name_size and q->qma_name_addr is set
  - EINVAL if only one of q->build_id_size and q->build_id_addr is set
  - ENAMETOOLONG if build_id_size or name_buf_size is too small
  - possibly ESRCH if we attempt to get memory map of a process that did exit and was waited for

- the invalid tests should be split into a separate test and stored in
  a tcase structure as we usually do, which makes it easier to add tests

Full diff:

diff --git a/testcases/kernel/syscalls/ioctl/ioctl10.c b/testcases/kernel/syscalls/ioctl/ioctl10.c
index 7ab3e4c43..6af2bb1f2 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl10.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl10.c
@@ -41,6 +41,9 @@ struct map_entry {
 	unsigned int vm_flags;
 };
 
+struct procmap_query *q;
+static int fd = -1;
+
 static unsigned int parse_vm_flags(const char *vm_flags_str)
 {
 	unsigned int flags = 0;
@@ -85,93 +88,108 @@ static void parse_maps_file(const char *filename, const char *keyword, struct ma
 
 static void verify_ioctl(void)
 {
-	struct procmap_query q;
-	int fd;
 	struct map_entry entry;
 
 	memset(&entry, 0, sizeof(entry));
 
-	fd = SAFE_OPEN("/proc/self/maps", O_RDONLY);
-
 	parse_maps_file(PROC_MAP_PATH, "*", &entry);
 
 	/* CASE 1: exact MATCH at query_addr */
-	memset(&q, 0, sizeof(q));
-	q.size = sizeof(q);
-	q.query_addr = (uint64_t)entry.vm_start;
-	q.query_flags = 0;
-
-	TEST(ioctl(fd, PROCMAP_QUERY, &q));
-
-	if ((TST_RET == -1) && (TST_ERR == ENOTTY))
-		tst_brk(TCONF,
-			"This system does not provide support for ioctl(PROCMAP_QUERY)");
-
-
-	TST_EXP_PASS(ioctl(fd, PROCMAP_QUERY, &q));
-
-	TST_EXP_EQ_LU(q.query_addr, entry.vm_start);
-	TST_EXP_EQ_LU(q.query_flags, 0);
-	TST_EXP_EQ_LU(q.vma_flags, entry.vm_flags);
-	TST_EXP_EQ_LU(q.vma_start, entry.vm_start);
-	TST_EXP_EQ_LU(q.vma_end, entry.vm_end);
-	TST_EXP_EQ_LU(q.vma_page_size, getpagesize());
-	TST_EXP_EQ_LU(q.vma_offset, entry.vm_pgoff);
-	TST_EXP_EQ_LU(q.inode, entry.vm_inode);
-	TST_EXP_EQ_LU(q.dev_major, entry.vm_major);
-	TST_EXP_EQ_LU(q.dev_minor, entry.vm_minor);
+	memset(q, 0, sizeof(*q));
+	q->size = sizeof(*q);
+	q->query_addr = (uint64_t)entry.vm_start;
+	q->query_flags = 0;
+
+	TST_EXP_PASS(ioctl(fd, PROCMAP_QUERY, q));
+
+	TST_EXP_EQ_LU(q->query_addr, entry.vm_start);
+	TST_EXP_EQ_LU(q->query_flags, 0);
+	TST_EXP_EQ_LU(q->vma_flags, entry.vm_flags);
+	TST_EXP_EQ_LU(q->vma_start, entry.vm_start);
+	TST_EXP_EQ_LU(q->vma_end, entry.vm_end);
+	TST_EXP_EQ_LU(q->vma_page_size, getpagesize());
+	TST_EXP_EQ_LU(q->vma_offset, entry.vm_pgoff);
+	TST_EXP_EQ_LU(q->inode, entry.vm_inode);
+	TST_EXP_EQ_LU(q->dev_major, entry.vm_major);
+	TST_EXP_EQ_LU(q->dev_minor, entry.vm_minor);
 
 	/* CASE 2: NO MATCH at query_addr */
-	memset(&q, 0, sizeof(q));
-	q.size = sizeof(q);
-	q.query_addr = entry.vm_start - 1;
-	q.query_flags = 0;
+	memset(q, 0, sizeof(*q));
+	q->size = sizeof(*q);
+	q->query_addr = entry.vm_start - 1;
+	q->query_flags = 0;
 
-	TST_EXP_FAIL(ioctl(fd, PROCMAP_QUERY, &q), ENOENT);
+	TST_EXP_FAIL(ioctl(fd, PROCMAP_QUERY, q), ENOENT);
 
 	/* CASE 3: MATCH COVERING_OR_NEXT_VMA */
-	memset(&q, 0, sizeof(q));
-	q.size = sizeof(q);
-	q.query_addr = entry.vm_start - 1;
-	q.query_flags = PROCMAP_QUERY_COVERING_OR_NEXT_VMA;
+	memset(q, 0, sizeof(*q));
+	q->size = sizeof(*q);
+	q->query_addr = entry.vm_start - 1;
+	q->query_flags = PROCMAP_QUERY_COVERING_OR_NEXT_VMA;
 
-	TST_EXP_PASS(ioctl(fd, PROCMAP_QUERY, &q));
+	TST_EXP_PASS(ioctl(fd, PROCMAP_QUERY, q));
 
 	/* CASE 4: NO MATCH WRITABLE at query_addr */
 	memset(&entry, 0, sizeof(entry));
 	parse_maps_file(PROC_MAP_PATH, "*r-?p *", &entry);
 
-	memset(&q, 0, sizeof(q));
-	q.size = sizeof(q);
-	q.query_addr = entry.vm_start;
-	q.query_flags = PROCMAP_QUERY_VMA_WRITABLE;
-	TST_EXP_FAIL(ioctl(fd, PROCMAP_QUERY, &q), ENOENT);
+	memset(q, 0, sizeof(*q));
+	q->size = sizeof(*q);
+	q->query_addr = entry.vm_start;
+	q->query_flags = PROCMAP_QUERY_VMA_WRITABLE;
+	TST_EXP_FAIL(ioctl(fd, PROCMAP_QUERY, q), ENOENT);
 
 	/* CASE 5: check vma_name_addr content */
 	char process_name[256];
-	char pattern[256];
+	char pattern[258];
 	char buf[256];
 
 	SAFE_READLINK("/proc/self/exe", process_name, sizeof(process_name));
-	sprintf(pattern, "*%s*", process_name);
+	snprintf(pattern, sizeof(pattern), "*%s*", process_name);
 	memset(&entry, 0, sizeof(entry));
 	parse_maps_file(PROC_MAP_PATH, pattern, &entry);
 
-	memset(&q, 0, sizeof(q));
-	q.size = sizeof(q);
-	q.query_addr = entry.vm_start;
-	q.query_flags = 0;
-	q.vma_name_addr = (uint64_t)(unsigned long)buf;
-	q.vma_name_size = sizeof(buf);
+	memset(q, 0, sizeof(*q));
+	q->size = sizeof(*q);
+	q->query_addr = entry.vm_start;
+	q->query_flags = 0;
+	q->vma_name_addr = (uint64_t)(unsigned long)buf;
+	q->vma_name_size = sizeof(buf);
 
-	TST_EXP_PASS(ioctl(fd, PROCMAP_QUERY, &q));
-	TST_EXP_EQ_LU(q.vma_name_size, strlen(process_name) + 1);
-	TST_EXP_EQ_STR((char *)q.vma_name_addr, process_name);
+	TST_EXP_PASS(ioctl(fd, PROCMAP_QUERY, q));
+	TST_EXP_EQ_LU(q->vma_name_size, strlen(process_name) + 1);
+	TST_EXP_EQ_STR((char *)q->vma_name_addr, process_name);
 
 	SAFE_CLOSE(fd);
 }
 
+static void setup(void)
+{
+	struct procmap_query q = {};
+
+	fd = SAFE_OPEN(PROC_MAP_PATH, O_RDONLY);
+
+	if (tst_kvercmp(6, 11, 0) < 0) {
+		TEST(ioctl(fd, PROCMAP_QUERY, q));
+
+		if ((TST_RET == -1) && (TST_ERR == ENOTTY))
+			tst_brk(TCONF,
+				"This system does not provide support for ioctl(PROCMAP_QUERY)");
+	}
+}
+
+static void cleanup(void)
+{
+	if (fd != -1)
+		SAFE_CLOSE(fd);
+}
+
 static struct tst_test test = {
+	.setup = setup,
+	.cleanup = cleanup,
 	.test_all = verify_ioctl,
-	.needs_root = 1,
+	.bufs = (struct tst_buffers []) {
+		{&q, .size = sizeof(*q)},
+		{}
+	}
 };

-- 
Cyril Hrubis
chrubis@suse.cz

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