| Message ID | TYCP286MB23235B138D18ECA0797A6D0FCA3D9@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/vmwgfx: Protect pin_user_pages with mmap_lock | expand |
From: Martin Krastev <krastevm@vmware.com> 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@outlook.com> > --- > v1: > https://lore.kernel.org/all/TYCP286MB23235C9A9FCF85C045F95EA7CA4F9@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM/ > > 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); >
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);
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@outlook.com> --- v1: https://lore.kernel.org/all/TYCP286MB23235C9A9FCF85C045F95EA7CA4F9@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM/ v1->v2: Rebased to latest vmwgfx/drm-misc-fixes --- drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)