On Mon, 27 Apr 2026 09:43:30 +0000 fujunjie <[email protected]> wrote:
> process_madvise() used to validate the advice while walking each
> imported iovec. If the vector has zero total length, vector_madvise()
> does not enter the loop and can return success without checking whether
> the advice value is valid.
>
> For a local mm, such as process_madvise(PIDFD_SELF, ...), the remote-only
> process_madvise_remote_valid() check is skipped. As a result, an invalid
> advice can be reported as success when the vector has zero total length.
> This differs from madvise(), which rejects an invalid advice before
> returning success for a zero-length range.
>
> Validate the generic madvise behavior at the syscall-facing entry points
> before any vector walk. In process_madvise(), do this before the
> remote-only advice restriction so unsupported advice is rejected with the
> same priority for local and remote mm. Then keep the per-range helper
> focused on address/length validation, avoiding repeated behavior checks
> for every iovec.
>
> Valid zero-length requests remain no-ops and continue to return 0. Add a
> selftest that covers invalid advice with a zero-length iovec and an empty
> vector, while also checking that a valid zero-length request still
> succeeds.
>
> Fixes: 021781b01275 ("mm/madvise: unrestrict process_madvise() for current
> process")
> Signed-off-by: fujunjie <[email protected]>
Looks good to me. I have trivial comments below, though. Because those are
really trivial, please feel free to add
Reviewed-by: SeongJae Park <[email protected]>
> ---
[...]
> *
> - * If the specified behaviour is invalid or nothing would occur, we skip the
> - * operation. This function returns true in the cases, otherwise false. In
> - * the former case we store an error on @err.
> + * If the specified range is invalid or nothing would occur, we skip the
> + * operation. This function returns true in these cases, otherwise false.
> In
> + * the former case we store an error in @err.
Maybe we can keep the second and the third lines of the above comment
unchanged?
[...]
> +/*
> + * Test that invalid advice is rejected even when the iovec has zero total
> + * length. A zero-length advice is a no-op for valid advice, but invalid
> + * advice should still fail with EINVAL.
> + */
> +TEST_F(process_madvise, invalid_advice_zero_length)
> +{
> + struct iovec vec = {
> + .iov_base = NULL,
> + .iov_len = 0,
> + };
> + int pidfd = self->pidfd;
> + ssize_t ret;
> +
> + errno = 0;
> + ret = sys_process_madvise(pidfd, &vec, 1, -1, 0);
> + ASSERT_EQ(ret, -1);
> + ASSERT_EQ(errno, EINVAL);
> +
> + errno = 0;
> + ret = sys_process_madvise(pidfd, &vec, 1, MADV_DONTNEED, 0);
> + ASSERT_EQ(ret, 0);
> +
> + errno = 0;
The previous sys_process_madvise() is expected to not set errno, correct?
Maybe the above 'errno' reassignment is unnecessary?
> + ret = sys_process_madvise(pidfd, NULL, 0, -1, 0);
> + ASSERT_EQ(ret, -1);
> + ASSERT_EQ(errno, EINVAL);
> +}
Thanks,
SJ
[...]
