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/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




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux