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
