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 Mon, 12 Nov 2018 11:00:55 +0100
Cornelia Huck <cohuck@xxxxxxxxxx> wrote:

> From: Cornelia Huck <cohuck@xxxxxxxxxx>
> To: Eric Farman <farman@xxxxxxxxxxxxx>
> Cc: Farhan Ali <alifm@xxxxxxxxxxxxx>, Halil Pasic
> <pasic@xxxxxxxxxxxxx>, Pierre Morel <pmorel@xxxxxxxxxxxxx>,
> linux-s390@xxxxxxxxxxxxxxx, kvm@xxxxxxxxxxxxxxx, "Jason J . Herne"
> <jjherne@xxxxxxxxxxxxx> Subject: Re: [RFC PATCH v1 01/10] s390/cio:
> Fix cleanup of pfn_array alloc failure Date: Mon, 12 Nov 2018
> 11:00:55 +0100 Sender: linux-s390-owner@xxxxxxxxxxxxxxx Organization:
> Red Hat GmbH
> 
> 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>

Acked-by: Halil Pasic <pasic@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.)

I second that! From what I've seen up until now, it will take time to
properly review the complete series. The two bugfixes in turn are easy
to understand.

Halil




[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