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

["Gupta, Pankaj" <pankaj.gupta@amd.com>]

Hi Lorenzo,

>>>>> 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.

The intention is to allow long-term pinning of file-backed mappings only 
for migration avoidance,

without kernel GUP writes, and therefore not impacting dirty tracking.

>>
>> 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?

Yes, I addressed this in my reply, but it wasn't a clean inline response.

>
> 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);
> }

Yes, this looks much better. Will incorporate this.

>
>>          /*
>> @@ -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).

Sure.

Also, I am also planning to rename (FOLL_LONGTERM_HACK -> 
FOLL_PIN_NO_GUP_WRITE) in v2.

Please let me know if you have a preference.

Thanks,

Pankaj

>
>>                  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