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

Message ID fbb44bc85f5dfe4fdaebaf9cb74efcfae4743fba.1562616169.git.alifm@linux.ibm.com
State New
Headers show
Series
  • Some vfio-ccw fixes
Related show

Commit Message

Farhan Ali July 8, 2019, 8:10 p.m. UTC
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(-)

Comments

Cornelia Huck July 9, 2019, 10:06 a.m. UTC | #1
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.

>  
>  	/* 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.
Farhan Ali July 9, 2019, 2:07 p.m. UTC | #2
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.
> 
>
Cornelia Huck July 9, 2019, 2:18 p.m. UTC | #3
On Tue, 9 Jul 2019 10:07:57 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> 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?

Yes to all of that :)

> 
> >   
> >>   
> >>   	/* 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.
> > 
> >   
>

Patch
diff mbox series

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 7622b72..31a04a5 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -421,7 +421,7 @@  static int ccwchain_loop_tic(struct ccwchain *chain,
 static int ccwchain_handle_ccw(u32 cda, struct channel_program *cp)
 {
 	struct ccwchain *chain;
-	int len;
+	int len, ret;
 
 	/* Copy 2K (the most we support today) of possible CCWs */
 	len = copy_from_iova(cp->mdev, cp->guest_cp, cda,
@@ -448,7 +448,12 @@  static int ccwchain_handle_ccw(u32 cda, struct channel_program *cp)
 	memcpy(chain->ch_ccw, cp->guest_cp, len * sizeof(struct ccw1));
 
 	/* Loop for tics on this new chain. */
-	return ccwchain_loop_tic(chain, cp);
+	ret = ccwchain_loop_tic(chain, cp);
+
+	if (ret)
+		ccwchain_free(chain);
+
+	return ret;
 }
 
 /* Loop for TICs. */
@@ -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);
 
 	/* It is safe to force: if it was not set but idals used
 	 * ccwchain_calc_length would have returned an error.