Re: [PATCH v2] drm/vmwgfx: Protect pin_user_pages with mmap_lock

[Martin Krastev (VMware)]

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