Re: [PATCH 2/5] mm/mseal: move madvise() logic to mm/madvise.c

[David Hildenbrand <david@redhat.com>]

On 14.07.25 15:00, Lorenzo Stoakes wrote:
> The madvise() logic is inexplicably performed in mm/mseal.c - this ought to
> be located in mm/madvise.c.
> 
> Additionally can_modify_vma_madv() is inconsistently named and, in
> combination with is_ro_anon(), is very confusing logic.
> 
> Put a static function in mm/madvise.c instead - can_madvise_modify() - that
> spells out exactly what's happening. Also explicitly check for an anon VMA.
> 
> Additionally add commentary to explain what's going on.
> 
> No functional change intended.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>   mm/madvise.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>   mm/mseal.c   | 49 -----------------------------------------
>   mm/vma.h     |  7 ------
>   3 files changed, 61 insertions(+), 57 deletions(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 9de9b7c797c6..75757ba418a8 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -19,6 +19,7 @@
>   #include <linux/sched.h>
>   #include <linux/sched/mm.h>
>   #include <linux/mm_inline.h>
> +#include <linux/mmu_context.h>
>   #include <linux/string.h>
>   #include <linux/uio.h>
>   #include <linux/ksm.h>
> @@ -1256,6 +1257,65 @@ static long madvise_guard_remove(struct madvise_behavior *madv_behavior)
>   			       &guard_remove_walk_ops, NULL);
>   }
> 
> +#ifdef CONFIG_64BIT

It's consistent with mm/Makefile etc. but having a simple

config MSEAL
	def_bool y if 64BIT

or sth like that would surely clean that up further.

> +/* Does the madvise operation result in discarding of mapped data? */
> +static bool is_discard(int behavior)
> +{
> +	switch (behavior) {
> +	case MADV_FREE:
> +	case MADV_DONTNEED:
> +	case MADV_DONTNEED_LOCKED:
> +	case MADV_REMOVE:
> +	case MADV_DONTFORK:
> +	case MADV_WIPEONFORK:
> +	case MADV_GUARD_INSTALL:
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +/*
> + * We are restricted from madvise()'ing mseal()'d VMAs only in very particular
> + * circumstances - discarding of data from read-only anonymous SEALED mappings.
> + *
> + * This is because users cannot trivally discard data from these VMAs, and may

s/trivally/trivially/

> + * only do so via an appropriate madvise() call.
> + */
> +static bool can_madvise_modify(struct madvise_behavior *madv_behavior)
> +{
> +	struct vm_area_struct *vma = madv_behavior->vma;
> +
> +	/* If the operation won't discard, we're good. */
> +	if (!is_discard(madv_behavior->behavior))
> +		return true;


Conceptually, I would do this first and then handle all the discard 
cases / exceptions.

> +
> +	/* If the VMA isn't sealed we're also good. */
> +	if (can_modify_vma(vma))
> +		return true;
 > +> +	/* File-backed mappings don't lose data on discard. */
> +	if (!vma_is_anonymous(vma))
> +		return true;
> +
> +	/*
> +	 * If the VMA is writable and the architecture permits write access,
> +	 * then discarding is fine.
> +	 */
> +	if ((vma->vm_flags & VM_WRITE) &&
> +	    arch_vma_access_permitted(vma, /* write= */ true,
> +			/* execute= */ false, /* foreign= */ false))
> +		return true;
> +
> +	return false;
> +}
> +#else
> +static bool can_madvise_modify(struct madvise_behavior *madv_behavior)
> +{
> +	return true;
> +}
> +#endif
> +
>   /*

LGTM

-- 
Cheers,

David / dhildenb