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 13:38:46 +0100
Halil Pasic <pasic@xxxxxxxxxxxxx> wrote:

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

So we'll go with this patch and defer any further rework to a v2 of the
complete series, ok?

I'll queue this patch, then; unless someone complains, I'll send a pull
request for vfio-ccw tomorrow.



[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