On 1/15/25 1:59 PM, Claudio Imbrenda wrote:
On Wed, 15 Jan 2025 13:48:47 +0100
Janosch Frank <frankja@xxxxxxxxxxxxx> wrote:
[...]
+static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
+{
+ struct folio *folio = page_folio(page);
+ int rc;
+
+ /*
+ * Secure pages cannot be huge and userspace should not combine both.
+ * In case userspace does it anyway this will result in an -EFAULT for
+ * the unpack. The guest is thus never reaching secure mode. If
+ * userspace is playing dirty tricky with mapping huge pages later
s/tricky/tricks/
But the whole last sentence is a bit iffy.
hmm yes I'll reword it
+ * on this will result in a segmentation fault or in a -EFAULT return
+ * code from the KVM_RUN ioctl.
+ */
+ if (folio_test_hugetlb(folio))
+ return -EFAULT;
+ if (folio_test_large(folio)) {
+ mmap_read_unlock(gmap->mm);
+ rc = uv_wiggle_folio(folio, true);
+ mmap_read_lock(gmap->mm);
You could move the unlock to uv_wiggle_folio() and add a
mmap_assert_locked() in front.
oh no, I don't want a function that drops a lock that has been acquired
outside of it.
by explicitly dropping and acquiring it, it's obvious what's going on,
and you can easily see that the lock is being dropped and re-acquired.
__gmap_destroy_page() does it, but it's called exactly in one spot,
namely gmap_destroy_page(), which is literally below it.
It's just very weird to to me to have the (un)lock dance the wrong way
around when reading it. At first I assumed you made a mistake when
remembering the context description in wiggle because I misread the
unlock() as lock().
But maybe that's just me and I don't mind it too much :)
