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.
