Re: [PATCH] KVM: SEV: drop FOLL_LONGTERM for encrypted region registration

[Lorenzo Stoakes <ljs@kernel.org>]

On Tue, Jul 07, 2026 at 04:58:39PM +0200, David Hildenbrand (Arm) wrote:
> On 7/7/26 15:45, Gupta, Pankaj wrote:
> >
> >>> Hi David,
> >>>
> >>> Yes, it fails in this path but for file backed mapping, vma_is_fsdax() returns
> >>> false because
> >>>
> >>> vma_is_dax() returns false:
> >> Ah, okay, so fsdax is not involved and we really only fail because of the
> >> writable_file_mapping_allowed() check.
> >>
> >> I was for a second thinking in terms of nested virt :)
> >>
> >>> Host side backend is regular file backed memory (no fsdax).
> >> Okay, so we'll end up mapping an ordinary file into VM memory, and expose that
> >> to the VM as part of virtio-pmem device.
> >>
> >> That also means that vfio etc. won't be able to longterm-pin such device memory.
> >> So this is not a problem isolated to SEV.
> >>
> >> Forbidding to longterm pin is actually the right thing to do if the filesystem
> >> relies on writenotify, as spelled out by Lorenzo's commit:
> >>
> >> "
> >>      Writing to file-backed mappings which require folio dirty tracking using
> >>      GUP is a fundamentally broken operation, as kernel write access to GUP
> >>      mappings do not adhere to the semantics expected by a file system.
> >>
> >>      A GUP caller uses the direct mapping to access the folio, which does not
> >>      cause write notify to trigger, nor does it enforce that the caller marks
> >>      the folio dirty.
> >>
> >>      The problem arises when, after an initial write to the folio, writeback
> >>      results in the folio being cleaned and then the caller, via the GUP
> >>      interface, writes to the folio again.
> >> "
> >>
> >> Hmmm
> >
> > Yes. For file based mapping we don't allow long term pinning.
> >
> > If we take into account the fragmentation concerns for MIGRATE_CMA and
> > ZONE_MOVABLE allocations
> >
> > solvable with FOLL_LONGTERM, I can think of two options(tested) to allow file
> > based mappings as well:
> >
> > 1. Fallback on FOLL_WRITE when FOLL_LONGTERM fails as suggested by Sean.
>
> That is just not acceptable, as it breaks random other stuff (MIGRATE_CMA, as
> one example) besides the file-pinning problems that Lorenzo added.
>
> If we're going to hack something in, then that we bypass the file writeback check.
> Not that we don't use FOLL_LONGTERM.
>
> I'd hate to use a GUP flag to indicate "this is a legacy hack", but it clearly isolates the
> issue (needs a better name obviously):

So under what circumstances are we happy with totally breaking dirty tracking?
:/ seems iffy, and exposing this to drivers generally is a bit worrysome.

>
>
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index ae9bca4eda5ca..e2c531f914d44 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -1912,6 +1912,9 @@ enum {
>          */
>         FOLL_HONOR_NUMA_FAULT = 1 << 12,
>
> +       /* TODO */
> +       FOLL_LONGTERM = 1 << 13,
> +
>         /* See also internal only FOLL flags in mm/internal.h */
>  };
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 0692119b79043..1fa0aa0cdc99d 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1186,8 +1186,8 @@ static bool writable_file_mapping_allowed(struct vm_area_struct *vma,
>          * If we aren't pinning then no problematic write can occur. A long term
>          * pin is the most egregious case so this is the case we disallow.
>          */
> -       if ((gup_flags & (FOLL_PIN | FOLL_LONGTERM)) !=
> -           (FOLL_PIN | FOLL_LONGTERM))
> +       if ((gup_flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_LONGTERM_HACK)) !=
> +           (FOLL_PIN | FOLL_LONGTERM | FOLL_LONGTERM_HACK))
>                 return true;

Hmm I'm confused, you're then allowing FOLL_PIN | FOLL_LONGTERM, but disallowing
FOLL_PIN | FOLL_LONGTERM | FOLL_LONGTERM_HACK?

By the way I think this should be expressed better if I criticise myself here :)

So like:

	if ((gup_flags & FOLL_PIN) && (gup_flags & FOLL_LONGTERM))

Or even:

	/* Only an issue if we pin... */
	if (!(gup_flags & FOLL_PIN))
		return false;
	/* ...and that pin is longterm... */
	if (!(gup_flags & FOLL_LONGTERM))
		return false;

But I'm confused as to why we are suddenly allowing something broken and what
this hack flag is supposed to achieve?

Shouldn't this rather be:

	/* Only an issue if we pin... */
	if (!(gup_flags & FOLL_PIN))
		return true;
	/* ...and that pin is longterm... */
	if (!(gup_flags & FOLL_LONGTERM))
		return true;
	/* ...and not overridden... */
	if (gup_flags & FOLL_LONGTERM_HACK)
		return true;
	/* ...and dirty tracking is required. */
	return !vma_needs_dirty_tracking(vma);
}

>
>         /*
> @@ -2746,7 +2746,7 @@ static bool gup_fast_folio_allowed(struct folio *folio, unsigned int flags)
>          * If we aren't pinning then no problematic write can occur. A long term
>          * pin is the most egregious case so this is the one we disallow.
>          */
> -       if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) ==
> +       if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE | FOLL_LONGTERM_HACK)) ==
>             (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE))

Yeah this is just a bit horrid having to stare at a this a while... So
FOLL_LONGTERM_HACK would enable here.

Be nice to avoid this form of it as it's difficult to understand, do something
like above or a clearer version anyway (probably best abstracted to a small
function).

>                 reject_file_backed = true;
>
> @@ -3180,7 +3180,7 @@ static int gup_fast_fallback(unsigned long start, unsigned long nr_pages,
>         int locked = 0;
>         int ret;
>
> -       if (WARN_ON_ONCE(gup_flags & ~(FOLL_WRITE | FOLL_LONGTERM |
> +       if (WARN_ON_ONCE(gup_flags & ~(FOLL_WRITE | FOLL_LONGTERM | FOLL_LONGTERM_HACK |
>                                        FOLL_FORCE | FOLL_PIN | FOLL_GET |
>                                        FOLL_FAST_ONLY | FOLL_NOFAULT |
>                                        FOLL_PCI_P2PDMA | FOLL_HONOR_NUMA_FAULT)))
>
>
> --
> Cheers,
>
> David

Thanks, Lorenzo