From: Martin Krastev <krastevm@xxxxxxxxxx>
Thanks for the catch. LGTM, with the following remarks:
1) Original design used erroneously pin_user_pages() in place of
pin_user_pages_fast(); you could just substitute pin_user_pages for
pin_user_pages_fast and call it a day, Please, consider that option
after reading (2) below.
2) Re exception handling in vmw_mksstat_add_ioctl(), the 'incorrect
exception handling' would be incorrect in the context of the new
refactor, i.e. with a common entry point to all pin_user_pages()
exceptions; it was correct originally, with dedicated entry points, as
all nr_pinned_* were used only after being assigned.
Basically, you could keep everything as it was and just do the
substitution suggested in (1) and the patch would be as good.
Regards,
Martin
On 6.11.22 г. 17:47 ч., Dawei Li wrote:
This patch includes changes below:
1) pin_user_pages() is unsafe without protection of mmap_lock,
fix it by calling mmap_read_lock() & mmap_read_unlock().
2) fix & refactor the incorrect exception handling procedure in
vmw_mksstat_add_ioctl().
Fixes: 7a7a933edd6c ("drm/vmwgfx: Introduce VMware mks-guest-stats")
Signed-off-by: Dawei Li <set_pte_at@xxxxxxxxxxx>
---
v1:
https://lore.kernel.org/all/TYCP286MB23235C9A9FCF85C045F95EA7CA4F9@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
v1->v2:
Rebased to latest vmwgfx/drm-misc-fixes
---
drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
index 089046fa21be..ec40a3364e0a 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
@@ -1020,9 +1020,9 @@ int vmw_mksstat_add_ioctl(struct drm_device *dev, void *data,
const size_t num_pages_info = PFN_UP(arg->info_len);
const size_t num_pages_strs = PFN_UP(arg->strs_len);
long desc_len;
- long nr_pinned_stat;
- long nr_pinned_info;
- long nr_pinned_strs;
+ long nr_pinned_stat = 0;
+ long nr_pinned_info = 0;
+ long nr_pinned_strs = 0;
struct page *pages_stat[ARRAY_SIZE(pdesc->statPPNs)];
struct page *pages_info[ARRAY_SIZE(pdesc->infoPPNs)];
struct page *pages_strs[ARRAY_SIZE(pdesc->strsPPNs)];
@@ -1084,28 +1084,33 @@ int vmw_mksstat_add_ioctl(struct drm_device *dev, void *data,
reset_ppn_array(pdesc->infoPPNs, ARRAY_SIZE(pdesc->infoPPNs));
reset_ppn_array(pdesc->strsPPNs, ARRAY_SIZE(pdesc->strsPPNs));
+ /* pin_user_pages() needs protection of mmap_lock */
+ mmap_read_lock(current->mm);
+
/* Pin mksGuestStat user pages and store those in the instance descriptor */
nr_pinned_stat = pin_user_pages(arg->stat, num_pages_stat, FOLL_LONGTERM, pages_stat, NULL);
if (num_pages_stat != nr_pinned_stat)
- goto err_pin_stat;
+ goto __err_pin_pages;
for (i = 0; i < num_pages_stat; ++i)
pdesc->statPPNs[i] = page_to_pfn(pages_stat[i]);
nr_pinned_info = pin_user_pages(arg->info, num_pages_info, FOLL_LONGTERM, pages_info, NULL);
if (num_pages_info != nr_pinned_info)
- goto err_pin_info;
+ goto __err_pin_pages;
for (i = 0; i < num_pages_info; ++i)
pdesc->infoPPNs[i] = page_to_pfn(pages_info[i]);
nr_pinned_strs = pin_user_pages(arg->strs, num_pages_strs, FOLL_LONGTERM, pages_strs, NULL);
if (num_pages_strs != nr_pinned_strs)
- goto err_pin_strs;
+ goto __err_pin_pages;
for (i = 0; i < num_pages_strs; ++i)
pdesc->strsPPNs[i] = page_to_pfn(pages_strs[i]);
+ mmap_read_unlock(current->mm);
+
/* Send the descriptor to the host via a hypervisor call. The mksGuestStat
pages will remain in use until the user requests a matching remove stats
or a stats reset occurs. */
@@ -1120,15 +1125,15 @@ int vmw_mksstat_add_ioctl(struct drm_device *dev, void *data,
return 0;
-err_pin_strs:
+__err_pin_pages:
+ mmap_read_unlock(current->mm);
+
if (nr_pinned_strs > 0)
unpin_user_pages(pages_strs, nr_pinned_strs);
-err_pin_info:
if (nr_pinned_info > 0)
unpin_user_pages(pages_info, nr_pinned_info);
-err_pin_stat:
if (nr_pinned_stat > 0)
unpin_user_pages(pages_stat, nr_pinned_stat);
