Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [linux-next PATCH v3] drivers/virt/fsl_hypervisor: Fix error
 handling path
To:     Souptick Joarder <jrdr.linux@gmail.com>,
        <akpm@linux-foundation.org>
CC:     <jgg@ziepe.ca>, <dan.j.williams@intel.com>,
        <gregkh@linuxfoundation.org>, <arnd@arndb.de>,
        <timur@freescale.com>, <galak@kernel.crashing.org>,
        <linux-kernel@vger.kernel.org>
References: <1598911668-6178-1-git-send-email-jrdr.linux@gmail.com>
From:   John Hubbard <jhubbard@nvidia.com>
Message-ID: <82329783-739b-a315-8957-2c49a3ab1350@nvidia.com>
Date:   Mon, 31 Aug 2020 15:58:19 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <1598911668-6178-1-git-send-email-jrdr.linux@gmail.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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