Re: [linux-next PATCH v3] drivers/virt/fsl_hypervisor: Fix error handling path

[John Hubbard]

On 8/31/20 3:07 PM, Souptick Joarder wrote:
> First, when memory allocation for sg_list_unaligned failed, there
> is a bug of calling put_pages() as we haven't pinned any pages.

"we should unpin"

...
>   
> @@ -250,7 +250,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
>   		num_pages, param.source != -1 ? FOLL_WRITE : 0, pages);
>   
>   	if (num_pinned != num_pages) {
> -		/* get_user_pages() failed */
> +		/* get_user_pages_fast() failed */

Let's please just delete that particular comment entirely. It's of
questionable accuracy (partial success is allowed with this API), and it
is echoing the code too closely to be worth the line that it consumes.

More importantly, though, we need to split up the cases of gup_fast
returning a negative value, and a zero or positive value. Either here,
or at "exit:", the negative return case should just skip any attempt to
do any put_page() calls at all. Because it's a maintenance hazard to
leave in a loop that depends on looping from zero, to -ERRNO, and *not*
doing any loops--especially in the signed/unsigned soupy mess around gup
calls.


>   		pr_debug("fsl-hv: could not lock source buffer\n");
>   		ret = (num_pinned < 0) ? num_pinned : -EFAULT;
>   		goto exit;
> @@ -293,12 +293,12 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
>   
>   exit:
>   	if (pages) {
> -		for (i = 0; i < num_pages; i++)
> -			if (pages[i])
> -				put_page(pages[i]);
> +		for (i = 0; i < num_pinned; i++)
> +			put_page(pages[i]);

Looks correct. I sometimes wonder why more callers don't use
release_pages() in situations like this, but that's beyond the scope of
your work here.


thanks,
--
John Hubbard
NVIDIA