Re: [RFC PATCH v1 01/10] s390/cio: Fix cleanup of pfn_array alloc failure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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

Thanks
Farhan




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux