Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [linux-next PATCH v3] drivers/virt/fsl_hypervisor: Fix error handling path | From | John Hubbard <> | Date | Mon, 31 Aug 2020 15:58:19 -0700 |
| |
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
| ![\](/images/icornerr.gif) |