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.
