linux-kernel - Re: [RFC PATCH v2 3/4] mm/madvise: split out madvise() behavior execution

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bd6d0bf1-c79e-46bd-a810-9791efb9ad73@lucifer.local>
Date: Fri, 31 Jan 2025 16:10:00 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: SeongJae Park <sj@...nel.org>
Cc: "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        David Hildenbrand <david@...hat.com>,
        Shakeel Butt <shakeel.butt@...ux.dev>,
        Vlastimil Babka <vbabka@...e.cz>, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org
Subject: Re: [RFC PATCH v2 3/4] mm/madvise: split out madvise() behavior
 execution

On Thu, Jan 16, 2025 at 05:30:57PM -0800, SeongJae Park wrote:
> Split out the madvise behavior applying logic from do_madvise() to make
> it easier to reuse from the following change.
>
> Signed-off-by: SeongJae Park <sj@...nel.org>

Thanks, once again this is a nice cleanup, and you're doing stuff I thought of
doing but never got round to :P so inevitably:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>

> ---
>  mm/madvise.c | 53 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 30 insertions(+), 23 deletions(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 9cc31efe875a..913936a5c353 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1613,6 +1613,35 @@ static bool is_valid_madvise(unsigned long start, size_t len_in, int behavior)
>  	return true;
>  }
>
> +static int madvise_do_behavior(struct mm_struct *mm,
> +		unsigned long start, size_t len_in, size_t len, int behavior)
> +{
> +	struct blk_plug plug;
> +	unsigned long end;
> +	int error;
> +
> +#ifdef CONFIG_MEMORY_FAILURE
> +	if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE)
> +		return madvise_inject_error(behavior, start, start + len_in);
> +#endif
> +	start = untagged_addr_remote(mm, start);
> +	end = start + len;
> +
> +	blk_start_plug(&plug);
> +	switch (behavior) {

I kind of hate this, I'd like somebody (maybe me maybe you maybe somebody else)
to go further and refactor this thing with fire and fumigation, because having
multiple layers of how we do stuff is just ugh.

Not your fault.

Not even related to your series.

But a moan I shall commit to list nonetheless! ;)

> +	case MADV_POPULATE_READ:
> +	case MADV_POPULATE_WRITE:
> +		error = madvise_populate(mm, start, end, behavior);
> +		break;
> +	default:
> +		error = madvise_walk_vmas(mm, start, end, behavior,
> +					  madvise_vma_behavior);
> +		break;
> +	}
> +	blk_finish_plug(&plug);
> +	return error;
> +}
> +
>  /*
>   * The madvise(2) system call.
>   *
> @@ -1690,7 +1719,6 @@ int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int beh
>  	unsigned long end;
>  	int error;
>  	size_t len;
> -	struct blk_plug plug;
>
>  	if (!is_valid_madvise(start, len_in, behavior))
>  		return -EINVAL;
> @@ -1704,28 +1732,7 @@ int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int beh
>  	error = madvise_lock(mm, behavior);
>  	if (error)
>  		return error;
> -
> -#ifdef CONFIG_MEMORY_FAILURE
> -	if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE)
> -		return madvise_inject_error(behavior, start, start + len_in);
> -#endif
> -
> -	start = untagged_addr_remote(mm, start);
> -	end = start + len;
> -
> -	blk_start_plug(&plug);
> -	switch (behavior) {
> -	case MADV_POPULATE_READ:
> -	case MADV_POPULATE_WRITE:
> -		error = madvise_populate(mm, start, end, behavior);
> -		break;
> -	default:
> -		error = madvise_walk_vmas(mm, start, end, behavior,
> -					  madvise_vma_behavior);
> -		break;
> -	}
> -	blk_finish_plug(&plug);
> -
> +	error = madvise_do_behavior(mm, start, len_in, len, behavior);
>  	madvise_unlock(mm, behavior);
>
>  	return error;
> --
> 2.39.5