Re: [PATCH 1/4] vfio: ccw: fix cleanup if cp_prefetch fails

[Cornelia Huck]

On Thu, 22 Mar 2018 10:37:36 +0100
Pierre Morel <pmorel@xxxxxxxxxxxxxxxxxx> wrote:

> On 22/03/2018 03:22, Dong Jia Shi wrote:
> > * Halil Pasic <pasic@xxxxxxxxxxxxxxxxxx> [2018-03-21 13:49:54 +0100]:
> >  
> >>
> >> On 03/21/2018 03:08 AM, Dong Jia Shi wrote:  
> >>> From: Halil Pasic <pasic@xxxxxxxxxxxxxxxxxx>
> >>>
> >>> If the translation of a channel program fails, we may end up attempting
> >>> to clean up (free, unpin) stuff that never got translated (and allocated,
> >>> pinned) in the first place.
> >>>
> >>> By adjusting the lengths of the chains accordingly (so the element that
> >>> failed, and all subsequent elements are excluded) cleanup activities
> >>> based on false assumptions can be avoided.
> >>>
> >>> Let's make sure cp_free works properly after cp_prefetch returns with an
> >>> error by setting ch_len to 0 for the ccw chains those are not prefetched.  
> >> This sentence used to be:
> >>
> >> Let's make sure cp_free works properly after cp_prefetch returns with an
> >> error.
> >>
> >> @Dong Jia
> >> I find the 'by setting ch_len to 0 for the ccw chains those are not prefetched'
> >> you added for clarification (I guess) somewhat problematic.
> >> The chain in which the translation failure occurred
> >> +	chain->ch_len = idx;  
> > I made a mistake. When rewording the message, I missed this part...
> > Sorry for the problem!
> >  
> >> is shortened so that only the translated elements (ccws) are going to
> >> get cleaned up (on a per element basis) by cp_free. This may or may
> >> not be the first ccw. Subsequent chains are shortened to 0 as there
> >> no translation took place.
> >>
> >> So as a result of this change only properly translated ccws are going
> >> to get (re)visited by cp_free as only those may have resources bound
> >> to them which need to be released.
> >>
> >> I'm not against improving the commit message. But this ain't
> >> an improvement to me.  
> > You are right. How about:
> > Let's make sure cp_free works properly after cp_prefetch returns with an
> > error by setting ch_len of a ccw chain to the number of the translated
> > ccws on that chain.  
> 
> By the way, since you will propose a new version,
> you have a long description of the cp_prefetch function in the code.
> I think you should modify it according to the changes and describe how and
> why the ch_len field of each chain is used and changed by this function.
> 
> Something like:
> 
> "
> For each chain composing the channel program:
> On entry ch_len hold the count of CCW to be translated.

s/hold/holds/ ?

> On exit ch_len is adjusted to the count of successfully translated CCW.
> 
> This allows cp_free to find in ch_len the count of CCW to free in a chain.
> "
> 
> Could also be inside the commit message.
> 
> Pierre
> 
> 
> >  
> >>> Acked-by: Pierre Morel <pmorel@xxxxxxxxxxxxxxxxxx>
> >>> Reviewed-by: Dong Jia Shi <bjsdjshi@xxxxxxxxxxxxxxxxxx>
> >>> Signed-off-by: Halil Pasic <pasic@xxxxxxxxxxxxxxxxxx>
> >>> Signed-off-by: Dong Jia Shi <bjsdjshi@xxxxxxxxxxxxxxxxxx>
> >>> ---
> >>>   drivers/s390/cio/vfio_ccw_cp.c | 9 ++++++++-
> >>>   1 file changed, 8 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> >>> index d9a2fffd034b..2be114db02f9 100644
> >>> --- a/drivers/s390/cio/vfio_ccw_cp.c
> >>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> >>> @@ -749,11 +749,18 @@ int cp_prefetch(struct channel_program *cp)
> >>>   		for (idx = 0; idx < len; idx++) {
> >>>   			ret = ccwchain_fetch_one(chain, idx, cp);
> >>>   			if (ret)
> >>> -				return ret;
> >>> +				goto out_err;
> >>>   		}
> >>>   	}
> >>>
> >>>   	return 0;
> >>> +out_err:
> >>> +	/* Only cleanup the chain elements that where actually translated. */

s/where/were/

> >>> +	chain->ch_len = idx;
> >>> +	list_for_each_entry_continue(chain, &cp->ccwchain_list, next) {
> >>> +		chain->ch_len = 0;
> >>> +	}
> >>> +	return ret;
> >>>   }
> >>>
> >>>   /**
> >>>  
>