diff mbox series

[v2] drm/vmwgfx: Protect pin_user_pages with mmap_lock

Message ID TYCP286MB23235B138D18ECA0797A6D0FCA3D9@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM
State New
Headers
Series [v2] drm/vmwgfx: Protect pin_user_pages with mmap_lock |

Commit Message

Dawei Li Nov. 6, 2022, 3:47 p.m. UTC 
  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(-)

Comments

Martin Krastev (VMware) Nov. 7, 2022, 3:16 p.m. UTC | #1 
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 mbox series

Patch

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