On 11/11/2018 19:13, Eric Farman wrote:
On 11/09/2018 04:19 PM, Farhan Ali wrote:
On 11/09/2018 12:01 PM, Halil Pasic wrote:
On Fri, 9 Nov 2018 09:49:22 -0500
Eric Farman <farman@xxxxxxxxxxxxx> wrote:
On 11/09/2018 05:45 AM, Cornelia Huck wrote:
On Fri, 9 Nov 2018 03:39:28 +0100
Eric Farman <farman@xxxxxxxxxxxxx> wrote:
If pfn_array_alloc fails somehow, we need to release the
pfn_array_table that was malloc'd earlier.
Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
---
drivers/s390/cio/vfio_ccw_cp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/s390/cio/vfio_ccw_cp.c
b/drivers/s390/cio/vfio_ccw_cp.c index fd77e46eb3b2..ef5ab45d94b3
100644 --- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -528,7 +528,7 @@ static int ccwchain_fetch_direct(struct
ccwchain *chain,
ret = pfn_array_alloc_pin(pat->pat_pa, cp->mdev,
ccw->cda, ccw->count); if (ret < 0)
- goto out_init;
+ goto out_unpin;
/* Translate this direct ccw to a idal ccw. */
idaws = kcalloc(ret, sizeof(*idaws), GFP_DMA |
GFP_KERNEL);
It's a bit confusing that this will also call vfio_unpin_pages()
even though there should not be any pinned pages at that point in
time; but from what I see, it should not hurt, so this patch is
fine.
Yeah, confusing indeed. The problem today is that we don't undo the
pfn_array_table_init() call that happened prior to this error, and so
there would be a leak of the pat->pat_pa memory. So going to
out_init is certainly not right.
In the pfn_array patches later, I do conditionally call
vfio_unpin_pages() based on the contents of pfn_array->pa_iova, to
avoid the scenario you describe.
- Eric
Quite confusing and generally awful. I will try to figure out the
before-after on a series level, and then consider the individual
patches in detail. The in between states are predestined to look awful
because of the current state.
Regards,
Halil
The fix is correct but yeah we are unpinning pages when we shouldn't
have anymore pinned pages.
Maybe add an extra check in pfn_array_unpin_free to see if pa_iova_pfn
is null? I don't know if it would make it even more confusing :)
I know I put this check in later, but I think it's a
belts-and-suspenders situation. If pfn_array_alloc_pin failed, then we
took one of three error exits:
1) pa->pa_nr is set to zero, based on the input length
2) pa->pa_iova_pfn is zero, because of -ENOMEM
3) vfio_pin_pages() failed somehow, so on cleanup we set pa->pa_nr to
zero, kfree(pa->pa_iova_pfn), and set pa->pa_iova_pfn to NULL
Since both pa_nr (npages) and pa_iova_pfn (user_pfn) are zero/NULL, the
unpin routine will exit out early with -EINVAL. We don't check the
return code from vfio_unpin_pages, but that's fine since we're already
in an error path.
I'm not opposed to a check here, so can spin a v2 of this patch if
desired. But I'm not as concerned about it with the state of the code
on this patch.
- Eric
Hi Eric,
I think the problem here comes from the pfn_array_table_unpin_free()
doing both unpin and free.
I would prefer that you change the pfn_array_table_init() to return the
pointer, so you can free the pointer in the caller like:
p = pfn_array_table_init(pat, 1);
if (!p)
goto out_init;
ret = pfn_array_alloc_pin(pat->pat_pa, cp->mdev, ccw->cda,
ccw->count);
if (ret < 0)
goto out_free;
...
out_unpin:
pfn_array_table_unpin_free(pat, cp->mdev);
out_free:
pfn_array_table_free(p);
out_init:
ccw->cda = 0;
return ret;
}
And modify the pfn_array_table_unpin_free() to pfn_array_table_unpin()
and add the freeing in pfn_array_table_free(p).
Something like that with a correct return code handle.
Which would make the code more logical and readable.
What do you think?
Regards,
Pierre
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
