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 Sun, 11 Nov 2018 13:13:48 -0500
Eric Farman <farman@xxxxxxxxxxxxx> 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.

It's probably better to consider the first two, bug-fixing patches
separately (the complete series is probably not 4.20 material.)

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

Yes, that was my reasoning as well.

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

I'd be happy to merge this patch as-is, but if people think a check
makes things clearer, I'd be happy to merge an updated patch as well.



[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