Re: [RFC v2 2/5] vfio-ccw: Fix memory leak and don't call cp_free in cp_init

Farhan Ali
2019-07-09T14:08:03Z

On 07/09/2019 06:06 AM, Cornelia Huck wrote:
> On Mon, 8 Jul 2019 16:10:35 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
>
>> We don't set cp->initialized to true so calling cp_free
>> will just return and not do anything.
>>
>> Also fix a memory leak where we fail to free a ccwchain
>> on an error.
>>
>> Fixes: 812271b910 ("s390/cio: Squash cp_free() and cp_unpin_free()")
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> ---
>> drivers/s390/cio/vfio_ccw_cp.c | 11 +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> (...)
>
>> @@ -642,8 +647,6 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
>>
>> /* Build a ccwchain for the first CCW segment */
>> ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
>> - if (ret)
>> - cp_free(cp);
>
> Now that I look again: it's a bit odd that we set the bit in all cases,
> even if we have an error. We could move that into the !ret branch that
> sets ->initialized; but it does not really hurt.

By setting the bit, I am assuming you meant cmd.c64?

Yes, it doesn't harm anything but for better code readability you have a
good point. I will move it into !ret branch in the first patch since I
think it would be more appropriate there, no?

>
>>
>> /* It is safe to force: if it was not set but idals used
>> * ccwchain_calc_length would have returned an error.
>
> The rest of the patch looks good to me.
>
>