Signed-off-by: Sameeksha Sankpal sameekshasankpal@gmail.com --- tools/testing/selftests/seccomp/seccomp_bpf.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 14ba51b52095..d6a85d7b26da 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -4508,7 +4508,11 @@ static char get_proc_stat(struct __test_metadata *_metadata, pid_t pid)
snprintf(proc_path, sizeof(proc_path), "/proc/%d/stat", pid); ASSERT_EQ(get_nth(_metadata, proc_path, 3, &line), 1); - + int rc = get_nth(_metadata, proc_path, 3, &line); + if (rc != 1) { + printf("[ERROR] user_notification_fifo: failed to read stat for PID %d (rc=%d)\n", pid, rc); + } + ASSERT_EQ(rc, 1); status = *line; free(line);
@@ -4518,6 +4522,7 @@ static char get_proc_stat(struct __test_metadata *_metadata, pid_t pid) TEST(user_notification_fifo) { struct seccomp_notif_resp resp = {}; + ksft_print_msg("[INFO] Starting FIFO notification test\n"); struct seccomp_notif req = {}; int i, status, listener; pid_t pid, pids[3]; @@ -4535,6 +4540,7 @@ TEST(user_notification_fifo) listener = user_notif_syscall(__NR_getppid, SECCOMP_FILTER_FLAG_NEW_LISTENER); ASSERT_GE(listener, 0); + ksft_print_msg("[INFO] user_notification_fifo: listener PID is %d\n", listener);
pid = fork(); ASSERT_GE(pid, 0);
On Sat, May 17, 2025 at 05:57:40PM +0530, Sameeksha Sankpal wrote:
Signed-off-by: Sameeksha Sankpal sameekshasankpal@gmail.com
tools/testing/selftests/seccomp/seccomp_bpf.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 14ba51b52095..d6a85d7b26da 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -4508,7 +4508,11 @@ static char get_proc_stat(struct __test_metadata *_metadata, pid_t pid) snprintf(proc_path, sizeof(proc_path), "/proc/%d/stat", pid); ASSERT_EQ(get_nth(_metadata, proc_path, 3, &line), 1);
- int rc = get_nth(_metadata, proc_path, 3, &line);
if (rc != 1) {printf("[ERROR] user_notification_fifo: failed to read stat for PID %d (rc=%d)\n", pid, rc);}ASSERT_EQ(rc, 1);
An ASSERT will stop execution, so if it fails, you'll never reach the printf. And a "printf" shouldn't be used. What you want to use is TH_LOG, probably like this:
rc = get_nth(_metadata, proc_path, 3, &line); ASSERT_EQ(rc, 1) { TH_LOG("user_notification_fifo: failed to read stat for PID %d (rc=%d)", pid, rc); }
And please don't introduce new variables in the middle -- they need to be declared at the top of the function.
-Kees
Use TH_LOG to report failure when reading /proc/<pid>/stat in get_proc_stat(), following kernel test framework conventions.
Previously, printf() was used which is discouraged.
Suggested-by: Kees Cook kees@kernel.org
Signed-off-by: Sameeksha Sankpal sameekshasankpal@gmail.com --- v1 -> v2: - Used TH_LOG instead of printf for error logging - Moved variable declaration to the top of the function - Applied review suggestion by Kees Cook
tools/testing/selftests/seccomp/seccomp_bpf.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index d6a85d7b26da..0f12052ef1c7 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -4505,14 +4505,14 @@ static char get_proc_stat(struct __test_metadata *_metadata, pid_t pid) char proc_path[100] = {0}; char status; char *line; + int rc;
snprintf(proc_path, sizeof(proc_path), "/proc/%d/stat", pid); ASSERT_EQ(get_nth(_metadata, proc_path, 3, &line), 1); - int rc = get_nth(_metadata, proc_path, 3, &line); - if (rc != 1) { - printf("[ERROR] user_notification_fifo: failed to read stat for PID %d (rc=%d)\n", pid, rc); - } - ASSERT_EQ(rc, 1); + rc = get_nth(_metadata, proc_path, 3, &line); + ASSERT_EQ(rc, 1) { + TH_LOG("user_notification_fifo: failed to read stat for PID %d (rc=%d)", pid, rc); + } status = *line; free(line);
On Wed, May 28, 2025 at 06:38:39AM +0530, Sameeksha Sankpal wrote:
Use TH_LOG to report failure when reading /proc/<pid>/stat in get_proc_stat(), following kernel test framework conventions.
Previously, printf() was used which is discouraged.
printf wasn't used previous, that was in your v1. :)
Suggested-by: Kees Cook kees@kernel.org
No blank line here -- other tags should all be together with the S-o-b line.
Signed-off-by: Sameeksha Sankpal sameekshasankpal@gmail.com
v1 -> v2:
- Used TH_LOG instead of printf for error logging
- Moved variable declaration to the top of the function
- Applied review suggestion by Kees Cook
tools/testing/selftests/seccomp/seccomp_bpf.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index d6a85d7b26da..0f12052ef1c7 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -4505,14 +4505,14 @@ static char get_proc_stat(struct __test_metadata *_metadata, pid_t pid) char proc_path[100] = {0}; char status; char *line;
- int rc;
snprintf(proc_path, sizeof(proc_path), "/proc/%d/stat", pid); ASSERT_EQ(get_nth(_metadata, proc_path, 3, &line), 1);
- int rc = get_nth(_metadata, proc_path, 3, &line);
if (rc != 1) {printf("[ERROR] user_notification_fifo: failed to read stat for PID %d (rc=%d)\n", pid, rc);}ASSERT_EQ(rc, 1);
This patch is against your v1 patch -- it doesn't apply to the seccomp tree as-is. Please rebase your v2 off of the upstream tree rather than your v1.
- rc = get_nth(_metadata, proc_path, 3, &line);
- ASSERT_EQ(rc, 1) {
Indenting looks wrong here, double-check you're using tabs. (And please use scripts/checkpatch.pl to check your patch for common errors.)
TH_LOG("user_notification_fifo: failed to read stat for PID %d (rc=%d)", pid, rc);- } status = *line; free(line);
Code-wise, it looks good. Please respin for a v3 and this change should be good to land.
-Kees
Rebase the error logging enhancement for get_proc_stat() against the upstream seccomp tree with proper indentation formatting.
Suggested-by: Kees Cook kees@kernel.org Signed-off-by: Sameeksha Sankpal sameekshasankpal@gmail.com --- v1 -> v2: - Used TH_LOG instead of printf for error logging - Moved variable declaration to the top of the function - Applied review suggestion by Kees Cook
v2 -> v3: - Rebased against upstream seccomp tree (was previously against v1) - Fixed indentation to use tabs instead of spaces - Used scripts/checkpatch.pl to check the patch for common errors - Removed the blank line beforeS S-o-b added in v2
tools/testing/selftests/seccomp/seccomp_bpf.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 61acbd45ffaa..dbd7e705a2af 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -4508,9 +4508,14 @@ static char get_proc_stat(struct __test_metadata *_metadata, pid_t pid) char proc_path[100] = {0}; char status; char *line; + int rc;
snprintf(proc_path, sizeof(proc_path), "/proc/%d/stat", pid); ASSERT_EQ(get_nth(_metadata, proc_path, 3, &line), 1); + rc = get_nth(_metadata, proc_path, 3, &line); + ASSERT_EQ(rc, 1) { + TH_LOG("user_notification_fifo: failed to read stat for PID %d (rc=%d)", pid, rc); + }
status = *line; free(line);
linux-kselftest-mirror@lists.linaro.org
-
Kees Cook -
Sameeksha Sankpal