From: R Nageswara Sastry <rnsastry@linux.ibm.com>
To: Christian Brauner <brauner@kernel.org>, linux-fsdevel@vger.kernel.org
Cc: Jacob Lalonde <jalalonde@meta.com>,
Josef Bacik <josef@toxicpanda.com>, Jann Horn <jannh@google.com>,
Alexander Viro <viro@zeniv.linux.org.uk>, Jan Kara <jack@suse.cz>,
Andrew Morton <akpm@linux-foundation.org>,
David Hildenbrand <david@kernel.org>,
Lorenzo Stoakes <ljs@kernel.org>,
"Liam R. Howlett" <liam@infradead.org>,
Vlastimil Babka <vbabka@kernel.org>,
Mike Rapoport <rppt@kernel.org>,
Suren Baghdasaryan <surenb@google.com>,
Michal Hocko <mhocko@suse.com>,
Omar Sandoval <osandov@osandov.com>,
Jacob Lalonde <jalalonde@fb.com>, Shuah Khan <shuah@kernel.org>,
linux-kernel@vger.kernel.org, linux-mm@kvack.org,
linux-kselftest@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v2 05/22] selftests/coredump: collapse the expected request check into the helper
Date: Wed, 9 Sep 2026 21:24:44 +0530 [thread overview]
Message-ID: <233bec97-49d2-4871-b9a2-150586aead35@linux.ibm.com> (raw)
In-Reply-To: <20260820-work-coredump-sparse-v2-5-ba32dd718c51@kernel.org>
On 20.08.2026 4:39 AM, Christian Brauner wrote:
> All nine callers spell the expected flags. So every new feature bit the
> kernel learns has to be cargo culted.
>
> The callers also all pass COREDUMP_ACK_SIZE_VER0 as the minimum request
> size although what is being validated is coredump_req->size. And
> read_coredump_req() makes the same mixup twice more.
>
> Clean this all up.
>
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
Tested-by: R Nageswara Sastry <rnsastry@linux.ibm.com>
System: ppc64le LPAR (IBM POWER), Linux 7.3-rc2
> ---
> .../coredump/coredump_socket_protocol_test.c | 36 ++++++----------------
> tools/testing/selftests/coredump/coredump_test.h | 3 +-
> .../selftests/coredump/coredump_test_helpers.c | 35 +++++++++++++--------
> 3 files changed, 32 insertions(+), 42 deletions(-)
>
> diff --git a/tools/testing/selftests/coredump/coredump_socket_protocol_test.c b/tools/testing/selftests/coredump/coredump_socket_protocol_test.c
> index d9fa6239b5a9..60a357e628eb 100644
> --- a/tools/testing/selftests/coredump/coredump_socket_protocol_test.c
> +++ b/tools/testing/selftests/coredump/coredump_socket_protocol_test.c
> @@ -151,9 +151,7 @@ TEST_F(coredump, socket_request_kernel)
> goto out;
> }
>
> - if (!check_coredump_req(&req, COREDUMP_ACK_SIZE_VER0,
> - COREDUMP_KERNEL | COREDUMP_USERSPACE |
> - COREDUMP_REJECT | COREDUMP_WAIT)) {
> + if (!check_coredump_req(&req)) {
> fprintf(stderr, "socket_request_kernel: check_coredump_req failed\n");
> goto out;
> }
> @@ -301,9 +299,7 @@ TEST_F(coredump, socket_request_userspace)
> goto out;
> }
>
> - if (!check_coredump_req(&req, COREDUMP_ACK_SIZE_VER0,
> - COREDUMP_KERNEL | COREDUMP_USERSPACE |
> - COREDUMP_REJECT | COREDUMP_WAIT)) {
> + if (!check_coredump_req(&req)) {
> fprintf(stderr, "socket_request_userspace: check_coredump_req failed\n");
> goto out;
> }
> @@ -441,9 +437,7 @@ TEST_F(coredump, socket_request_reject)
> goto out;
> }
>
> - if (!check_coredump_req(&req, COREDUMP_ACK_SIZE_VER0,
> - COREDUMP_KERNEL | COREDUMP_USERSPACE |
> - COREDUMP_REJECT | COREDUMP_WAIT)) {
> + if (!check_coredump_req(&req)) {
> fprintf(stderr, "socket_request_reject: check_coredump_req failed\n");
> goto out;
> }
> @@ -581,9 +575,7 @@ TEST_F(coredump, socket_request_invalid_flag_combination)
> goto out;
> }
>
> - if (!check_coredump_req(&req, COREDUMP_ACK_SIZE_VER0,
> - COREDUMP_KERNEL | COREDUMP_USERSPACE |
> - COREDUMP_REJECT | COREDUMP_WAIT)) {
> + if (!check_coredump_req(&req)) {
> fprintf(stderr, "socket_request_invalid_flag_combination: check_coredump_req failed\n");
> goto out;
> }
> @@ -702,9 +694,7 @@ TEST_F(coredump, socket_request_unknown_flag)
> goto out;
> }
>
> - if (!check_coredump_req(&req, COREDUMP_ACK_SIZE_VER0,
> - COREDUMP_KERNEL | COREDUMP_USERSPACE |
> - COREDUMP_REJECT | COREDUMP_WAIT)) {
> + if (!check_coredump_req(&req)) {
> fprintf(stderr, "socket_request_unknown_flag: check_coredump_req failed\n");
> goto out;
> }
> @@ -822,9 +812,7 @@ TEST_F(coredump, socket_request_invalid_size_small)
> goto out;
> }
>
> - if (!check_coredump_req(&req, COREDUMP_ACK_SIZE_VER0,
> - COREDUMP_KERNEL | COREDUMP_USERSPACE |
> - COREDUMP_REJECT | COREDUMP_WAIT)) {
> + if (!check_coredump_req(&req)) {
> fprintf(stderr, "socket_request_invalid_size_small: check_coredump_req failed\n");
> goto out;
> }
> @@ -944,9 +932,7 @@ TEST_F(coredump, socket_request_invalid_size_large)
> goto out;
> }
>
> - if (!check_coredump_req(&req, COREDUMP_ACK_SIZE_VER0,
> - COREDUMP_KERNEL | COREDUMP_USERSPACE |
> - COREDUMP_REJECT | COREDUMP_WAIT)) {
> + if (!check_coredump_req(&req)) {
> fprintf(stderr, "socket_request_invalid_size_large: check_coredump_req failed\n");
> goto out;
> }
> @@ -1355,9 +1341,7 @@ TEST_F_TIMEOUT(coredump, socket_multiple_crashing_coredumps, 500)
> goto out;
> }
>
> - if (!check_coredump_req(&req, COREDUMP_ACK_SIZE_VER0,
> - COREDUMP_KERNEL | COREDUMP_USERSPACE |
> - COREDUMP_REJECT | COREDUMP_WAIT)) {
> + if (!check_coredump_req(&req)) {
> fprintf(stderr, "check_coredump_req failed for fd %d\n", fd_coredump);
> goto out;
> }
> @@ -1509,9 +1493,7 @@ TEST_F_TIMEOUT(coredump, socket_multiple_crashing_coredumps_epoll_workers, 500)
> fprintf(stderr, "socket_multiple_crashing_coredumps_epoll_workers: read_coredump_req failed\n");
> goto out;
> }
> - if (!check_coredump_req(&req, COREDUMP_ACK_SIZE_VER0,
> - COREDUMP_KERNEL | COREDUMP_USERSPACE |
> - COREDUMP_REJECT | COREDUMP_WAIT)) {
> + if (!check_coredump_req(&req)) {
> fprintf(stderr, "socket_multiple_crashing_coredumps_epoll_workers: check_coredump_req failed\n");
> goto out;
> }
> diff --git a/tools/testing/selftests/coredump/coredump_test.h b/tools/testing/selftests/coredump/coredump_test.h
> index ed47f01fa53c..a02809145e2d 100644
> --- a/tools/testing/selftests/coredump/coredump_test.h
> +++ b/tools/testing/selftests/coredump/coredump_test.h
> @@ -51,8 +51,7 @@ bool read_marker(int fd, enum coredump_mark mark);
> bool read_coredump_req(int fd, struct coredump_req *req);
> bool send_coredump_ack(int fd, const struct coredump_req *req,
> __u64 mask, size_t size_ack);
> -bool check_coredump_req(const struct coredump_req *req, size_t min_size,
> - __u64 required_mask);
> +bool check_coredump_req(const struct coredump_req *req);
> int open_coredump_tmpfile(int fd_tmpfs_detached);
> void process_coredump_worker(int fd_coredump, int fd_peer_pidfd, int fd_core_file);
>
> diff --git a/tools/testing/selftests/coredump/coredump_test_helpers.c b/tools/testing/selftests/coredump/coredump_test_helpers.c
> index 524fa5370593..306711e1b24d 100644
> --- a/tools/testing/selftests/coredump/coredump_test_helpers.c
> +++ b/tools/testing/selftests/coredump/coredump_test_helpers.c
> @@ -200,7 +200,7 @@ bool read_marker(int fd, enum coredump_mark mark)
> bool read_coredump_req(int fd, struct coredump_req *req)
> {
> ssize_t ret;
> - size_t field_size, user_size, ack_size, kernel_size, remaining_size;
> + size_t field_size, user_size, known_size, kernel_size, remaining_size;
>
> memset(req, 0, sizeof(*req));
> field_size = sizeof(req->size);
> @@ -214,9 +214,9 @@ bool read_coredump_req(int fd, struct coredump_req *req)
> }
> kernel_size = req->size;
>
> - if (kernel_size < COREDUMP_ACK_SIZE_VER0) {
> + if (kernel_size < COREDUMP_REQ_SIZE_VER0) {
> fprintf(stderr, "read_coredump_req: kernel_size %zu < min %d\n",
> - kernel_size, COREDUMP_ACK_SIZE_VER0);
> + kernel_size, COREDUMP_REQ_SIZE_VER0);
> return false;
> }
> if (kernel_size >= PAGE_SIZE) {
> @@ -225,11 +225,11 @@ bool read_coredump_req(int fd, struct coredump_req *req)
> return false;
> }
>
> - /* Use the minimum of user and kernel size to read the full request. */
> + /* Consume as much of the request as we know about. */
> user_size = sizeof(struct coredump_req);
> - ack_size = user_size < kernel_size ? user_size : kernel_size;
> - ret = recv(fd, req, ack_size, MSG_WAITALL);
> - if (ret != ack_size)
> + known_size = user_size < kernel_size ? user_size : kernel_size;
> + ret = recv(fd, req, known_size, MSG_WAITALL);
> + if (ret != known_size)
> return false;
>
> fprintf(stderr, "Read coredump request with size %u and mask 0x%llx\n",
> @@ -287,15 +287,24 @@ bool send_coredump_ack(int fd, const struct coredump_req *req,
> return true;
> }
>
> -bool check_coredump_req(const struct coredump_req *req, size_t min_size,
> - __u64 required_mask)
> +/* Every option the kernel is expected to advertise in coredump_req->mask. */
> +#define TEST_REQ_MASK_ALL \
> + (COREDUMP_KERNEL | COREDUMP_USERSPACE | \
> + COREDUMP_REJECT | COREDUMP_WAIT)
> +
> +bool check_coredump_req(const struct coredump_req *req)
> {
> - if (req->size < min_size)
> - return false;
> - if ((req->mask & required_mask) != required_mask)
> + if (req->size < COREDUMP_REQ_SIZE_VER0) {
> + fprintf(stderr, "%s: size %u below minimum %d\n",
> + __func__, req->size, COREDUMP_REQ_SIZE_VER0);
> return false;
> - if (req->mask & ~required_mask)
> + }
> + if (req->mask != TEST_REQ_MASK_ALL) {
> + fprintf(stderr, "%s: mask 0x%llx, expected 0x%llx\n",
> + __func__, (unsigned long long)req->mask,
> + (unsigned long long)TEST_REQ_MASK_ALL);
> return false;
> + }
> return true;
> }
>
>
--
Thanks and Regards
R.Nageswara Sastry
next prev parent reply other threads:[~2026-09-09 15:55 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-08-19 23:09 [PATCH v2 00/22] coredump: allow to create sparse coredumps on the coredump socket Christian Brauner
2026-08-19 23:09 ` [PATCH v2 01/22] powerpc/spufs: don't dump more than the note supports Christian Brauner
2026-09-09 15:52 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 02/22] coredump: refuse negative skips Christian Brauner
2026-09-09 15:53 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 03/22] coredump: set the minimum send buffer size Christian Brauner
2026-09-09 15:53 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 04/22] selftests/coredump: discard the right amount after the coredump request Christian Brauner
2026-09-09 15:54 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 05/22] selftests/coredump: collapse the expected request check into the helper Christian Brauner
2026-09-09 15:54 ` R Nageswara Sastry [this message]
2026-08-19 23:09 ` [PATCH v2 06/22] selftests/coredump: add a separate helper header Christian Brauner
2026-09-09 15:55 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 07/22] coredump: pin the protocol struct sizes Christian Brauner
2026-09-09 15:55 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 08/22] coredump: move the negotiated mask into struct coredump_params Christian Brauner
2026-09-09 15:56 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 09/22] coredump: deduplicate the to_skip flush Christian Brauner
2026-09-09 15:56 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 10/22] coredump: make the dump helper return bool Christian Brauner
2026-09-09 15:59 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 11/22] coredump: always chunk writes Christian Brauner
2026-09-09 16:00 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 12/22] coredump: clean up coredump state handling Christian Brauner
2026-09-09 16:01 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 13/22] coredump: add COREDUMP_RECORDS to the coredump socket protocol Christian Brauner
2026-09-09 16:02 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 14/22] coredump: add COREDUMP_SPARSE " Christian Brauner
2026-09-09 16:02 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 15/22] tools: sync coredump.h header Christian Brauner
2026-09-09 16:03 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 16/22] coredump: send the coredump in records if requested Christian Brauner
2026-09-09 16:04 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 17/22] coredump: describe the holes when COREDUMP_SPARSE is negotiated Christian Brauner
2026-09-09 16:04 ` R Nageswara Sastry
2026-09-09 16:21 ` David Hildenbrand (Arm)
2026-08-19 23:09 ` [PATCH v2 18/22] selftests/coredump: test COREDUMP_RECORDS and COREDUMP_SPARSE Christian Brauner
2026-09-09 16:05 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 19/22] selftests/coredump: hand the record stream to a sink Christian Brauner
2026-09-09 16:05 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 20/22] selftests/coredump: put a hole in the middle of a sparse mapping Christian Brauner
2026-09-09 16:06 ` R Nageswara Sastry
2026-09-09 16:18 ` David Hildenbrand (Arm)
2026-08-19 23:09 ` [PATCH v2 21/22] selftests/coredump: simulate a blob store Christian Brauner
2026-09-09 16:06 ` R Nageswara Sastry
2026-08-19 23:09 ` [PATCH v2 22/22] selftests/coredump: show how to inspect the task to decide how the coredump should be sent Christian Brauner
2026-09-09 16:07 ` R Nageswara Sastry
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=233bec97-49d2-4871-b9a2-150586aead35@linux.ibm.com \
--to=rnsastry@linux.ibm.com \
--cc=akpm@linux-foundation.org \
--cc=brauner@kernel.org \
--cc=david@kernel.org \
--cc=jack@suse.cz \
--cc=jalalonde@fb.com \
--cc=jalalonde@meta.com \
--cc=jannh@google.com \
--cc=josef@toxicpanda.com \
--cc=liam@infradead.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=ljs@kernel.org \
--cc=mhocko@suse.com \
--cc=osandov@osandov.com \
--cc=rppt@kernel.org \
--cc=shuah@kernel.org \
--cc=surenb@google.com \
--cc=vbabka@kernel.org \
--cc=viro@zeniv.linux.org.uk \
/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